Skip to content

Commit 3f62cfc

Browse files
fix: rewrite SDK field names in OData filter/orderby/select/expand [PLT-102084] (#536)
* fix: rewrite SDK field names in OData filter/orderby/select/expand Field maps applied SDK-side renames on responses only, so consumers had to remember two names per field: the SDK name for reading the response and the API name for filter/orderby/select/expand. Filtering by the SDK name was silently forwarded to the API and rejected. Add a token-aware rewriter that walks OData expressions, preserves quoted string literals, and rewrites identifier tokens via the reversed `{Entity}Map`. Wire it into `PaginationHelpers.getAll` and into the direct `addPrefixToKeys` sites used by `getById` / `getByName` / `processes.start` / bucket read-write URIs across Jobs, Queues, Assets, Buckets, Processes, Attachments, and Tasks. Refs: PLT-102084 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address convention review comments - Replace `as any` mock-resolved values with typed factory helpers (createMockTasks, createMockBuckets, createMockOrchestratorProcesses) - Remove unused rewriteODataIdentifiers entry from transform mock — only rewriteODataRequestFields is reached through service paths - Type rewriteODataIdentifiers requestMap as FieldMapping for consistency with the rest of transform.ts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: narrow PR scope to Jobs; rename helper to transformOptions Per discussion, scoping this PR to a single service (Jobs) so reviewers can evaluate the approach in isolation. Other affected services (Queues, Assets, Buckets, Processes, Attachments, Tasks) will be picked up in follow-up PRs that reuse the same plumbing introduced here. Also renames the public helper rewriteODataRequestFields → transformOptions to mirror the existing transformRequest naming. transformRequest is for request bodies (object keys), transformOptions is for OData query options (strings with embedded identifiers); same reversed-map mechanism under the hood. Behavior change: none beyond what's already shipping for Jobs. * refactor: simplify rewriteODataIdentifiers with regex alternation Replace the ~30-line char-by-char state machine with a 4-line String.prototype.replace using a regex that alternates between single-quoted literals (with '' escape) and OData identifiers. The regex engine handles both states for free via alternation and greedy identifier matching: '(?:[^']|'')*' atomic quoted literal (identifiers inside don't get a separate match) [A-Za-z_][A-Za-z0-9_]* full identifier token (no substring collisions) All 17 transform.test.ts cases pass unchanged — same invariants (literal-atomicity, full-token matching, empty-map no-op). * refactor: call transformOptions in service layer for symmetry with transformRequest Removes the fieldMap config option from PaginationHelpers / GetAllConfig and moves the transformOptions call into JobService.getAll, mirroring the existing pattern where services call transformRequest directly on request bodies before delegating to the HTTP/pagination layer. Benefits: - PaginationHelpers stays generic — no service-specific concept like "field map" in the shared helper. - One convention across the SDK: services own request-side translation, whether the data is a body (transformRequest) or OData query options (transformOptions). The helpers layer plumbs only. - Unit test asserts the post-translation shape directly (filter contains releaseName, not processName) rather than asserting a wiring detail. No behavior change. * fix(tests): address claude bot review comments - Rename misleading operator-collision test: the assertion proves the rewriter DOES rewrite operator-named map keys, so the test name must not say "does not rewrite operators". - Drop the `null as unknown as string` cast — `transformOptions`'s generic accepts the wider type directly. * fix: drop redundant `as T` cast on transformOptions return transformOptions's generic signature `<T extends Record<string, any>>(options: T, ...): T` already returns T; the explicit cast was redundant. * fix(jobs): address PR review feedback - Consolidate JSDoc filter example: drop the unrelated `state eq 'Running'` case and the verbose "SDK field names" preamble; keep a single example demonstrating filter + orderby with the renamed processName and createdTime. - Simplify getById's option-prep into a single `apiFieldOptions` chain: transformOptions → addPrefixToKeys in two lines, no intermediate `rewrittenOptions` / `keysToPrefix` variables. - Add an integration test that exercises SDK-renamed fields end-to-end: `orderby: 'createdTime desc'` and `select: 'key,processName,state,createdTime'`. The Orchestrator API would 400 on either of those without the rewriter, so this test fails if the rewrite pipeline regresses. * refactor: remove chained renames from transformData The previous implementation iterated the fieldMapping entries and mutated `result` in place — which meant a map with both `a → b` and `b → c` would chain through both renames whenever the input had `a`, producing `c` instead of `b`. ProcessMap's `releaseKey → processKey` + `processKey → packageKey` entries are the canonical example; the chain was a latent quirk waiting to misbehave on a response that contained both keys. The new implementation walks `Object.entries(data)` and looks each key up in the mapping exactly once. Renames are independent — `a → b` does not compose with `b → c` because the result is built fresh and never re-read inside the loop. The original key is dropped (no source + target coexistence in the result). Side effects: - Strictly cheaper: O(D) iterations with no upfront shallow copy and no `delete` calls. The previous code was O(D + M) plus V8 hidden-class transitions on every delete. - Output key order now matches the input's iteration order, which is slightly more predictable. - All 1898 pre-existing unit tests still pass — no test mock contained both ends of a ProcessMap chain, so the chained behaviour was never actually exercised. The removal is observably a no-op for existing inputs. 7 new tests pin the no-chain contract: - basic rename - passthrough of unmapped keys - original key dropped (no source + target coexistence) - no chaining (`a → b → c` map produces `b`, not `c`) - both ends of a chain present in input produce independent renames - array recursion - empty input * feat(processes): expose processKey on ProcessStartResponse The Orchestrator start endpoint returns the originating process's ReleaseKey alongside the job's own Key. With the no-chaining transformData, ProcessMap's `releaseKey → processKey` entry now delivers that as `processKey` on the response without accidentally re-renaming it to `packageKey` (which the old chained transform did). Adding the typed field surfaces it on the public response. No behaviour change at runtime — the field was already being populated by the rename pipeline; it just wasn't declared on the type. * docs(processes): trim processKey JSDoc * fix(transform): handle null/undefined input in transformData The previous chained implementation tolerated null/undefined via the shallow spread (`{ ...null }` is `{}`). The refactored implementation called `Object.entries(data)` which throws `TypeError: Cannot convert undefined or null to object`. The CI integration test `attachments.integration.test.ts` exposed this: `AttachmentService.getById` calls `transformData(camelCased.blobFileAccess, BucketMap)`, and when an OData `select` excludes `blobFileAccess`, the field arrives undefined. The old code returned `{}` silently; the new code threw. Restore null/undefined tolerance with an early return that passes the value through unchanged. Add two unit tests pinning the behaviour. * refactor(transform): tighten transformOptions per review comments - Drop the `any` from the generic constraint. Switch to `T extends object` (matching `transformRequest`'s pattern) and cast the spread destination to `Record<string, unknown>` so we keep one named cast instead of leaking `any` through the signature and the internal result type. - Remove the `!options || !responseMap` runtime guard. Both parameters are non-optional in the type contract, and the two current call sites in jobs.ts already guard `options?` before invoking. Per conventions.md: don't add validation for scenarios that can't happen. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c946ddb commit 3f62cfc

8 files changed

Lines changed: 412 additions & 23 deletions

File tree

src/models/orchestrator/jobs.models.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,9 @@ export interface JobServiceModel {
4242
* const folderJobs = await jobs.getAll({ folderId: <folderId> });
4343
*
4444
* // With filtering
45-
* const runningJobs = await jobs.getAll({
46-
* filter: "state eq 'Running'"
45+
* const recentInvoiceJobs = await jobs.getAll({
46+
* filter: "processName eq 'InvoiceBot'",
47+
* orderby: 'createdTime desc',
4748
* });
4849
*
4950
* // First page with pagination

src/models/orchestrator/processes.types.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,9 +305,11 @@ export interface ProcessStartResponse extends ProcessProperties, FolderPropertie
305305
sourceType: JobSourceType;
306306
batchExecutionKey: string;
307307
info: string | null;
308-
createdTime: string;
308+
createdTime: string;
309309
startingScheduleId: number | null;
310310
processName: string;
311+
/** Key of the process this job was started from. */
312+
processKey: string;
311313
type: JobType;
312314
inputFile: string | null;
313315
outputArguments: string | null;

src/services/orchestrator/jobs/jobs.ts

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { FolderScopedService } from '../../folder-scoped';
22
import { RawJobGetResponse, JobGetAllOptions, JobGetByIdOptions, JobStopOptions, JobResumeOptions } from '../../../models/orchestrator/jobs.types';
33
import { JobServiceModel, JobGetResponse, createJobWithMethods } from '../../../models/orchestrator/jobs.models';
4-
import { addPrefixToKeys, pascalToCamelCaseKeys, transformData } from '../../../utils/transform';
4+
import { addPrefixToKeys, pascalToCamelCaseKeys, transformOptions, transformData } from '../../../utils/transform';
55
import { JOB_ENDPOINTS } from '../../../utils/constants/endpoints';
66
import { ODATA_PAGINATION, ODATA_OFFSET_PARAMS, ODATA_PREFIX } from '../../../utils/constants/common';
77
import { JobMap, JOB_KEY_RESOLUTION_CHUNK_SIZE } from '../../../models/orchestrator/jobs.constants';
@@ -55,8 +55,9 @@ export class JobService extends FolderScopedService implements JobServiceModel {
5555
* const folderJobs = await jobs.getAll({ folderId: <folderId> });
5656
*
5757
* // With filtering
58-
* const runningJobs = await jobs.getAll({
59-
* filter: "state eq 'Running'"
58+
* const recentInvoiceJobs = await jobs.getAll({
59+
* filter: "processName eq 'InvoiceBot'",
60+
* orderby: 'createdTime desc',
6061
* });
6162
*
6263
* // First page with pagination
@@ -87,6 +88,11 @@ export class JobService extends FolderScopedService implements JobServiceModel {
8788
return createJobWithMethods(rawJob, this);
8889
};
8990

91+
// Rewrite renamed SDK field names → API names inside OData strings
92+
// before delegating, mirroring the transformRequest pattern used for
93+
// request bodies.
94+
const apiOptions = options ? transformOptions(options, JobMap) : options;
95+
9096
return PaginationHelpers.getAll({
9197
serviceAccess: this.createPaginationServiceAccess(),
9298
getEndpoint: () => JOB_ENDPOINTS.GET_ALL,
@@ -102,7 +108,7 @@ export class JobService extends FolderScopedService implements JobServiceModel {
102108
countParam: ODATA_OFFSET_PARAMS.COUNT_PARAM,
103109
},
104110
},
105-
}, options) as any;
111+
}, apiOptions) as any;
106112
}
107113

108114
/**
@@ -142,8 +148,8 @@ export class JobService extends FolderScopedService implements JobServiceModel {
142148
}
143149

144150
const headers = createHeaders({ [FOLDER_ID]: folderId });
145-
const keysToPrefix = Object.keys(options ?? {});
146-
const apiOptions = options ? addPrefixToKeys(options, ODATA_PREFIX, keysToPrefix) : {};
151+
const apiFieldOptions = options ? transformOptions(options, JobMap) : {};
152+
const apiOptions = addPrefixToKeys(apiFieldOptions, ODATA_PREFIX, Object.keys(apiFieldOptions));
147153

148154
const response = await this.get<Record<string, unknown>>(
149155
JOB_ENDPOINTS.GET_BY_KEY(id),

src/utils/transform.ts

Lines changed: 109 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,28 @@ export type FieldMapping = {
1616
};
1717

1818
/**
19-
* Transforms data by mapping fields according to the provided field mapping
19+
* Transforms data by renaming each key in `data` exactly once, using the
20+
* mapping (`sourceField → targetField`). Keys not present in the mapping
21+
* pass through unchanged. The original (pre-rename) key is dropped — the
22+
* result contains only the renamed key.
23+
*
24+
* Each rename is independent. If the mapping happens to contain chained
25+
* entries (`a → b` and `b → c`), they do NOT compose: a field named `a`
26+
* in `data` becomes `b` (not `c`), because the renames are applied based
27+
* on the original data's keys, not the running result.
28+
*
2029
* @param data The source data to transform
2130
* @param fieldMapping Object mapping source field names to target field names
2231
* @returns Transformed data with mapped field names
23-
*
32+
*
2433
* @example
2534
* ```typescript
2635
* // Single object transformation
2736
* const data = { id: '123', userName: 'john' };
2837
* const mapping = { id: 'userId', userName: 'name' };
2938
* const result = transformData(data, mapping);
3039
* // result = { userId: '123', name: 'john' }
31-
*
40+
*
3241
* // Array transformation
3342
* const dataArray = [
3443
* { id: '123', userName: 'john' },
@@ -39,29 +48,35 @@ export type FieldMapping = {
3948
* // { userId: '123', name: 'john' },
4049
* // { userId: '456', name: 'jane' }
4150
* // ]
51+
*
52+
* // No chaining — `a → b` does not become `a → c` even if the map has `b → c`.
53+
* transformData({ a: 1 }, { a: 'b', b: 'c' });
54+
* // result = { b: 1 }
4255
* ```
4356
*/
4457
export function transformData<T extends object>(
4558
data: T | T[],
4659
fieldMapping: FieldMapping
4760
): T {
61+
// Pass null/undefined through unchanged — callers (e.g. AttachmentService.getById)
62+
// may invoke this on optional fields that an OData `select` excluded.
63+
if (data == null) {
64+
return data as T;
65+
}
66+
4867
// Handle array of objects
4968
if (Array.isArray(data)) {
5069
return data.map(item => transformData(item, fieldMapping)) as unknown as T;
5170
}
5271

53-
// Handle single object
54-
const result = { ...data };
55-
56-
for (const [sourceField, targetField] of Object.entries(fieldMapping)) {
57-
if (sourceField in result) {
58-
const value = result[sourceField as keyof T];
59-
delete result[sourceField as keyof T];
60-
(result as any)[targetField] = value;
61-
}
72+
// Walk the ORIGINAL data's keys, look up each in the mapping. One rename
73+
// per data key — no mutation of an in-progress result, so chains can't form.
74+
const result: Record<string, unknown> = {};
75+
for (const [key, value] of Object.entries(data)) {
76+
const renamedKey = fieldMapping[key] ?? key;
77+
result[renamedKey] = value;
6278
}
63-
64-
return result;
79+
return result as T;
6580
}
6681

6782
/**
@@ -376,6 +391,86 @@ export function transformRequest<T extends object>(
376391
return result;
377392
}
378393

394+
/**
395+
* OData query-string keys whose values may contain field identifiers that
396+
* need rewriting from SDK names → API names.
397+
*/
398+
const ODATA_FIELD_PARAM_KEYS = ['filter', 'orderby', 'select', 'expand'] as const;
399+
400+
/**
401+
* Matches one token at a time in an OData expression:
402+
* 1. A single-quoted string literal, allowing the `''` escape sequence —
403+
* consumed atomically so identifiers inside the literal can't match.
404+
* 2. An OData identifier (`[A-Za-z_][A-Za-z0-9_]*`).
405+
* Anything else (whitespace, operators, parens, commas) is left alone by
406+
* `String.prototype.replace`, which only substitutes matched substrings.
407+
*/
408+
const ODATA_TOKEN_RE = /'(?:[^']|'')*'|[A-Za-z_][A-Za-z0-9_]*/g;
409+
410+
/**
411+
* Rewrites SDK field identifiers to API field identifiers inside an OData
412+
* expression string (`$filter`, `$orderby`, `$select`, `$expand`).
413+
*
414+
* Field maps (e.g. `JobMap`) rename API fields → SDK fields on responses, so
415+
* SDK consumers see the renamed names. Without this rewrite, the same name
416+
* in a `filter` string would be forwarded verbatim and the API (which still
417+
* uses the original name) would reject it.
418+
*
419+
* Quoted string literals (with the OData `''` escape) are preserved exactly:
420+
* the token regex consumes them whole, so identifiers inside literals never
421+
* match. Identifier tokens are looked up in the reversed field map.
422+
*
423+
* @example
424+
* ```typescript
425+
* const requestMap = { processName: 'releaseName' };
426+
* rewriteODataIdentifiers("processName eq 'processName'", requestMap);
427+
* // "releaseName eq 'processName'" — identifier rewritten, literal preserved
428+
* ```
429+
*/
430+
export function rewriteODataIdentifiers(expression: string, requestMap: FieldMapping): string {
431+
if (!expression) return expression;
432+
return expression.replace(ODATA_TOKEN_RE, (match) =>
433+
match.startsWith("'") ? match : (requestMap[match] ?? match),
434+
);
435+
}
436+
437+
/**
438+
* Symmetric counterpart of {@link transformRequest} for OData query options:
439+
* rewrites SDK field identifiers inside the recognized OData string params
440+
* (`filter`, `orderby`, `select`, `expand`) to their API names using the
441+
* reversed form of a response field map. Returns a shallow copy with the
442+
* relevant values rewritten; other keys pass through unchanged.
443+
*
444+
* Use at the OData edge so SDK consumers can refer to renamed fields by
445+
* their SDK name throughout — for reading the response and for filtering /
446+
* sorting / projecting / expanding.
447+
*
448+
* @param options The OData query options as authored with SDK field names
449+
* @param responseMap The response field map (API → SDK); reversed internally
450+
*
451+
* @example
452+
* ```typescript
453+
* // JobMap renames releaseName → processName on responses.
454+
* transformOptions({ filter: "processName eq 'X'" }, JobMap);
455+
* // { filter: "releaseName eq 'X'" }
456+
* ```
457+
*/
458+
export function transformOptions<T extends object>(
459+
options: T,
460+
responseMap: FieldMapping,
461+
): T {
462+
const requestMap = reverseMap(responseMap);
463+
if (Object.keys(requestMap).length === 0) return options;
464+
const result: Record<string, unknown> = { ...(options as Record<string, unknown>) };
465+
for (const key of ODATA_FIELD_PARAM_KEYS) {
466+
const value = result[key];
467+
if (typeof value === 'string') {
468+
result[key] = rewriteODataIdentifiers(value, requestMap);
469+
}
470+
}
471+
return result as T;
472+
}
473+
379474
/**
380475
* Renames fields in an object in-place
381476
* @param obj The object to modify

tests/integration/shared/orchestrator/jobs.integration.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,27 @@ describe.each(modes)('Orchestrator Jobs - Integration Tests [%s]', (mode) => {
6161
expect(result.items).toBeDefined();
6262
expect(Array.isArray(result.items)).toBe(true);
6363
});
64+
65+
it('should retrieve jobs filtered and sorted by SDK field names', async () => {
66+
const { jobs, folderId } = getJobsService();
67+
68+
// Uses SDK names that the field map renames:
69+
// processName → releaseName, createdTime → creationTime.
70+
// If the rewriter doesn't translate these, the API returns 400.
71+
const result = await jobs.getAll({
72+
folderId,
73+
pageSize: 5,
74+
orderby: 'createdTime desc',
75+
select: 'key,processName,state,createdTime',
76+
});
77+
78+
expect(result).toBeDefined();
79+
expect(Array.isArray(result.items)).toBe(true);
80+
// Response is also in SDK shape — `createdTime` is the renamed key.
81+
if (result.items.length > 0) {
82+
expect(result.items[0]).toHaveProperty('createdTime');
83+
}
84+
});
6485
});
6586

6687
describe('getById', () => {

tests/unit/services/orchestrator/jobs.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,27 @@ describe('JobService Unit Tests', () => {
158158
await expect(jobService.getAll()).rejects.toThrow(TEST_CONSTANTS.ERROR_MESSAGE);
159159
});
160160

161+
it('should translate SDK field names to API names in filter/orderby before delegating', async () => {
162+
vi.mocked(PaginationHelpers.getAll).mockResolvedValue(
163+
createMockTransformedJobCollection(),
164+
);
165+
166+
await jobService.getAll({
167+
filter: "processName eq 'InvoiceBot' and folderId eq 7",
168+
orderby: 'createdTime desc',
169+
});
170+
171+
// Options arriving at PaginationHelpers should already be in API field space:
172+
// processName → releaseName, folderId → organizationUnitId, createdTime → creationTime.
173+
expect(PaginationHelpers.getAll).toHaveBeenCalledWith(
174+
expect.any(Object),
175+
expect.objectContaining({
176+
filter: "releaseName eq 'InvoiceBot' and organizationUnitId eq 7",
177+
orderby: 'creationTime desc',
178+
}),
179+
);
180+
});
181+
161182
it('should attach getOutput method to transformed job objects', async () => {
162183
const mockResponse = createMockTransformedJobCollection();
163184
vi.mocked(PaginationHelpers.getAll).mockResolvedValue(mockResponse);
@@ -231,6 +252,25 @@ describe('JobService Unit Tests', () => {
231252
expect(typeof result.stop).toBe('function');
232253
});
233254

255+
it('should rewrite SDK field names in select back to API names', async () => {
256+
const mockRawJob = createMockRawJob();
257+
mockApiClient.get.mockResolvedValueOnce(mockRawJob);
258+
259+
await jobService.getById(JOB_TEST_CONSTANTS.JOB_KEY, TEST_CONSTANTS.FOLDER_ID, {
260+
select: 'processName,createdTime,folderId',
261+
});
262+
263+
// processName → releaseName, createdTime → creationTime, folderId → organizationUnitId
264+
expect(mockApiClient.get).toHaveBeenCalledWith(
265+
JOB_ENDPOINTS.GET_BY_KEY(JOB_TEST_CONSTANTS.JOB_KEY),
266+
expect.objectContaining({
267+
params: expect.objectContaining({
268+
$select: 'releaseName,creationTime,organizationUnitId',
269+
}),
270+
}),
271+
);
272+
});
273+
234274
it('should pass expand and select options with OData prefix', async () => {
235275
const mockRawJob = createMockRawJob();
236276
mockApiClient.get.mockResolvedValueOnce(mockRawJob);

0 commit comments

Comments
 (0)