Keep field expression keys in patch() and merge() data when building the query (#53)

doc/release-notes/changelog.md

@@ -74,6 +74,7 @@
 - `cancelQuery()` with a single row in the `beforeInsert` hook of an `insert()` merges it into the inserted model, like an array of rows, instead of ignoring it [#66](https://github.com/ditojs/objection/issues/66)
 - `insertGraph()` and `upsertGraph()` reject only after the queries they already started have finished, so a transaction is rolled back after them. Before, rows of these queries could be committed after the rollback on SQLite. [#335](https://github.com/ditojs/objection/pull/335)
 - `upsertGraph()` sets the foreign key of a model that a `#ref` relates into a `HasManyRelation` or `HasOneRelation`. Before, the row was left without it, and the id of the owner could be cleared [#311](https://github.com/ditojs/objection/issues/311)
+- `patch()` and `merge()` with field expression keys like `'meta:color'` keep writing them when the query is built more than once, e.g. with `toKnexQuery()` before running it, or through a `clone()`, instead of resolving to 0 without a write. Building the query no longer changes the data of `patch()` and `merge()`: the patched model keeps these keys until the write has run, so `$afterUpdate()` sees them, and then loses them, also with `snakeCaseMappers()`, so the models that `$query().patch()`, `patchAndFetch*()` and `upsertGraph()` return don't have them [#53](https://github.com/ditojs/objection/issues/53)
 
 ### Types
 

lib/queryBuilder/operations/MergeOperation.js

@@ -29,7 +29,7 @@ export class MergeOperation extends QueryBuilderOperation {
 
     if (this.model) {
       const json = this.model.$toDatabaseJson(builder);
-      const convertedJson = convertFieldExpressionsToRaw(builder, this.model, json);
+      const convertedJson = convertFieldExpressionsToRaw(builder, json);
 
       return knexBuilder.merge(convertedJson);
     }

lib/queryBuilder/operations/UpdateOperation.js

@@ -2,7 +2,11 @@ import { isEmpty } from '../../utils/objectUtils.js';
 import { QueryBuilderOperation } from './QueryBuilderOperation.js';
 import { StaticHookArguments } from '../StaticHookArguments.js';
 import { removeOverriddenValues } from '../../model/modelQueryProps.js';
-import { checkFieldExpressions, convertFieldExpressionsToRaw } from './fieldExpressions.js';
+import {
+  checkFieldExpressions,
+  convertFieldExpressionsToRaw,
+  removeFieldExpressions,
+} from './fieldExpressions.js';
 
 export class UpdateOperation extends QueryBuilderOperation {
   constructor(name, opt) {
@@ -57,7 +61,7 @@ export class UpdateOperation extends QueryBuilderOperation {
     const json = this.model.$toDatabaseJson(builder);
 
     if (this.modelOptions.patch) {
-      return knexBuilder.update(convertFieldExpressionsToRaw(builder, this.model, json));
+      return knexBuilder.update(convertFieldExpressionsToRaw(builder, json));
     } else {
       checkFieldExpressions(builder.modelClass(), this.model, 'update');
       return knexBuilder.update(json);
@@ -66,6 +70,9 @@ export class UpdateOperation extends QueryBuilderOperation {
 
   async onAfter2(builder, numUpdated) {
     await this.model.$afterUpdate(this.modelOptions, builder.context());
+    // The patched model is what the write returns, e.g. to `upsertGraph()` and
+    // `patchAndFetch()`, or what it merges into the instance of `$query()`:
+    removeFieldExpressions(builder.modelClass(), this.model);
     return numUpdated;
   }
 

lib/queryBuilder/operations/fieldExpressions.js

@@ -10,10 +10,10 @@ import { getQueryPropKeys } from '../../model/modelQueryProps.js';
 // columns with field expressions like `'meta:color'`. Keys with a ':' that don't
 // parse as field expressions are columns, as in the query properties.
 export function checkFieldExpressions(modelClass, model, operation) {
-  const jsonSchema = modelClass.getJsonSchema();
-  const keys = [...Object.keys(model), ...getQueryPropKeys(model)].filter(
-    (key) => getFieldExpressionColumn(key, jsonSchema?.properties) !== null,
-  );
+  const keys = getFieldExpressionKeys(modelClass, [
+    ...Object.keys(model),
+    ...getQueryPropKeys(model),
+  ]);
 
   if (keys.length > 0) {
     throw modelClass.createValidationError({
@@ -28,7 +28,15 @@ export function checkFieldExpressions(modelClass, model, operation) {
   }
 }
 
-export function convertFieldExpressionsToRaw(builder, model, json) {
+// Field expressions only describe the writes into json columns, they aren't
+// properties of the models that the writes return:
+export function removeFieldExpressions(modelClass, model) {
+  for (const key of getFieldExpressionKeys(modelClass, Object.keys(model))) {
+    delete model[key];
+  }
+}
+
+export function convertFieldExpressionsToRaw(builder, json) {
   const knex = builder.knex();
   const dialect = getJsonDialect(knex);
   const modelClass = builder.modelClass();
@@ -78,8 +86,6 @@ export function convertFieldExpressionsToRaw(builder, model, json) {
         dialect.set('??', parsed.parsedExpr.access, valuePlaceholder),
         [convertedJson[parsed.column] || `${builder.tableRef()}.${parsed.column}`, val],
       );
-
-      delete model[key];
     } else {
       convertedJson[key] = val;
     }
@@ -88,6 +94,11 @@ export function convertFieldExpressionsToRaw(builder, model, json) {
   return convertedJson;
 }
 
+function getFieldExpressionKeys(modelClass, keys) {
+  const jsonSchema = modelClass.getJsonSchema();
+  return keys.filter((key) => getFieldExpressionColumn(key, jsonSchema?.properties) !== null);
+}
+
 const declaredColumnsByModelClass = new WeakMap();
 
 // The keys of `json` are column names, so map the properties with a ':' that the

tests/integration/misc/fieldExpressionKeys.js

@@ -255,6 +255,40 @@ export default (session) => {
         expect(item.meta).toEqual({ size: 1, color: 'red' });
       });
 
+      it('patch() should write an undeclared key with a colon to the json path when built before', async () => {
+        const query = Item.query().patch({ 'meta:color': 'red' }).where('id', 1);
+        query.toKnexQuery();
+        query.clone().toKnexQuery();
+        expect(await query).toBe(1);
+        const item = await Item.query().findById(1);
+        expect(item.meta).toEqual({ size: 1, color: 'red' });
+      });
+
+      it('upsertGraph() should not return the undeclared keys with a colon', async () => {
+        const item = await Item.query().upsertGraph({ id: 1, 'meta:color': 'red' });
+        expect(item).not.toHaveProperty('meta:color');
+      });
+
+      it('patchAndFetchById() should not return the undeclared keys with a colon', async () => {
+        const item = await Item.query().patchAndFetchById(1, { 'meta:color': 'red' });
+        expect(item).not.toHaveProperty('meta:color');
+        expect(item.meta).toEqual({ size: 1, color: 'red' });
+      });
+
+      it('$query().patch() should not set the keys with a colon mapped by snakeCaseMappers', async () => {
+        class SnakeCaseItem extends Item {
+          static get columnNameMappers() {
+            return snakeCaseMappers();
+          }
+        }
+
+        const item = await SnakeCaseItem.query().findById(1);
+        await item.$query().patch({ 'extraMeta:color': 'red' });
+        expect(item).not.toHaveProperty('extraMeta:color');
+        const row = await knex('field_expression_keys').where('id', 1).first();
+        expect(parseJson(row.extra_meta)).toEqual({ size: 1, color: 'red' });
+      });
+
       it('patch() should pick an undeclared key with a colon by its json column with pickJsonSchemaProperties', async () => {
         class PickItem extends Item {
           static get pickJsonSchemaProperties() {
@@ -296,6 +330,17 @@ export default (session) => {
         expect(item.meta).toEqual({ size: 1, color: 'red' });
       });
 
+      it('merge() should write an undeclared key with a colon to the json path when built before', async () => {
+        const query = Item.query()
+          .insert({ id: 1 })
+          .onConflict('id')
+          .merge({ 'meta:color': 'red' });
+        query.toKnexQuery();
+        await query;
+        const item = await Item.query().findById(1);
+        expect(item.meta).toEqual({ size: 1, color: 'red' });
+      });
+
       // knex doesn't support aliases in inserts on MySQL.
       it.skipIf(session.isMySql())(
         'merge() should write a key with a colon to the json path of an aliased table',