diff --git a/.gitignore b/.gitignore index e9aff8eab..dc48b04c0 100644 --- a/.gitignore +++ b/.gitignore @@ -12,4 +12,5 @@ profiling npm-debug.log* .vscode /**/yarn.lock -/**/.vuepress/dist \ No newline at end of file +/**/.vuepress/dist +docker-compose.override.yml \ No newline at end of file diff --git a/lib/model/Model.js b/lib/model/Model.js index 5c160eb11..bd9a47939 100644 --- a/lib/model/Model.js +++ b/lib/model/Model.js @@ -98,19 +98,26 @@ class Model { const columnNameMappers = this.constructor.getColumnNameMappers(); if (columnNameMappers) { - json = columnNameMappers.parse(json); + json = columnNameMappers.parse(json, this.isJsonColumn(this.constructor.$$columnInfo)); } return parseJsonAttributes(json, this.constructor); } + isJsonColumn(columnInfo) { + return (columnName) => + typeof columnInfo == 'object' && + columnInfo.hasOwnProperty(columnName) && + (columnInfo[columnName]['type'] == 'json' || columnInfo[columnName]['type'] == 'jsonb'); + } + $formatDatabaseJson(json) { const columnNameMappers = this.constructor.getColumnNameMappers(); json = formatJsonAttributes(json, this.constructor); if (columnNameMappers) { - json = columnNameMappers.format(json); + json = columnNameMappers.format(json, this.isJsonColumn(this.constructor.$$columnInfo)); } return json; @@ -405,6 +412,10 @@ class Model { return this.modifiers || {}; } + static async columnInfo(creator) { + return await cachedGetAsync(this, '$$columnInfo', creator); + } + static columnNameToPropertyName(columnName) { let colToProp = cachedGet(this, '$$colToProp', () => new Map()); let propertyName = colToProp.get(columnName); @@ -816,6 +827,14 @@ function relatedQuery({ modelClass, relationName, transaction, alwaysReturnArray }); } +async function cachedGetAsync(target, hiddenPropertyName, creator) { + if (!target.hasOwnProperty(hiddenPropertyName)) { + defineNonEnumerableProperty(target, hiddenPropertyName, await creator(target)); + } + + return target[hiddenPropertyName]; +} + function cachedGet(target, hiddenPropertyName, creator) { if (!target.hasOwnProperty(hiddenPropertyName)) { defineNonEnumerableProperty(target, hiddenPropertyName, creator(target)); diff --git a/lib/model/modelUtils.js b/lib/model/modelUtils.js index bc65101be..4b73d5ad2 100644 --- a/lib/model/modelUtils.js +++ b/lib/model/modelUtils.js @@ -17,6 +17,7 @@ const staticHiddenProps = [ '$$readOnlyAttributes', '$$idRelationProperty', '$$virtualAttributes', + '$$columnInfo', ]; function defineNonEnumerableProperty(obj, prop, value) { diff --git a/lib/queryBuilder/QueryBuilder.js b/lib/queryBuilder/QueryBuilder.js index a5e0e4874..d7eccb26a 100644 --- a/lib/queryBuilder/QueryBuilder.js +++ b/lib/queryBuilder/QueryBuilder.js @@ -440,8 +440,11 @@ class QueryBuilder extends QueryBuilderBase { async execute() { // Take a clone so that we don't modify this instance during execution. const builder = this.clone(); + const colBuilder = this.clone(); try { + // Lazy fetch columns info (fixes #1089) + await builder._modelClass.columnInfo(async (_) => await colBuilder.columnInfo()); await beforeExecute(builder); const result = await doExecute(builder); return await afterExecute(builder, result); @@ -1097,7 +1100,6 @@ function doExecute(builder) { promise = Promise.resolve(queryExecutorOperation.queryExecutor(builder)); } else { promise = Promise.resolve(buildKnexQuery(builder)); - promise = chainOperationHooks(promise, builder, 'onRawResult'); promise = promise.then((result) => createModels(result, builder)); } diff --git a/lib/utils/identifierMapping.js b/lib/utils/identifierMapping.js index dfa2164c9..ef99df5d8 100644 --- a/lib/utils/identifierMapping.js +++ b/lib/utils/identifierMapping.js @@ -145,7 +145,7 @@ function mapLastPart(mapper, separator) { // Returns a function that takes an object as an input and maps the object's keys // using `mapper`. If the input is not an object, the input is returned unchanged. function keyMapper(mapper) { - return (obj) => { + return (obj, isJsonColumn = () => false) => { if (!isObject(obj) || Array.isArray(obj)) { return obj; } @@ -155,7 +155,17 @@ function keyMapper(mapper) { for (let i = 0, l = keys.length; i < l; ++i) { const key = keys[i]; - out[mapper(key)] = obj[key]; + + if (key.indexOf(':') > -1) { + const parts = key.split(':'); + if (isJsonColumn(mapper(parts[0]))) { + out[`${mapper(parts[0])}:${parts[1]}`] = obj[key]; + } else { + out[mapper(key)] = obj[key]; + } + } else { + out[mapper(key)] = obj[key]; + } } return out; diff --git a/tests/integration/graph/GraphInsert.js b/tests/integration/graph/GraphInsert.js index 40b7d897c..b3d477ee0 100644 --- a/tests/integration/graph/GraphInsert.js +++ b/tests/integration/graph/GraphInsert.js @@ -1116,7 +1116,13 @@ module.exports = (session) => { function createModels() { mockKnex = mockKnexFactory(session.knex, function (_, oldImpl, args) { - ++numExecutedQueries; + const queryString = this.toSQL().sql; + if ( + queryString.match(/select \* from information_schema\.columns/) == null && + queryString.match(/PRAGMA/) == null + ) { + ++numExecutedQueries; + } return oldImpl.apply(this, args); }); diff --git a/tests/integration/misc/#1089.js b/tests/integration/misc/#1089.js new file mode 100644 index 000000000..aaae8d92c --- /dev/null +++ b/tests/integration/misc/#1089.js @@ -0,0 +1,58 @@ +const _ = require('lodash'); +const expect = require('expect.js'); +const { Model, snakeCaseMappers } = require('../../../'); + +module.exports = (session) => { + describe("Objection shouldn't touch JSON fields using columnNameMappers and FieldExpressions (#1089)", () => { + class A extends Model { + static get tableName() { + return 'a'; + } + + static get columnNameMappers() { + return snakeCaseMappers(); + } + } + + beforeEach(() => { + return session.knex.schema + .dropTableIfExists('a') + .createTable('a', (table) => { + table.integer('id').primary(); + table.jsonb('my_json').nullable().defaultTo(null); + }) + .then(() => { + return Promise.all([ + session.knex('a').insert({ + id: 1, + my_json: JSON.stringify([{ innerKey: 2 }]), + }), + ]); + }); + }); + + afterEach(() => { + return session.knex.schema.dropTableIfExists('a'); + }); + + it("json field keys aren't modified with columnNameMappers with FieldExpressions", () => { + if (!session.isPostgres()) { + // Note(cesumilo): Only working on postgresql. + return expect(true).to.eql(true); + } + + return A.query(session.knex) + .patch({ + 'myJson:[0][innerKey]': 1, + }) + .then((result) => { + expect(result).to.eql(1); + return A.query(session.knex); + }) + .then((results) => { + expect(results[0].id).to.eql(1); + expect(results[0].myJson[0].innerKey).to.eql(1); + }); + }); + }); +}; diff --git a/tests/integration/upsertGraph.js b/tests/integration/upsertGraph.js index 5ac663a38..19c5c4ce9 100644 --- a/tests/integration/upsertGraph.js +++ b/tests/integration/upsertGraph.js @@ -165,7 +165,13 @@ module.exports = (session) => { // Wrap the transaction to catch the executed sql. trx = mockKnexFactory(trx, function (mock, oldImpl, args) { - sql.push(this.toString()); + const queryString = this.toString(); + if ( + queryString.match(/PRAGMA/) == null && + queryString.match(/select \* from information_schema\.columns/) == null + ) { + sql.push(queryString); + } return oldImpl.apply(this, args); }); @@ -710,7 +716,13 @@ module.exports = (session) => { // Wrap the transaction to catch the executed sql. trx = mockKnexFactory(trx, function (mock, oldImpl, args) { - sql.push(this.toString()); + const queryString = this.toString(); + if ( + queryString.match(/select \* from information_schema\.columns/) == null && + queryString.match(/PRAGMA/) == null + ) { + sql.push(queryString); + } return oldImpl.apply(this, args); }); @@ -2668,7 +2680,13 @@ module.exports = (session) => { // Wrap the transaction to catch the executed sql. trx = mockKnexFactory(trx, function (mock, oldImpl, args) { - sql.push(this.toString()); + const queryString = this.toString(); + if ( + queryString.match(/select \* from information_schema\.columns/) == null && + queryString.match(/PRAGMA/) == null + ) { + sql.push(queryString); + } return oldImpl.apply(this, args); }); diff --git a/tests/unit/queryBuilder/QueryBuilder.js b/tests/unit/queryBuilder/QueryBuilder.js index d02884480..bd5020a43 100644 --- a/tests/unit/queryBuilder/QueryBuilder.js +++ b/tests/unit/queryBuilder/QueryBuilder.js @@ -17,12 +17,24 @@ describe('QueryBuilder', () => { let executedQueries = []; let mockKnex = null; let TestModel = null; + let keepColumnInfo = false; + let skippedColumnInfo = 0; before(() => { let knex = Knex({ client: 'pg' }); mockKnex = knexMocker(knex, function (mock, oldImpl, args) { - executedQueries.push(this.toString()); + const queryString = this.toString(); + const matchColInfo = queryString.match(/select \* from information_schema\.columns/) != null; + + if (matchColInfo && keepColumnInfo && skippedColumnInfo++ > 0) { + executedQueries.push(queryString); + } else if (!matchColInfo) { + executedQueries.push(queryString); + } else { + const promise = Promise.resolve([]); + return promise.then.apply(promise, args); + } let result = mockKnexQueryResults[mockKnexQueryResultIndex++] || []; let promise = Promise.resolve(result); @@ -35,6 +47,8 @@ describe('QueryBuilder', () => { mockKnexQueryResults = []; mockKnexQueryResultIndex = 0; executedQueries = []; + keepColumnInfo = false; + skippedColumnInfo = 0; TestModel = class TestModel extends Model { static get tableName() { @@ -925,6 +939,8 @@ describe('QueryBuilder', () => { }); it('should consider withSchema when looking for column info', (done) => { + keepColumnInfo = true; + class TestModelRelated extends Model { static get tableName() { return 'Related'; diff --git a/tests/unit/relations/BelongsToOneRelation.js b/tests/unit/relations/BelongsToOneRelation.js index a56506588..5604464ac 100644 --- a/tests/unit/relations/BelongsToOneRelation.js +++ b/tests/unit/relations/BelongsToOneRelation.js @@ -20,11 +20,24 @@ describe('BelongsToOneRelation', () => { let relation; let compositeKeyRelation; + let keepColumnInfo = false; + let skippedColumnInfo = 0; + before(() => { let knex = Knex({ client: 'pg' }); mockKnex = knexMocker(knex, function (mock, oldImpl, args) { - executedQueries.push(this.toString()); + const queryString = this.toString(); + const matchColInfo = queryString.match(/select \* from information_schema\.columns/) != null; + + if (matchColInfo && keepColumnInfo && skippedColumnInfo++ > 0) { + executedQueries.push(queryString); + } else if (!matchColInfo) { + executedQueries.push(queryString); + } else { + const promise = Promise.resolve([]); + return promise.then.apply(promise, args); + } let result = mockKnexQueryResults.shift() || []; let promise = Promise.resolve(result); @@ -36,6 +49,8 @@ describe('BelongsToOneRelation', () => { beforeEach(() => { mockKnexQueryResults = []; executedQueries = []; + keepColumnInfo = false; + skippedColumnInfo = 0; OwnerModel = class OwnerModel extends Model { static get tableName() { diff --git a/tests/unit/relations/HasManyRelation.js b/tests/unit/relations/HasManyRelation.js index 9ce41a7fd..611f75a9e 100644 --- a/tests/unit/relations/HasManyRelation.js +++ b/tests/unit/relations/HasManyRelation.js @@ -20,11 +20,24 @@ describe('HasManyRelation', () => { let relation; let compositeKeyRelation; + let keepColumnInfo = false; + let skippedColumnInfo = 0; + before(() => { let knex = Knex({ client: 'pg' }); mockKnex = knexMocker(knex, function (mock, oldImpl, args) { - executedQueries.push(this.toString()); + const queryString = this.toString(); + const matchColInfo = queryString.match(/select \* from information_schema\.columns/) != null; + + if (matchColInfo && keepColumnInfo && skippedColumnInfo++ > 0) { + executedQueries.push(queryString); + } else if (!matchColInfo) { + executedQueries.push(queryString); + } else { + const promise = Promise.resolve([]); + return promise.then.apply(promise, args); + } let result = mockKnexQueryResults.shift() || []; let promise = Promise.resolve(result); @@ -36,6 +49,8 @@ describe('HasManyRelation', () => { beforeEach(() => { mockKnexQueryResults = []; executedQueries = []; + keepColumnInfo = false; + skippedColumnInfo = 0; OwnerModel = class OwnerModel extends Model { static get tableName() { diff --git a/tests/unit/relations/ManyToManyRelation.js b/tests/unit/relations/ManyToManyRelation.js index 2d5f537cd..d0a8e8990 100644 --- a/tests/unit/relations/ManyToManyRelation.js +++ b/tests/unit/relations/ManyToManyRelation.js @@ -23,11 +23,24 @@ describe('ManyToManyRelation', () => { let relation; let compositeKeyRelation; + let keepColumnInfo = false; + let skippedColumnInfo = 0; + beforeEach(() => { let knex = Knex({ client: 'pg' }); mockKnex = knexMocker(knex, function (mock, oldImpl, args) { - executedQueries.push(this.toString()); + const queryString = this.toString(); + const matchColInfo = queryString.match(/select \* from information_schema\.columns/) != null; + + if (matchColInfo && keepColumnInfo && skippedColumnInfo++ > 0) { + executedQueries.push(queryString); + } else if (!matchColInfo) { + executedQueries.push(queryString); + } else { + const promise = Promise.resolve([]); + return promise.then.apply(promise, args); + } let result = mockKnexQueryResults.shift() || []; let promise = Promise.resolve(result); @@ -39,6 +52,8 @@ describe('ManyToManyRelation', () => { beforeEach(() => { mockKnexQueryResults = []; executedQueries = []; + keepColumnInfo = false; + skippedColumnInfo = 0; OwnerModel = class OwnerModel extends Model { static get tableName() {