fix(data-fabric): multi-join queries were silently no-ops — route to the multi-entity endpoint with the correct wire contract#591
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // and translate each join to the wire shape ({ type, entity, on: { left, right } }). | ||
| let getEndpoint = () => DATA_FABRIC_ENDPOINTS.ENTITY.QUERY_BY_ID(id); | ||
| if (options?.joins && options.joins.length > 0) { | ||
| const { name: baseEntityName } = await this.getById(id, folderKey !== undefined ? { folderKey } : undefined); |
There was a problem hiding this comment.
Double-telemetry violation per conventions.md — the "Delegation anti-pattern":
queryRecordsById is decorated with @track('Entities.QueryRecordsById'). Calling this.getById() here fires its own @track('Entities.GetById') decorator, so every join query emits two telemetry events — one for the query and a spurious one for the internal entity-name lookup.
"Delegation anti-pattern: when a public service method delegates to another service's public
@track-decorated method, both@trackdecorators fire, causing double telemetry. Fix: extract the shared logic into an internal helper function (no@track) and have both public service methods call the helper directly."
The fix is a private helper that makes the HTTP call directly without the decorator:
/** Resolves the entity name for a given entity ID (no telemetry — internal use only). */
private async resolveEntityName(id: string, folderKey?: string): Promise<string> {
const response = await this.get<RawEntityGetResponse>(
DATA_FABRIC_ENDPOINTS.ENTITY.GET_BY_ID(id),
{ headers: createHeaders({ [FOLDER_KEY]: folderKey }) },
);
const metadata = transformData(response.data as RawEntityGetResponse, EntityMap);
return metadata.name as string;
}Then in queryRecordsById:
const baseEntityName = await this.resolveEntityName(id, folderKey);And getById continues to call its own (full) internal pipeline unchanged.
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
…ct for joins The joins support added in #543 posted the EntityJoin shape to the ID-based query route, which does not implement the multi-entity contract - the server silently drops the unknown joins body key and returns base-entity rows, so every join query 200s while performing no join (verified live: a join to a nonexistent entity returned 200). The multi-entity contract lives on the name-based route (POST /api/EntityService/{EntityName}/query) and expects { type: LEFT|INNER, entity, on: { left, right } } with entity-qualified field references. - Keep the public EntityJoin shape unchanged; translate it to the wire contract internally and route join queries to the name-based endpoint (entity name resolved via getById, folderKey passthrough). - Add JoinType.InnerJoin (contract supports INNER and LEFT). - Validate client-side that join queries carry selectedFields or aggregates (the API rejects joins without a projection). - Document that folder-scoped entities are not supported by the multi-entity query API (verified live: ID-based reads work with the folder header, the name-based route 403s - the joins authz path is not folder-aware). - Unit tests assert the translated wire body and endpoint routing, plus the no-joins regression path (ID endpoint, no metadata lookup). - Integration tests assert the join EFFECT (qualified related-entity keys in rows, INNER within LEFT) and a negative control (join to a nonexistent entity must reject) - envelope-only assertions cannot fail when the backend ignores the joins clause, which is how the original wiring shipped. Verified live against a Data Fabric tenant: chained 2-join LEFT query returns related-entity fields, INNER join works, pagination (body start/limit) works, and no-joins queries are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d67d5e6 to
f3aa69b
Compare
| vi.mocked(PaginationHelpers.getAll).mockResolvedValue({ items: [], totalCount: 0 }); | ||
| const getByIdSpy = vi | ||
| .spyOn(entityService, "getById") | ||
| .mockResolvedValue({ name: "Order" } as any); |
There was a problem hiding this comment.
as any appears 7 times across the new join tests (lines 1717, 1755, 1780, 1804, 1831, 1867, 1928), violating the no-any rule from conventions.md:
"No
anytype — useunknownif truly unknown, then validate."
The root cause is that getById returns EntityGetResponse (= RawEntityGetResponse & EntityMethods), and the tests only need { name } from that shape, making a full typed mock awkward.
The cleanest fix that stays within the conventions is a scoped helper at the top of the queryRecordsById describe block:
const mockGetByIdName = (name: string) =>
vi.spyOn(entityService, "getById").mockImplementation(
() => Promise.resolve(createEntityWithMethods({ name } as RawEntityGetResponse, entityService))
);Alternatively, since the only consumed field is name, define a local interface and use a cast that's honest about what it omits:
// Cast is intentional: queryRecordsById only reads `.name` from getById's result.
vi.spyOn(entityService, "getById").mockResolvedValue(
{ name: "Order" } as Pick<EntityGetResponse, "name"> as unknown as EntityGetResponse
);Either way, 7 identical inline casts should be collapsed to one shared setup rather than repeated in every test.
Review summaryNew finding posted this run:
Still-open thread from a previous run (not re-raised): |
…ed helper
Collapses seven inline '{ name } as any' casts into a single mockGetByIdName
helper with a direct EntityGetResponse assertion, per the no-any convention
(review feedback).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
| * several composes a multi-entity (multi-join) query. Up to 3 joins are | ||
| * supported (the SDK throws a `ValidationError` for more), and all of them | ||
| * must share the same {@link JoinType}. | ||
| * must share the same {@link JoinType} (the API rejects mixed join types). |
There was a problem hiding this comment.
no need to add API behaviour
There was a problem hiding this comment.
Dropped in 33ae90f — the parenthetical is gone; "must share the same JoinType" carries it.
| * must share the same {@link JoinType}. | ||
| * must share the same {@link JoinType} (the API rejects mixed join types). | ||
| * | ||
| * Join queries require `selectedFields` or `aggregates`. Multi-entity result |
There was a problem hiding this comment.
what are selectedFields and aggregates? are they fields within a type? probably we can simplify this comment for ease of understanding
| * | ||
| * // Multi-join: pull fields from related entities into the query | ||
| * // Multi-join: pull fields from related entities into the query. | ||
| * // Reference fields as "EntityName.Field" wherever the name is ambiguous |
There was a problem hiding this comment.
do we need this info at method level? we already have the info at "EntityJoin" type JSDoc?
There was a problem hiding this comment.
Agreed — trimmed in 33ae90f. The method-level example comment is now one line pointing at EntityJoin (in all three JSDoc copies: queryRecordsById in the service + models, and queryRecords), so the type doc is the single source for join semantics.
- Drop the mixed-join-types parenthetical (the JoinType sentence covers it). - Point selectedFields/aggregates at EntityQueryRecordsOptions so it is clear they are query options, and compress the folder-scope note. - De-duplicate method-level example commentary; EntityJoin is the single source for join semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| * }); | ||
| * | ||
| * // Multi-join: pull fields from related entities into the query | ||
| * // (result rows use entity-qualified keys — see EntityJoin). |
There was a problem hiding this comment.
have we defined qualified/non-qualified anywhere? We should make it explicit if not already (since its not obvious)
There was a problem hiding this comment.
Good catch — the terms were shown by example but never defined. Done in f127aa88: the EntityJoin JSDoc (the place both call-site comments link to) now spells it out — qualified = entity-name-dot-prefixed ("Customer.Name"), unqualified = bare field name ("Name"), with the resolution rule (unqualified only works while the name is unique across all joined entities).
Actions runs were never enqueued for the previous push. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n EntityJoin JSDoc Address PR review: the terms were used but never defined — spell out that qualified = entity-name-dot-prefixed, unqualified = bare name, and when each resolves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolve conflict in src/services/data-fabric/entities.ts: main's #594 removed class-level JSDoc (now inherited from EntityServiceModel); the interface already carries this PR's multi-join docs, so the duplicated class block is dropped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
|
2 participants
Summary
Fixes the multi-join support shipped in #543 (v1.5.2), which never performed a join: the SDK posted
joinsto the ID-based query route, where the backend does not implement the multi-entity contract and silently drops the unknown body key — every join query returned 200 with plausible base-entity rows while performing no join.Live evidence (prod + alpha, identical behavior):
entity/{id}/querywith a join to a nonexistent entity → 200, base rows (thejoinskey is never parsed)/{EntityName}/querywith the SDK's shape →400 'joins[0].type' is required.The actual contract is the Multi-Entity Query API:
POST /api/EntityService/{EntityName}/querywith{ type: "LEFT"|"INNER", entity, on: { left, right } }and entity-qualified field references; result rows come back with entity-qualified keys.What changed
EntityJoinkeeps its released shape; existing user code starts working. The SDK translates it to the wire contract internally (joinType→type,relatedEntityName→entity, join keys → qualifiedon.left/on.right, defaulting the base entity from the queried entity's name).joinsis present,queryRecordsByIdresolves the entity name (onegetByIdmetadata call,folderKeypassthrough) and posts to the name-based route. No-joins queries are byte-for-byte unchanged (ID route, no metadata lookup).JoinType.InnerJoinadded (contract supports INNER and LEFT; mixed types rejected by the API).Fields or Aggregates required), so the SDK now throws an actionableValidationErrorwhenjoinsis supplied withoutselectedFields/aggregates.start/limitpath (the name-based route ignores URL pagination params; verified cursor paging across pages live).Why the tests didn't catch it in #543 — and what changed
The join integration test asserted the response envelope (no 4xx, items array,
Idpresent, valid pagination) — all of which a backend that ignoresjoinssatisfies. The tests now assert the join effect:"sdkJoinRelated.*") — fails when the backend ignores the clausejoinsBehavior notes
"Customer.Name") — this is the multi-entity API's response format (previously joins returned unqualified base rows only, i.e. the feature didn't work, so no working code depended on the old shape)null)EntityJoin)User lacks READ permission— the joins authz path is not folder-aware server-side). Documented onEntityJoin; the server's 403 is surfaced as-is.Test plan
npm run typecheck— cleannpm run lint— 0 warnings, 0 errorsnpx vitest run tests/unit— 2065 tests pass (8 new/updated join tests)npm run build— succeedsEntity NoSuchXYZ does not exist), missing-projection throws client-side, paginated join (pageSize 1 + cursor to page 2), no-joins filter query unchanged,getRecordByIdunchangedFollow-up
The Python SDK (UiPath/uipath-python#1616) has the same defect — same ID-based route, same non-contract shape — and needs the equivalent fix.
🤖 Generated with Claude Code