Optimize Duplications of ComputedField queries

For each modelName the a `ComputedField` is only compiled into abstract-sql-query once. Afterwards it's used as ReferencedField Change-type: minor Signed-off-by: fisehara <[email protected]>
balena-io-modules · Nov 17, 2023 · 6f3293d · 6f3293d
1 parent 46a7ede
commit 6f3293d
Show file tree

Hide file tree

Showing 5 changed files with 236 additions and 10 deletions.
diff --git a/src/odata-to-abstract-sql.ts b/src/odata-to-abstract-sql.ts
@@ -117,6 +117,8 @@ type Overwrite<T, U> = Pick<T, Exclude<keyof T, keyof U>> & U;
 type RequiredField<T, F extends keyof T> = Overwrite<T, Required<Pick<T, F>>>;
 type AliasedResource = RequiredField<Resource, 'tableAlias'>;
 
+type AlreadyComputedFieldsLookup = { [resourceAndFieldName: string]: boolean };
+
 export type ResourceFunction = (
 	this: OData2AbstractSQL,
 	property: {
@@ -298,10 +300,17 @@ export const isBindReference = (maybeBind: {
 	);
 };
 
-const isDynamicResource = (resource: Resource): boolean => {
+const isDynamicResource = (
+	resource: Resource,
+	alreadyComputedFieldsLookup: AlreadyComputedFieldsLookup,
+): boolean => {
 	return (
 		resource.definition != null ||
-		resource.fields.some((f) => f.computed != null)
+		resource.fields.some(
+			(f) =>
+				f.computed != null &&
+				!alreadyComputedFieldsLookup[resource.name + '$' + f.fieldName],
+		)
 	);
 };
 
@@ -327,6 +336,7 @@ export class OData2AbstractSQL {
 	public defaultResource: Resource | undefined;
 	public bindVarsLength: number = 0;
 	private checkAlias: (alias: string) => string;
+	private alreadyComputedFields: AlreadyComputedFieldsLookup = {};
 
 	constructor(
 		private clientModel: RequiredAbstractSqlModelSubset,
@@ -544,7 +554,9 @@ export class OData2AbstractSQL {
 			// For updates/deletes that we use a `WHERE id IN (SELECT...)` subquery to apply options and in the case of a definition
 			// we make sure to always apply it. This means that the definition will still be applied for these queries
 			if (
-				(hasQueryOpts || isDynamicResource(resource) || pathKeyWhere != null) &&
+				(hasQueryOpts ||
+					isDynamicResource(resource, this.alreadyComputedFields) ||
+					pathKeyWhere != null) &&
 				(method === 'POST' || method === 'PUT-INSERT')
 			) {
 				// For insert statements we need to use an INSERT INTO ... SELECT * FROM (binds) WHERE ... style query
@@ -563,8 +575,9 @@ export class OData2AbstractSQL {
 						'SelectQuery',
 						[
 							'Select',
-							(resource.modifyFields ?? resource.fields).map(
-								(field): AliasNode<CastNode> => {
+							(resource.modifyFields ?? resource.fields)
+								.filter((field) => field.computed == null)
+								.map((field): AliasNode<CastNode> => {
 									const alias = field.fieldName;
 									const bindVar = bindVars?.find((v) => v[0] === alias);
 									const value = bindVar?.[1] ?? ['Null'];
@@ -574,8 +587,7 @@ export class OData2AbstractSQL {
 										);
 									}
 									return ['Alias', ['Cast', value, field.dataType], alias];
-								},
-							),
+								}),
 						],
 					],
 					'$insert',
@@ -688,7 +700,8 @@ export class OData2AbstractSQL {
 		// we make sure to always apply it. This means that the definition will still be applied for these queries, for insert queries
 		// this is handled when we set the 'Values'
 		if (
-			(hasQueryOpts || isDynamicResource(resource)) &&
+			(hasQueryOpts ||
+				isDynamicResource(resource, this.alreadyComputedFields)) &&
 			(method === 'PUT' ||
 				method === 'PATCH' ||
 				method === 'MERGE' ||
@@ -992,11 +1005,18 @@ export class OData2AbstractSQL {
 		fieldName: string,
 		computed?: AbstractSqlQuery,
 		alias: string = fieldName,
+		// Setting this flag to true will ignore the lookup for alreadyComputedFields
+		// and will compile the computed Field statement into the abstract SQL statement regardless
+		forceCompilingComputedField: boolean = false,
 	):
 		| ReferencedFieldNode
 		| AliasNode<ReferencedFieldNode>
 		| AliasNode<AbstractSqlQuery> {
-		if (computed) {
+		const key = resource.name + '$' + fieldName;
+		if (
+			computed &&
+			(!this.alreadyComputedFields[key] || forceCompilingComputedField)
+		) {
 			if (
 				resource.tableAlias != null &&
 				resource.tableAlias !== resource.name
@@ -1007,6 +1027,7 @@ export class OData2AbstractSQL {
 					resource.tableAlias,
 				);
 			}
+			this.alreadyComputedFields[key] = true;
 			return ['Alias', computed, alias];
 		}
 		const referencedField = this.ReferencedField(resource, fieldName);
@@ -1639,6 +1660,7 @@ export class OData2AbstractSQL {
 		this.putReset();
 		this.extraBodyVars = {};
 		this.extraBindVars = [] as unknown as ODataBinds;
+		this.alreadyComputedFields = {};
 	}
 
 	putReset() {
@@ -1702,6 +1724,7 @@ export class OData2AbstractSQL {
 							sqlNameToODataName(field.fieldName),
 							field.computed,
 							field.fieldName,
+							true,
 						),
 					),
 				];

diff --git a/test/chai-sql.js b/test/chai-sql.js
@@ -108,6 +108,20 @@ const generateClientModel = function (input) {
 const sbvrModel = fs.readFileSync(require.resolve('./model.sbvr'), 'utf8');
 export const clientModel = generateClientModel(sbvrModel);
 
+clientModel.tables['copilot'].fields.push({
+	fieldName: 'is blocked',
+	dataType: 'Boolean',
+	// The cast is needed because AbstractSqlQuery cannot express a constant value.
+	computed: ['Boolean', false],
+});
+
+clientModel.tables['copilot'].fields.push({
+	fieldName: 'rank',
+	dataType: 'Text',
+	// The cast is needed because AbstractSqlQuery cannot express a constant value.
+	computed: ['Text', 'Junior'],
+});
+
 const odataNameToSqlName = (odataName) =>
 	odataName.replace(/__/g, '-').replace(/_/g, ' ');
 
@@ -363,3 +377,14 @@ export const teamFields = [
 ];
 
 export const $count = [['Alias', ['Count', '*'], '$count']];
+export const $count = [['Alias', ['Count', '*'], '$count']];
+
+export const copilotFields = [
+	['Alias', ['ReferencedField', 'copilot', 'created at'], 'created_at'],
+	['Alias', ['ReferencedField', 'copilot', 'modified at'], 'modified_at'],
+	['ReferencedField', 'copilot', 'id'],
+	['ReferencedField', 'copilot', 'person'],
+	['ReferencedField', 'copilot', 'assists-pilot'],
+	['Alias', ['ReferencedField', 'copilot', 'is blocked'], 'is_blocked'],
+	['ReferencedField', 'copilot', 'rank'],
+];
diff --git a/test/filterby.js b/test/filterby.js
@@ -1447,3 +1447,97 @@ run(function () {
 		}),
 	);
 });
+
+test(`/copilot?$select=id,rank&$filter=rank eq 'major'`, (result) =>
+	it(`should get and filter copilot on computed field rank`, () => {
+		expect(result).to.be.a.query.to.deep.equal([
+			'SelectQuery',
+			[
+				'Select',
+				[
+					['ReferencedField', 'copilot', 'id'],
+					['ReferencedField', 'copilot', 'rank'],
+				],
+			],
+			[
+				'From',
+				[
+					'Alias',
+					[
+						'SelectQuery',
+						[
+							'Select',
+							[
+								['Field', '*'],
+								['Alias', ['Boolean', false], 'is blocked'],
+								['Alias', ['Text', 'Junior'], 'rank'],
+							],
+						],
+						['From', ['Table', 'copilot']],
+					],
+					'copilot',
+				],
+			],
+			[
+				'Where',
+				[
+					'IsNotDistinctFrom',
+					['ReferencedField', 'copilot', 'rank'],
+					['Bind', 0],
+				],
+			],
+		]);
+	}));
+
+test(
+	`/copilot?$select=id,rank&$filter=rank eq 'major'`,
+	'PATCH',
+	{ assists__pilot: 1 },
+	(result) =>
+		it(`should PATCH copilot based on filtered computed field rank`, () => {
+			expect(result).to.be.a.query.to.deep.equal([
+				'UpdateQuery',
+				['From', ['Table', 'copilot']],
+				[
+					'Where',
+					[
+						'In',
+						['ReferencedField', 'copilot', 'id'],
+						[
+							'SelectQuery',
+							['Select', [['ReferencedField', 'copilot', 'id']]],
+							[
+								'From',
+								[
+									'Alias',
+									[
+										'SelectQuery',
+										[
+											'Select',
+											[
+												['Field', '*'],
+												['Alias', ['Boolean', false], 'is blocked'],
+												['Alias', ['Text', 'Junior'], 'rank'],
+											],
+										],
+										['From', ['Table', 'copilot']],
+									],
+									'copilot',
+								],
+							],
+							[
+								'Where',
+								[
+									'IsNotDistinctFrom',
+									['ReferencedField', 'copilot', 'rank'],
+									['Bind', 0],
+								],
+							],
+						],
+					],
+				],
+				['Fields', ['assists-pilot']],
+				['Values', [['Bind', 'copilot', 'assists__pilot']]],
+			]);
+		}),
+);
diff --git a/test/model.sbvr b/test/model.sbvr
@@ -57,4 +57,10 @@ Fact type:  pilot has hire date
 
 Fact type:  pilot1 was trained by pilot2
 	Synonymous Form: pilot2 trained pilot1
-	Necessity: each pilot was trained by exactly one pilot
+	Necessity: each pilot was trained by exactly one pilot
+
+Term:	copilot
+	Concept Type:	pilot
+
+Fact Type: copilot assists pilot
+	Necessity: each copilot assists at most one pilot
diff --git a/test/select.js b/test/select.js
@@ -152,3 +152,81 @@ test('/pilot?$select=can_fly__plane/plane/id', (result) =>
 					['ReferencedField', 'pilot.pilot-can fly-plane.plane', 'id'],
 				],
 			])));
+
+test('/copilot?$select=*', (result) =>
+	it('should select * from copilot', () =>
+		expect(result).to.be.a.query.to.deep.equal([
+			'SelectQuery',
+			[
+				'Select',
+				[
+					['Alias', ['ReferencedField', 'copilot', 'created at'], 'created_at'],
+					[
+						'Alias',
+						['ReferencedField', 'copilot', 'modified at'],
+						'modified_at',
+					],
+					['ReferencedField', 'copilot', 'id'],
+					['ReferencedField', 'copilot', 'pilot'],
+					[
+						'Alias',
+						['ReferencedField', 'copilot', 'assists-pilot'],
+						'assists__pilot',
+					],
+					['Alias', ['ReferencedField', 'copilot', 'is blocked'], 'is_blocked'],
+					['ReferencedField', 'copilot', 'rank'],
+				],
+			],
+			[
+				'From',
+				[
+					'Alias',
+					[
+						'SelectQuery',
+						[
+							'Select',
+							[
+								['Field', '*'],
+								['Alias', ['Boolean', false], 'is blocked'],
+								['Alias', ['Text', 'Junior'], 'rank'],
+							],
+						],
+						['From', ['Table', 'copilot']],
+					],
+					'copilot',
+				],
+			],
+		])));
+
+test('/copilot?$select=id,is_blocked,rank', (result) =>
+	it('should select * from copilot', () =>
+		expect(result).to.be.a.query.to.deep.equal([
+			'SelectQuery',
+			[
+				'Select',
+				[
+					['ReferencedField', 'copilot', 'id'],
+					['Alias', ['ReferencedField', 'copilot', 'is blocked'], 'is_blocked'],
+					['ReferencedField', 'copilot', 'rank'],
+				],
+			],
+			[
+				'From',
+				[
+					'Alias',
+					[
+						'SelectQuery',
+						[
+							'Select',
+							[
+								['Field', '*'],
+								['Alias', ['Boolean', false], 'is blocked'],
+								['Alias', ['Text', 'Junior'], 'rank'],
+							],
+						],
+						['From', ['Table', 'copilot']],
+					],
+					'copilot',
+				],
+			],
+		])));