Skip to content

feat(data-fabric): add query method with joins, aggregates, and filters#334

Open
keerthikiranvalathoru wants to merge 5 commits intomainfrom
feat/data-fabric-query-with-joins
Open

feat(data-fabric): add query method with joins, aggregates, and filters#334
keerthikiranvalathoru wants to merge 5 commits intomainfrom
feat/data-fabric-query-with-joins

Conversation

@keerthikiranvalathoru
Copy link
Copy Markdown

Add query() method to EntityService supporting cross-entity joins, aggregates, filters, grouping, and sorting via POST /api/EntityService/{EntityName}/query.

Changes:

  • Add QUERY endpoint to DATA_FABRIC_ENDPOINTS
  • Add query types: QueryAggregate, QueryJoin, QueryFilter, QueryFilterGroup, QuerySortOption, EntityQueryOptions, EntityQueryResponse
  • Add query enums: QueryAggregateFunction, QueryJoinType, QueryLogicalOperator
  • Add query() to EntityServiceModel interface and EntityService implementation
  • Add bound query() to EntityMethods and createEntityMethods factory
  • Add unit tests for service and model layers

@keerthikiranvalathoru keerthikiranvalathoru marked this pull request as ready for review March 28, 2026 14:12
@keerthikiranvalathoru keerthikiranvalathoru requested a review from a team March 28, 2026 14:12
keerthikiranvalathoru and others added 5 commits March 30, 2026 11:56
Add `query()` method to EntityService supporting cross-entity joins,
aggregates, filters, grouping, and sorting via POST /api/EntityService/{EntityName}/query.

Changes:
- Add QUERY endpoint to DATA_FABRIC_ENDPOINTS
- Add query types: QueryAggregate, QueryJoin, QueryFilter, QueryFilterGroup, QuerySortOption, EntityQueryOptions, EntityQueryResponse
- Add query enums: QueryAggregateFunction, QueryJoinType, QueryLogicalOperator
- Add query() to EntityServiceModel interface and EntityService implementation
- Add bound query() to EntityMethods and createEntityMethods factory
- Add unit tests for service and model layers

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add @experimental JSDoc tag and preview warning to all new query types,
enums, and methods following the pattern from PR #328.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The query endpoint uses /api/EntityService/entity/{entityId}/query
matching the pattern of all other Data Fabric endpoints, not
/api/EntityService/{EntityName}/query.

Renames entityName -> entityId across endpoint, service, models, and tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verified query API against live Data Fabric at cloud.uipath.com.
The API returns { totalRecordCount, value }, not { totalCount, records }.

Also improved JSDoc: selectedFields is required when using aggregates
and must include groupBy fields.

Tested against keerthiuipclitest org:
- Filtered query on Customers (1000 records): OK
- Aggregates with groupBy on ClaimHeaders (5000 records): OK
- INNER join between ClaimHeaders and Policies: OK

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ting

String operators must be lowercase: "contains", "startswith", "endswith".
Verified via 44 live SDK tests against cloud.uipath.com.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@@ -590,6 +590,40 @@ describe('Entity Models', () => {
).rejects.toThrow(ENTITY_TEST_CONSTANTS.ERROR_MESSAGE_ENTITY_ID_UNDEFINED);
});
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.

Can you add integration test

@Sarath1018
Copy link
Copy Markdown
Contributor

Please update docs/oauth-scopes.md with query() method

@Sarath1018 Sarath1018 force-pushed the feat/data-fabric-query-with-joins branch from ee8f133 to b55f4e1 Compare March 30, 2026 06:38
@sonarqubecloud
Copy link
Copy Markdown

@Sarath1018
Copy link
Copy Markdown
Contributor

@claude review this PR

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 30, 2026

Claude finished @Sarath1018's task in 1m 58s —— View job


PR Review

  • Read changed files
  • Review types and enums
  • Review service implementation
  • Review tests
  • Check documentation/conventions compliance
  • Post final review

