Add legacy db snake case mappers #131
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://eaflood.atlassian.net/browse/WATER-3896 Whilst working on [Add SROC Supplementary Billing Invoice Service](#119) we hit an issue. It was the first time we needed to query the `crm_v2` schema. But that threw errors due to the way [Objection.js knexSnakeCaseMappers()](https://vincit.github.io/objection.js/api/objection/#knexsnakecasemappers) works. It uses Knex's [wrapIdentifier()](https://knexjs.org/guide/#wrapidentifier) and [postProcessResponse()](https://knexjs.org/guide/#postprocessresponse) hooks to see each 'identifier' name. It can then test whether it needs converting, either from or to snake case. For example, `knex('table').withSchema('foo').select('table.field as otherName').where('id', 1)` will call `wrapIdentifier()` for the values `'table'`, `'foo'`, `'table'`, `'field'`, `'otherName'` and `'id'`. **knexSnakeCaseMappers()** takes some options, one of them being `underscoreBeforeDigits`. If we didn't use this fields like `address_line_1` or `section_127_agreement` in the DB would be incorrectly converted to `address_line1` and `section127_agreement`. But this is what leads to our problem. The previous teams' decision to name one of the schemas `crm_v2` leads to an incorrect conversion. **knexSnakeCaseMappers()** is seeing this and returning `'crm_v_2'`. It doesn't know it's a schema instead of a column name because Knex doesn't provide that context. This is the exact same issue we faced in [Make timestamps consistent at model layer](#85). Thankfully, a solution that didn't work there will work here. We can provide our own custom knex snake case mappers implementation which knows to ignore `'crm_v_2'`, whilst calling Objection.js own methods for everything else. It also gives us a solution if we face any more funnies like this when dealing with the legacy data.
Cruikshanks
added
the
housekeeping
Cruikshanks
changed the title
Cruikshanks
changed the title
We're testing the behaviour we expect from the mappers; that they do what we expect with everything but 'crm_v2' which should just be ignored. (See note about our failed attempts to confirm how Objection is being used. It's not critical though. I was just a nice to have!)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-3896
Whilst working on Add SROC Supplementary Billing Invoice Service we hit an issue. It was the first time we needed to query the
crm_v2schema. But that threw errors due to the way Objection.js knexSnakeCaseMappers() works. It uses Knex's wrapIdentifier() and postProcessResponse() hooks to see each 'identifier' name. It can then test whether it needs converting, either from or to snake case.For example,
knex('table').withSchema('foo').select('table.field as otherName').where('id', 1)will callwrapIdentifier()for the values'table','foo','table','field','otherName'and'id'.knexSnakeCaseMappers() takes some options, one of them being
underscoreBeforeDigits. If we didn't use this, fields likeaddress_line_1orsection_127_agreementin the DB would be incorrectly converted toaddress_line1andsection127_agreement. But this is what leads to our problem.The previous teams' decision to name one of the schemas
crm_v2leads to an incorrect conversion. knexSnakeCaseMappers() is seeing this and returning'crm_v_2'. It doesn't know it's a schema instead of a column name because Knex doesn't provide that context. This is the exact same issue we faced in Make timestamps consistent at model layer.Thankfully, a solution that didn't work there will work here. We can provide our own custom Knex snake case mappers implementation which knows to ignore
'crm_v_2', whilst calling Objection.js own methods for everything else. It also gives us a solution if we face any more funnies like this when dealing with the legacy database.