Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/nasty-beers-invite.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@smithy/util-endpoints": patch
---

Reduce temporary object allocations
18 changes: 11 additions & 7 deletions packages/util-endpoints/src/utils/evaluateCondition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,20 @@ import type { ConditionObject, EvaluateOptions } from "../types";
import { EndpointError } from "../types";
import { callFunction } from "./callFunction";

export const evaluateCondition = ({ assign, ...fnArgs }: ConditionObject, options: EvaluateOptions) => {
export const evaluateCondition = (condition: ConditionObject, options: EvaluateOptions) => {
const { assign } = condition;

if (assign && assign in options.referenceRecord) {
throw new EndpointError(`'${assign}' is already defined in Reference Record.`);
}
const value = callFunction(fnArgs, options);
const value = callFunction(condition, options);
Comment thread
trivikr marked this conversation as resolved.

options.logger?.debug?.(`${debugId} evaluateCondition: ${toDebugString(condition)} = ${toDebugString(value)}`);

options.logger?.debug?.(`${debugId} evaluateCondition: ${toDebugString(fnArgs)} = ${toDebugString(value)}`);
const result = value === "" ? true : !!value;

return {
result: value === "" ? true : !!value,
...(assign != null && { toAssign: { name: assign, value } }),
};
if (assign != null) {
return { result, toAssign: { name: assign, value } };
}
return { result };
};
26 changes: 13 additions & 13 deletions packages/util-endpoints/src/utils/evaluateConditions.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,26 +50,26 @@ describe(evaluateConditions.name, () => {
const value1 = "value1";
const value2 = "value2";

vi.mocked(evaluateCondition).mockReturnValueOnce({
result: true,
toAssign: { name: mockCn1.assign!, value: value1 },
});
vi.mocked(evaluateCondition).mockReturnValueOnce({
result: true,
toAssign: { name: mockCn2.assign!, value: value2 },
});
const capturedReferenceRecords: Record<string, unknown>[] = [];

vi.mocked(evaluateCondition)
.mockImplementationOnce((_condition, options) => {
capturedReferenceRecords.push({ ...options.referenceRecord });
return { result: true, toAssign: { name: mockCn1.assign!, value: value1 } };
})
.mockImplementationOnce((_condition, options) => {
capturedReferenceRecords.push({ ...options.referenceRecord });
return { result: true, toAssign: { name: mockCn2.assign!, value: value2 } };
});

const { result, referenceRecord } = evaluateConditions([mockCn1, mockCn2], { ...mockOptions });
expect(result).toBe(true);
expect(referenceRecord).toEqual({
[mockCn1.assign!]: value1,
[mockCn2.assign!]: value2,
});
expect(evaluateCondition).toHaveBeenNthCalledWith(1, mockCn1, mockOptions);
expect(evaluateCondition).toHaveBeenNthCalledWith(2, mockCn2, {
...mockOptions,
referenceRecord: { [mockCn1.assign!]: value1 },
});
expect(capturedReferenceRecords[0]).toEqual({});
expect(capturedReferenceRecords[1]).toEqual({ [mockCn1.assign!]: value1 });
expect(mockLogger.debug).nthCalledWith(1, `${debugId} assign: ${mockCn1.assign} := ${toDebugString(value1)}`);
expect(mockLogger.debug).nthCalledWith(2, `${debugId} assign: ${mockCn2.assign} := ${toDebugString(value2)}`);
});
Expand Down
13 changes: 6 additions & 7 deletions packages/util-endpoints/src/utils/evaluateConditions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,21 @@ import { evaluateCondition } from "./evaluateCondition";

export const evaluateConditions = (conditions: ConditionObject[] = [], options: EvaluateOptions) => {
const conditionsReferenceRecord: Record<string, FunctionReturn> = {};
const conditionOptions: EvaluateOptions = {
...options,
referenceRecord: { ...options.referenceRecord },
};
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.

is it safe to use options all the way through with no copying?

Why I think it might be safe: options object lifecycle is one endpoint resolution, and this is all synchronous code...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll avoid modifying options, as it might create issues in future. It can be tested in a separate PR.


for (const condition of conditions) {
const { result, toAssign } = evaluateCondition(condition, {
...options,
referenceRecord: {
...options.referenceRecord,
...conditionsReferenceRecord,
},
});
Comment thread
trivikr marked this conversation as resolved.
const { result, toAssign } = evaluateCondition(condition, conditionOptions);

if (!result) {
return { result };
}

if (toAssign) {
conditionsReferenceRecord[toAssign.name] = toAssign.value;
conditionOptions.referenceRecord[toAssign.name] = toAssign.value;
options.logger?.debug?.(`${debugId} assign: ${toAssign.name} := ${toDebugString(toAssign.value)}`);
}
}
Expand Down
18 changes: 9 additions & 9 deletions packages/util-endpoints/src/utils/evaluateEndpointRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ export const evaluateEndpointRule = (

options.logger?.debug?.(`${debugId} Resolving endpoint from template: ${toDebugString(endpoint)}`);

return {
...(headers != undefined && {
headers: getEndpointHeaders(headers, endpointRuleOptions),
}),
...(properties != undefined && {
properties: getEndpointProperties(properties, endpointRuleOptions),
}),
url: getEndpointUrl(url, endpointRuleOptions),
};
const endpointToReturn: EndpointV2 = { url: getEndpointUrl(url, endpointRuleOptions) };
if (headers != undefined) {
Comment thread
trivikr marked this conversation as resolved.
Outdated
endpointToReturn.headers = getEndpointHeaders(headers, endpointRuleOptions);
}
if (properties != undefined) {
endpointToReturn.properties = getEndpointProperties(properties, endpointRuleOptions);
}

return endpointToReturn;
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.

unlikely, but does the call order matter for exception handling?

the old code called getEndpointUrl last.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, the previous code would have thrown exceptions from getEndpointHeaders and getEndpointProperties before those from getEndpointUrl.

It should be okay though.

};
10 changes: 6 additions & 4 deletions packages/util-endpoints/src/utils/evaluateRules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,12 @@ export const evaluateTreeRule = (treeRule: TreeRuleObject, options: EvaluateOpti
return;
}

return group.evaluateRules(rules, {
...options,
referenceRecord: { ...options.referenceRecord, ...referenceRecord },
});
const treeRuleOptions =
Object.keys(referenceRecord ?? {}).length > 0
? { ...options, referenceRecord: { ...options.referenceRecord, ...referenceRecord } }
: options;

return group.evaluateRules(rules, treeRuleOptions);
};

export const group = {
Expand Down
12 changes: 6 additions & 6 deletions packages/util-endpoints/src/utils/getEndpointHeaders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ import { evaluateExpression } from "./evaluateExpression";

export const getEndpointHeaders = (headers: EndpointObjectHeaders, options: EvaluateOptions) =>
Object.entries(headers ?? {}).reduce(
(acc, [headerKey, headerVal]) => ({
...acc,
[headerKey]: headerVal.map((headerValEntry) => {
(acc, [headerKey, headerVal]) => {
acc[headerKey] = headerVal.map((headerValEntry) => {
const processedExpr = evaluateExpression(headerValEntry, "Header value entry", options);
if (typeof processedExpr !== "string") {
throw new EndpointError(`Header '${headerKey}' value '${processedExpr}' is not a string`);
}
return processedExpr;
}),
}),
{}
});
return acc;
},
{} as Record<string, string[]>
);
10 changes: 5 additions & 5 deletions packages/util-endpoints/src/utils/getEndpointProperties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import { evaluateTemplate } from "./evaluateTemplate";

export const getEndpointProperties = (properties: EndpointObjectProperties, options: EvaluateOptions) =>
Object.entries(properties).reduce(
(acc, [propertyKey, propertyVal]) => ({
...acc,
[propertyKey]: group.getEndpointProperty(propertyVal, options),
}),
{}
(acc, [propertyKey, propertyVal]) => {
acc[propertyKey] = group.getEndpointProperty(propertyVal, options);
return acc;
},
{} as Record<string, EndpointObjectProperty>
);

export const getEndpointProperty = (
Expand Down
Loading