Overall this is a solid addition — the service implementation, method binding, and endpoint constant all follow existing patterns correctly. There are a few issues to address before merging.

Bugs / Correctness

1. Test mock data doesn't match EntityQueryResponse interface (tests/unit/services/data-fabric/entities.test.ts:1028-1036)

The mock API response uses Records / TotalCount (PascalCase):

data: {
  Records: [...],
  TotalCount: 2
}

After pascalToCamelCaseKeys this becomes { records, totalCount }. But EntityQueryResponse defines { value, totalRecordCount }. The test mock data doesn't reflect what the real API returns, so pascalToCamelCaseKeys never produces the right shape in tests. The mock should use Value / TotalRecordCount to match what the live API returns (based on EntityQueryResponse having value and totalRecordCount). Fix this →

2. No transform validation in tests (per conventions)

For methods with a transform pipeline, tests must verify both:

  • (a) transformed camelCase fields exist: result.value, result.totalRecordCount
  • (b) original PascalCase fields are absent: (result as any).Value === undefined, (result as any).TotalRecordCount === undefined

None of the query service tests assert on the actual response field names.

Missing Requirements

3. docs/oauth-scopes.md not updated (as noted by @Sarath1018)

query() is missing from the Entities table. Per rules.md: NEVER skip docs/oauth-scopes.md when adding a method. The scope for a query/read operation would be DataFabric.Data.Read. Fix this →

4. No integration test (as noted by @Sarath1018)

query() has no entry in tests/integration/shared/data-fabric/entities.integration.test.ts. Per conventions, every new method needs an integration test.

Type Safety

5. QueryFilter.operator typed as string (src/models/data-fabric/entities.types.ts:312)

The operators are a fixed, known set: "=", "!=", ">", "<", ">=", "<=", "contains", "startswith", "endswith". Using string loses type safety and autocomplete. This should be a QueryFilterOperator enum or a string union type:

export type QueryFilterOperator = '=' | '!=' | '>' | '<' | '>=' | '<=' | 'contains' | 'startswith' | 'endswith';

Fix this →

6. QueryLogicalOperator uses numeric values (src/models/data-fabric/entities.types.ts:239-242)

export enum QueryLogicalOperator {
  AND = 0,
  OR = 1,
}

All other enums in this file (QueryAggregateFunction, QueryJoinType, EntityFieldDataType, etc.) use string values. Numeric enums are less readable and the JSDoc comment in QueryFilterGroup leaks the magic numbers: "Logical operator to combine filters (0 = AND, 1 = OR)". If the API genuinely requires integer 0/1, that's acceptable — but the JSDoc should be updated to reference the enum: Use {@link QueryLogicalOperator}, and the examples should use QueryLogicalOperator.AND not 0.

JSDoc Issues

7. Examples use raw string literals instead of enums (src/models/data-fabric/entities.models.ts:499-516)

// ❌ Current
joins: [{ type: 'INNER', entity: 'Customer', on: { left: 'customerId', right: 'Id' } }]
aggregates: [{ function: 'COUNT', field: 'Id', alias: 'totalOrders' }]

// ✅ Should be
joins: [{ type: QueryJoinType.INNER, entity: 'Customer', on: { left: 'customerId', right: 'Id' } }]
aggregates: [{ function: QueryAggregateFunction.COUNT, field: 'Id', alias: 'totalOrders' }]

Same issue in src/services/data-fabric/entities.ts:614-626. Examples must show correct SDK usage with enums.

8. logicalOperator: 0 in JSDoc (src/models/data-fabric/entities.models.ts:491, entities.ts:603)

Should be logicalOperator: QueryLogicalOperator.AND.

9. Unusual /// warning JSDoc format (src/models/data-fabric/entities.types.ts:197-202, repeated ~10×)

