diff --git a/app/lib/legacy-db-snake-case-mappers.lib.js b/app/lib/legacy-db-snake-case-mappers.lib.js new file mode 100644 index 0000000000..35b060e664 --- /dev/null +++ b/app/lib/legacy-db-snake-case-mappers.lib.js @@ -0,0 +1,69 @@ +'use strict' + +/** + * General helper methods + * @module LegacyDbSnakeCaseMappersLib + */ + +const { camelCase, knexIdentifierMappers, snakeCase } = require('objection/lib/utils/identifierMapping') + +/** + * Return an object containing Knex postProcessResponse() and wrapIdentifier() hooks used in Db query and result parsing + * + * The standard problem with a JavaScript app talking to a DB is the convention for SQL is to use snake_case for field + * names and in Javascript it's camelCase. When dealing with results or sending data to the DB, in code you want to use + * camelCase. But the db needs to see snake_case. + * + * Both Objection.js and Knex have solutions for this; generally referred to as 'snake case mappers'. + * `knexfile.application.js` has more details on this. + * + * But we have had to customise the out-of-the-box solution because of naming choices made by the previous delivery team + * in the legacy DB. Specifically, using 'crm_v2' for a schema name. The out-of-the-box solution has an option, + * `underscoreBeforeDigits`, for dealing with property names like `addressLine1`. Without it the DB would be sent + * `address_line1`. + * + * So, we have to have this set. But that breaks the schema name parsing. `crmV2` becomes `crm_v_2`. We cannot think of + * a way to express it in the code which will make the out-of-the-box solution work. SO, instead we have had to create + * our own legacyDbSnakeCaseMappers(). + * + * It simply looks for the value 'crm_v2' and when seen, returns it as is without any formatting. For everything else, + * it passes control to the out-of-the-box solution. + * + * @param {Object} opt Object containing options used by + * {@link https://vincit.github.io/objection.js/api/objection/#knexsnakecasemappers|knexsnakecasemappers()} + * + * @returns object containing Knex postProcessResponse() and wrapIdentifier() hooks + */ +function legacyDbSnakeCaseMappers (opt = {}) { + return knexIdentifierMappers({ + parse: (str) => _legacyCamelCase(str, opt), + format: (str) => _legacySnakeCase(str, opt) + }) +} + +function _legacyCamelCase (str, { upperCase = false } = {}) { + if (str === 'crm_v2') { + return str + } + + return camelCase(str, { upperCase }) +} + +function _legacySnakeCase ( + str, + { + upperCase = false, + underscoreBeforeDigits = false, + underscoreBetweenUppercaseLetters = false + } = {} +) { + if (str === 'crm_v2') { + return str + } + + return snakeCase(str, { upperCase, underscoreBeforeDigits, underscoreBetweenUppercaseLetters }) +} + +module.exports = { + legacyDbSnakeCaseMappers +} diff --git a/knexfile.application.js b/knexfile.application.js index e2f7ff0692..0e5bc7f439 100644 --- a/knexfile.application.js +++ b/knexfile.application.js @@ -1,10 +1,11 @@ 'use strict' -const { knexSnakeCaseMappers } = require('objection') +const { legacyDbSnakeCaseMappers } = require('./app/lib/legacy-db-snake-case-mappers.lib.js') /** - * Passing in `knexSnakeCaseMappers` allows us to use camelCase everywhere and knex will convert it to snake_case on - * the fly. + * Passing in our variant of `knexSnakeCaseMappers` allows us to use camelCase everywhere and knex will convert it to + * snake_case on the fly. (see `app/lib/legacy-db-snake-case-mappers.lib.js` for details on why we have our own + * variant) * * This means when we access a property on the model we can use camelCase even if the underlying database property * was snake_case. It also means we get camelCase object keys, handy when you need to return a db query result as is @@ -14,18 +15,16 @@ const { knexSnakeCaseMappers } = require('objection') * * We set the `underscoreBeforeDigits` option so that properties like lineAttr1 are correctly changed to line_attr_1. * - * However this causes issues with migrations as it still applies the underscore before the digit even if the rest of - * the name is snake case. So for example, a migration to create line_attr_1 will actually create line_attr__1. We - * therefore only add `knexSnakeCaseMappers` when running the application to ensure that it isn't applied to - * migrations. - * - * + * However, this causes issues with migrations as it works differently. It still applies the underscore before the digit + * even if the rest of the name is snake case. For example, a migration to create `line_attr_1` will actually create + * `line_attr__1`. So, we only add `knexSnakeCaseMappers` when running the application to ensure that it isn't applied + * to migrations. */ const knexfile = require('./knexfile') for (const environment in knexfile) { - Object.assign(knexfile[environment], knexSnakeCaseMappers({ underscoreBeforeDigits: true })) + Object.assign(knexfile[environment], legacyDbSnakeCaseMappers({ underscoreBeforeDigits: true })) } module.exports = { ...knexfile } diff --git a/test/lib/legacy-db-snake-case-mappers.lib.test.js b/test/lib/legacy-db-snake-case-mappers.lib.test.js new file mode 100644 index 0000000000..31474529da --- /dev/null +++ b/test/lib/legacy-db-snake-case-mappers.lib.test.js @@ -0,0 +1,117 @@ +'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 + +// Thing under test +const LegacyDbSnakeCaseMappersLib = require('../../app/lib/legacy-db-snake-case-mappers.lib.js') + +// NOTE: Ideally, we would have liked to spy on the Objection snakeCase and camelCase methods to confirm they are or are +// not being called depending on the circumstance. But all our attempts with Sinon failed (a common issue we have when +// testing with Objection.js) +describe('RequestLib', () => { + describe('#legacyDbSnakeCaseMappers', () => { + // We always pass in these options. See knexfile.application.js and how legacyDbSnakeCaseMappers() is called + const options = { underscoreBeforeDigits: true } + + describe('when called', () => { + it('returns an object containing knex wrapIdentifier() and postProcessResponse() hooks', (options) => { + const result = LegacyDbSnakeCaseMappersLib.legacyDbSnakeCaseMappers() + + expect(result).to.include('wrapIdentifier') + expect(result).to.include('postProcessResponse') + }) + }) + + describe('when the postProcessResponse() hook it creates is used', () => { + const dbResult = [ + { + address_line_1: '10 Downing Street', + purpose: 'Residence of the prime minster', + is_occupied: true, + section_127_Agreement: false, + crm_v2: 'I am really a schema disguised as a table column :-)' + } + ] + + it('handles the knex db result object as expected', () => { + const identifierMapping = LegacyDbSnakeCaseMappersLib.legacyDbSnakeCaseMappers(options) + const result = identifierMapping.postProcessResponse(dbResult) + + expect(result).to.equal([ + { + addressLine1: '10 Downing Street', + purpose: 'Residence of the prime minster', + isOccupied: true, + section127Agreement: false, + crm_v2: 'I am really a schema disguised as a table column :-)' + } + ]) + }) + }) + + describe('when the wrapIdentifier() hook it creates is used', () => { + function origWrap (identifier) { + return identifier + } + + describe('and passed the identifier `addressLine1`', () => { + it('returns `address_line_1`', () => { + const identifierMapping = LegacyDbSnakeCaseMappersLib.legacyDbSnakeCaseMappers(options) + const result = identifierMapping.wrapIdentifier('addressLine1', origWrap) + + expect(result).to.equal('address_line_1') + }) + }) + + describe('and passed the identifier `purpose`', () => { + it('returns `purpose`', () => { + const identifierMapping = LegacyDbSnakeCaseMappersLib.legacyDbSnakeCaseMappers(options) + const result = identifierMapping.wrapIdentifier('purpose', origWrap) + + expect(result).to.equal('purpose') + }) + }) + + describe('and passed the identifier `isOccupied`', () => { + it('returns `is_occupied`', () => { + const identifierMapping = LegacyDbSnakeCaseMappersLib.legacyDbSnakeCaseMappers(options) + const result = identifierMapping.wrapIdentifier('isOccupied', origWrap) + + expect(result).to.equal('is_occupied') + }) + }) + + describe('and passed the identifier `section127Agreement`', () => { + it('returns `section_127_agreement`', () => { + const identifierMapping = LegacyDbSnakeCaseMappersLib.legacyDbSnakeCaseMappers(options) + const result = identifierMapping.wrapIdentifier('section127Agreement', origWrap) + + expect(result).to.equal('section_127_agreement') + }) + }) + + describe('and passed the identifier `crm_v2`', () => { + it('returns `crm_v2` (it does no formatting)', () => { + const identifierMapping = LegacyDbSnakeCaseMappersLib.legacyDbSnakeCaseMappers(options) + const result = identifierMapping.wrapIdentifier('crm_v2', origWrap) + + expect(result).to.equal('crm_v2') + }) + }) + + describe('and passed the identifier `foo_v2`', () => { + it('returns `foo_v_2` (confirmation on crm_v2 is special!)', () => { + const identifierMapping = LegacyDbSnakeCaseMappersLib.legacyDbSnakeCaseMappers(options) + const result = identifierMapping.wrapIdentifier('foo_v2', origWrap) + + expect(result).to.equal('foo_v_2') + }) + }) + }) + }) +})