Skip to content

fix(data-fabric): valueList filters on join queries + page-independent join tests - #729

Open
Sarath1018 wants to merge 2 commits into
mainfrom
test/join-tests-page-independence
Open

fix(data-fabric): valueList filters on join queries + page-independent join tests#729
Sarath1018 wants to merge 2 commits into
mainfrom
test/join-tests-page-independence

Conversation

@Sarath1018

@Sarath1018 Sarath1018 commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Fix valueList filters on join queries; make join tests page-independent

Problem 1 — deterministic join-test failures on CI (test design)

The LEFT/INNER join tests queried integrationTestEntity with pageSize: 25 and asserted that at least one returned row carries the joined entity's qualified keys. The seeded match is one of the oldest rows in the fixture and the API returns rows newest-first — so whenever leaked records accumulate on the shared fixture (e.g. from storm-aborted CI runs), the seed is pushed out of page 1 and the tests fail deterministically:

AssertionError: expected 0 to be greater than 0

A raw probe of POST /api/EntityService/integrationTestEntity/query confirmed the join route itself works (HTTP 200, totalRecordCount: 771) — page 1 was simply 25 rows of unrelated leaked records with no joined keys. This is what has been failing the coverage job on #727 and #663.

Problem 2 — the SDK's valueList filters are broken on join queries (product bug, found while fixing 1)

The obvious page-independent design — filter the base query to the related entity's join values with operator: In + valueList — failed with:

ValidationError: Multi-entity query failed. Filter value null can only be used with operator = or !=. Field: SomeText

Live probes isolated the contract difference: the multi-entity (joins) parser is the only query route that ignores valueList and requires value as a JSON-stringified array ("value": "[\"a\",\"b\"]"); the by-id route and the no-joins by-name route both accept valueList natively. Since queryRecordsById transparently reroutes join queries to the multi-entity route, any caller following the SDK's documented contract ("For In/NotIn operators, use valueList instead of value") gets a 400 on every join query.

Fix: toWireFilterGroup() in entities.ts — applied only on the joins path, next to the existing toWireJoin() translation — serializes valueList into the value JSON-array form, recursing into nested filter groups. Non-join queries are untouched.

Test changes

entities-query.integration.test.ts — join tests wrapped in a cross-entity joins describe with a shared beforeAll that:

  • fetches the related entity's join values once and filters both join queries with In(valueList), so the assertions are fixture-driven and independent of page position;
  • seeds one base row with a join value that matches nothing on the related entity (registered for cleanup), so LEFT vs INNER produce provably different result sets — the INNER test asserts left.totalCount === inner.totalCount + 1 (exactly the seeded row apart), which restores the INNER-semantics guard the filter would otherwise have weakened and stays valid at any fixture size, immune to the page cap;
  • polls the exact query shape until the seeded row is queryable (bounded, warns per miss, throws at exhaustion), per the sanctioned readiness pattern.

New unit tests cover the translation (flat + nested groups, valueList key removed) and the no-joins passthrough.

Validation

  • typecheck, oxlint, unit suite (285 tests) all clean; rollup build + docs:validate clean.
  • Live run of entities-query.integration.test.ts against the alpha tenant passes 16/16 — with the fixture sized so the seeded match is not in page 1, which directly proves page-independence.

🤖 Generated with Claude Code

…in route

The multi-entity (joins) query parser is the only route that ignores
EntityQueryFilter.valueList and requires value as a JSON-stringified
array — so any caller following the documented In/NotIn contract got a
400 on every join query. Translate valueList to that wire form on the
joins path only, recursing into nested groups.

Also make the join integration tests page-independent: filter the base
query to the related entity's join values plus one seeded no-match row,
so leaked records on the shared fixture can no longer push the seeded
match out of the page window (the failure blocking #727 and #663), and
the seeded row keeps the LEFT vs INNER contrast provable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Sarath1018
Sarath1018 requested a review from a team September 10, 2026 02:46
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

- Compare LEFT/INNER join results by totalCount (exactly one apart via
  the seeded no-match row) instead of page-capped items.length, so the
  contrast survives fixture growth.
- Remove the now-unused dataFabricTestJoinEntityName config/env plumbing.
- Lock the documented valueList-over-value precedence with a unit case.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
},
});
const wireGroup = (downstream as { filterGroup: { queryFilters: Record<string, unknown>[] } }).filterGroup;
expect(wireGroup.queryFilters[0]).not.toHaveProperty("valueList");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The absence check only covers queryFilters[0]. Two other filters in this test also carried valueListqueryFilters[2] (the "status" filter with both value and valueList set) and the nested group's "tier" filter — and toMatchObject won't catch a leftover valueList on either of those. Per the transform-completeness rule, both the transformed value AND the absence of the original field must be verified for every item that had the field.

Suggested change
expect(wireGroup.queryFilters[0]).not.toHaveProperty("valueList");
const wireGroup = (downstream as {
filterGroup: {
queryFilters: Record<string, unknown>[];
filterGroups: { queryFilters: Record<string, unknown>[] }[];
};
}).filterGroup;
// Verify valueList is stripped from every filter that had it (not just index 0).
expect(wireGroup.queryFilters[0]).not.toHaveProperty("valueList");
expect(wireGroup.queryFilters[2]).not.toHaveProperty("valueList");
expect(wireGroup.filterGroups[0].queryFilters[0]).not.toHaveProperty("valueList");

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Review summary

One new finding this run:

transform-completeness gap in the join unit test (line 1871) — the valueList absence assertion only covers queryFilters[0]. Two other filters also had valueList (queryFilters[2] — "status" with both value and valueList set, and the nested filterGroups[0].queryFilters[0] — "tier"), but toMatchObject allows extra properties so a leftover valueList on those items would go undetected. Suggestion posted inline.

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant