Fixes columnNameMappers applied to json column when using FieldExpres…

…sions (Vincit#1089)
inarix · Mar 15, 2024 · a8c937f · a8c937f
1 parent f0c3bf5
commit a8c937f
Show file tree

Hide file tree

Showing 12 changed files with 190 additions and 14 deletions.
diff --git a/.gitignore b/.gitignore
@@ -12,4 +12,5 @@ profiling
 npm-debug.log*
 .vscode
 /**/yarn.lock
-/**/.vuepress/dist
+/**/.vuepress/dist
+docker-compose.override.yml
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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() {