/**
 * @experimental
 *
 * /// warning
  Preview: This type is experimental...
  ///

This looks like Rust doc comment syntax and won't render correctly in TypeDoc. Use standard JSDoc:

/**
 * @experimental
 * @remarks **Preview**: This type is experimental and may change or be removed in future releases.

Minor

10. EntityQueryOptions mixes SDK pagination style with inline start/limit — since this is a POST body (not query params), inline pagination is fine. But consider documenting why this diverges from PaginationOptions to avoid future confusion.


Summary: The core implementation is correct. The main blockers are:

  1. Test mock data mismatch (Onboard action center #1) and missing transform validation (Add getFormById for Form Actions #2)
  2. Missing docs/oauth-scopes.md entry (Add getUsers method for Action service #3)
  3. Missing integration test (Rename 'action' to 'task' #4)
  4. QueryFilter.operator should be typed (export Task models #5)

Items #6#9 are documentation quality issues that should also be fixed before merging.

Comment on lines +487 to +516
* // Simple query with filters
* const result = await entities.query('<entityId>', {
* selectedFields: ['name', 'email'],
* filterGroup: {
* logicalOperator: 0,
* queryFilters: [{ fieldName: 'status', operator: '=', value: 'active' }]
* },
* limit: 50
* });
*
* // Cross-entity join query
* const joinResult = await entities.query('<entityId>', {
* selectedFields: ['Orders.orderId', 'Customer.name'],
* joins: [{
* type: 'INNER',
* entity: 'Customer',
* on: { left: 'customerId', right: 'Id' }
* }],
* sortOptions: [{ fieldName: 'Orders.createdDate', isDescending: true }],
* limit: 100
* });
*
* // Aggregate query
* const stats = await entities.query('<entityId>', {
* aggregates: [
* { function: 'COUNT', field: 'Id', alias: 'totalOrders' },
* { function: 'SUM', field: 'amount', alias: 'totalAmount' }
* ],
* groupBy: ['status']
* });
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since we have defined enums for these values, why not use them in the examples as well?
line 491 logicalOperator: 0, -> QueryLogicalOperator.AND
line 501 type: 'INNER', -> QueryJoinType.INNER
and the aggregate function on line 512 and 513

Comment on lines +312 to +313
/** Comparison operator: "=", "!=", ">", "<", ">=", "<=", "contains", "startswith", "endswith" (lowercase for string operators) */
operator: string;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we have this as an enum?

/** Field to aggregate on */
field: string;
/** Alias for the aggregated result */
alias: string;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

question : Is alias always required by the API?

Comment on lines +377 to +381
/** Starting offset for pagination (default: 0) */
start?: number;
/** Maximum number of records to return (default: 100) */
limit?: number;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The SDK already supports pagination. Can't we use that instead?

Comment on lines +649 to +650
const camelResponse = pascalToCamelCaseKeys(response.data);
return camelResponse;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Sarath1018 we had plans of removing this conversion from records. Please check its status, we'll need to update here accordingly.

Comment on lines +633 to +647
const body: Record<string, unknown> = {};

if (options.selectedFields) body.selectedFields = options.selectedFields;
if (options.aggregates) body.aggregates = options.aggregates;
if (options.joins) body.joins = options.joins;
if (options.filterGroup) body.filterGroup = options.filterGroup;
if (options.groupBy) body.groupBy = options.groupBy;
if (options.sortOptions) body.sortOptions = options.sortOptions;
if (options.start !== undefined) body.start = options.start;
if (options.limit !== undefined) body.limit = options.limit;

const response = await this.post<EntityQueryResponse>(
DATA_FABRIC_ENDPOINTS.ENTITY.QUERY(entityId),
body
);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this necessary? Can we not pass the options object directly?

Comment on lines +510 to +515
* const stats = await entities.query('<entityId>', {
* aggregates: [
* { function: 'COUNT', field: 'Id', alias: 'totalOrders' },
* { function: 'SUM', field: 'amount', alias: 'totalAmount' }
* ],
* groupBy: ['status']
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This example uses aggregates without selectedFields. Wouldn't this error out?

@swati354 swati354 mentioned this pull request Apr 8, 2026
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.

3 participants