From a7482fe3994c7e90ba4aae4ae1df24db33ebc323 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sat, 16 Sep 2023 14:09:25 +0100 Subject: [PATCH 1/2] Add support for alias to Legacy Models https://github.com/DEFRA/water-abstraction-team/issues/97 We hit a snag as part of the **Great Rename**! We want `water.charge_elements` to be `ChargeReferences`. We then want `water.charge_purposes` to be `ChargeElements`. But there is a relationship between the 2 that has to be defined in `ChargeReferenceModel`. The problem is when you run `const query = await ChargeReferenceModel.query().innerJoinRelated('chargeElements')` you get an error from [Objection.js](https://vincit.github.io/objection.js). ``` select "charge_elements".* from "water"."charge_elements" inner join "water"."charge_purposes" as "charge_elements" on "charge_elements"."charge_element_id" = "charge_elements"."charge_element_id" - table name "charge_elements" specified more than once ``` If we can get Objection to alias `charge_elements` as `charge_references` in our query then we would remove the error. To do this we need to use `alias()` in the custom query builder used by the model. Currently, all our models extend `LegacyBaseModel` which uses `SchemaQueryBuilder` to ensure all legacy models apply a schema. This change refactors `SchemaQueryBuilder` to also handle applying a table alias when one exists. From 547f87a5a508177195f03bf0bf0438e88c2a9072 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Sun, 17 Sep 2023 16:55:55 +0100 Subject: [PATCH 2/2] Implement new legacy query builder --- app/models/legacy-base.model.js | 13 ++--- app/models/legacy.query-builder.js | 31 +++++++++++ test/models/legacy.query-builder.test.js | 66 ++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 10 deletions(-) create mode 100644 app/models/legacy.query-builder.js create mode 100644 test/models/legacy.query-builder.test.js diff --git a/app/models/legacy-base.model.js b/app/models/legacy-base.model.js index e91f8aef3b..60d8eef311 100644 --- a/app/models/legacy-base.model.js +++ b/app/models/legacy-base.model.js @@ -5,10 +5,10 @@ * @module LegacyBaseModel */ -const { QueryBuilder } = require('objection') const path = require('path') const BaseModel = require('./base.model.js') +const LegacyQueryBuilder = require('./legacy.query-builder.js') class LegacyBaseModel extends BaseModel { /** @@ -36,10 +36,10 @@ class LegacyBaseModel extends BaseModel { * * See `schema()` for further details. * - * @returns {Object} a custom Objection `QueryBuilder` + * @returns {module:LegacyQueryBuilder} a custom Objection `QueryBuilder` */ static get QueryBuilder () { - return SchemaQueryBuilder + return LegacyQueryBuilder } /** @@ -223,11 +223,4 @@ class LegacyBaseModel extends BaseModel { } } -class SchemaQueryBuilder extends QueryBuilder { - constructor (modelClass) { - super(modelClass) - this.withSchema(modelClass.schema) - } -} - module.exports = LegacyBaseModel diff --git a/app/models/legacy.query-builder.js b/app/models/legacy.query-builder.js new file mode 100644 index 0000000000..fee0ef0711 --- /dev/null +++ b/app/models/legacy.query-builder.js @@ -0,0 +1,31 @@ +'use strict' + +/** + * Wrapper around Objection.js QueryBuilder to add schema and alias support to Legacy Models + * @module LegacyBaseModel + */ + +const { QueryBuilder } = require('objection') + +class LegacyQueryBuilder extends QueryBuilder { + /** + * Wrapper around Objection.js QueryBuilder to add schema and alias support to Legacy Models + * + * @param {Object} modelClass The Objection legacy model to which this QueryBuilder will be applied + */ + constructor (modelClass) { + super(modelClass) + + // ALL legacy models must implement a `schema` property. So, we always call this + this.withSchema(modelClass.schema) + + // Only alias those that need to alias the table name to avoid errors, for example, needing to alias + // `charge_elements` as 'charge_references' so we can link them to `charge_purposes` but call purposes + // `ChargeElements`. + if (modelClass.alias) { + this.alias(modelClass.alias) + } + } +} + +module.exports = LegacyQueryBuilder diff --git a/test/models/legacy.query-builder.test.js b/test/models/legacy.query-builder.test.js new file mode 100644 index 0000000000..d7dc28fdb4 --- /dev/null +++ b/test/models/legacy.query-builder.test.js @@ -0,0 +1,66 @@ +'use strict' + +'use strict' + +// Test framework dependencies +const Lab = require('@hapi/lab') +const Code = require('@hapi/code') + +const { describe, it } = exports.lab = Lab.script() +const { expect } = Code + +// Test helpers +const LegacyBaseModel = require('../../app/models/legacy-base.model.js') + +// Thing under test +const LegacyQueryBuilder = require('../../app/models/legacy.query-builder.js') + +describe('Legacy Base model', () => { + describe('when given a model class with only a schema', () => { + class SchemaLegacyModel extends LegacyBaseModel { + static get tableName () { + return 'schemaTable' + } + + static get schema () { + return 'water' + } + + static get QueryBuilder () { + return LegacyQueryBuilder + } + } + + it('adds the schema name to the table in the queries it generates', () => { + const result = SchemaLegacyModel.query().toKnexQuery().toQuery() + + expect(result).to.endWith('from "water"."schema_table"') + }) + }) + + describe('when given a model class with an alias', () => { + class AliasLegacyModel extends LegacyBaseModel { + static get tableName () { + return 'aliasTable' + } + + static get schema () { + return 'water' + } + + static get alias () { + return 'chargeReferences' + } + + static get QueryBuilder () { + return LegacyQueryBuilder + } + } + + it('aliases the table in the queries it generates', () => { + const result = AliasLegacyModel.query().toKnexQuery().toQuery() + + expect(result).to.endWith('from "water"."alias_table" as "charge_references"') + }) + }) +})