diff --git a/doc/release-notes/changelog.md b/doc/release-notes/changelog.md index c92b48ffb..c736e7d6f 100644 --- a/doc/release-notes/changelog.md +++ b/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 diff --git a/lib/queryBuilder/operations/MergeOperation.js b/lib/queryBuilder/operations/MergeOperation.js index cac841baf..7ab13c098 100644 --- a/lib/queryBuilder/operations/MergeOperation.js +++ b/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); } diff --git a/lib/queryBuilder/operations/UpdateOperation.js b/lib/queryBuilder/operations/UpdateOperation.js index 9fbb98f6e..8cd8849f1 100644 --- a/lib/queryBuilder/operations/UpdateOperation.js +++ b/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; } diff --git a/lib/queryBuilder/operations/fieldExpressions.js b/lib/queryBuilder/operations/fieldExpressions.js index c20f53a2a..1f4eaaad8 100644 --- a/lib/queryBuilder/operations/fieldExpressions.js +++ b/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 diff --git a/tests/integration/misc/fieldExpressionKeys.js b/tests/integration/misc/fieldExpressionKeys.js index 1f80f896a..d8c2be24f 100644 --- a/tests/integration/misc/fieldExpressionKeys.js +++ b/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',