From 1167b54f9c362195e9a9f0dde74524514cb6b534 Mon Sep 17 00:00:00 2001 From: Harminder virk Date: Sat, 18 Apr 2020 12:14:32 +0530 Subject: [PATCH] improvement: raise error when trying to merge or fill extra properties to model --- adonis-typings/model.ts | 4 +-- src/Orm/BaseModel/index.ts | 50 ++++++++++++++++++++++--------------- test/orm/base-model.spec.ts | 28 +++++++++++++++------ 3 files changed, 53 insertions(+), 29 deletions(-) diff --git a/adonis-typings/model.ts b/adonis-typings/model.ts index 66435205..26520e54 100644 --- a/adonis-typings/model.ts +++ b/adonis-typings/model.ts @@ -451,8 +451,8 @@ declare module '@ioc:Adonis/Lucid/Model' { $consumeAdapterResult (adapterResult: ModelObject, sideloadAttributes?: ModelObject): void $hydrateOriginals(): void - fill (value: Partial>, ignoreUndefined?: boolean): void - merge (value: Partial>, ignoreUndefined?: boolean): void + fill (value: Partial>, allowNonExtraProperties?: boolean): void + merge (value: Partial>, allowNonExtraProperties?: boolean): void save (): Promise delete (): Promise refresh (): Promise diff --git a/src/Orm/BaseModel/index.ts b/src/Orm/BaseModel/index.ts index 45d5dfab..892a116a 100644 --- a/src/Orm/BaseModel/index.ts +++ b/src/Orm/BaseModel/index.ts @@ -468,10 +468,7 @@ export class BaseModel implements LucidRow { /** * Find model instance using a key/value pair */ - public static async find ( - value: any, - options?: ModelAdapterOptions, - ) { + public static async find (value: any, options?: ModelAdapterOptions) { if (value === undefined) { throw new Exception('"find" expects a value. Received undefined') } @@ -482,10 +479,7 @@ export class BaseModel implements LucidRow { /** * Find model instance using a key/value pair */ - public static async findOrFail ( - value: any, - options?: ModelAdapterOptions, - ) { + public static async findOrFail (value: any, options?: ModelAdapterOptions) { if (value === undefined) { throw new Exception('"findOrFail" expects a value. Received undefined') } @@ -509,10 +503,7 @@ export class BaseModel implements LucidRow { /** * Find model instance using a key/value pair */ - public static async findMany ( - value: any[], - options?: ModelAdapterOptions, - ) { + public static async findMany (value: any[], options?: ModelAdapterOptions) { if (value === undefined) { throw new Exception('"findMany" expects a value. Received undefined') } @@ -1230,9 +1221,9 @@ export class BaseModel implements LucidRow { * fill isn't allowed, since we disallow setting relationships * locally */ - public fill (values: any, ignoreUndefined: boolean = true) { + public fill (values: any, allowNonExtraProperties: boolean = false) { this.$attributes = {} - this.merge(values, ignoreUndefined) + this.merge(values, allowNonExtraProperties) this.fillInvoked = true } @@ -1242,7 +1233,7 @@ export class BaseModel implements LucidRow { * 1. If key is unknown, it will be added to the `extras` object. * 2. If key is defined as a relationship, it will be ignored and one must call `$setRelated`. */ - public merge (values: any, ignoreUndefined: boolean = true) { + public merge (values: any, allowNonExtraProperties: boolean = false) { const Model = this.constructor as typeof BaseModel /** @@ -1251,15 +1242,25 @@ export class BaseModel implements LucidRow { if (isObject(values)) { Object.keys(values).forEach((key) => { const value = values[key] - if (ignoreUndefined && value === undefined) { - return - } + /** + * Set as column + */ if (Model.$hasColumn(key)) { - this[key] = values[key] + this[key] = value return } + /** + * Resolve the attribute name from the column names. Since people + * usaully define the column names directly as well by + * accepting them directly from the API. + */ + const attributeName = Model.$keys.columnsToAttributes.get(key) + if (attributeName) { + this[attributeName] = value + } + /** * If key is defined as a relation, then ignore it, since one * must pass a qualified model to `this.$setRelated()` @@ -1268,7 +1269,16 @@ export class BaseModel implements LucidRow { return } - this.$extras[key] = values[key] + /** + * Raise error when not instructed to ignore non-existing properties. + */ + if (!allowNonExtraProperties) { + throw new Error( + `Cannot define "${key}" on "${Model.name}" model, since it is not defined as a model property`, + ) + } + + this.$extras[key] = value }) } } diff --git a/test/orm/base-model.spec.ts b/test/orm/base-model.spec.ts index 0bea808c..63d9be04 100644 --- a/test/orm/base-model.spec.ts +++ b/test/orm/base-model.spec.ts @@ -1516,18 +1516,32 @@ test.group('BaseModel | fill/merge', (group) => { } const user = new User() - user.fill({ username: 'virk', isAdmin: true } as any) + user.fill({ username: 'virk' }) assert.deepEqual(user.$attributes, { username: 'virk' }) }) - test('set extra properties via fill', (assert) => { + test('raise error when extra properties are defined', (assert) => { + class User extends BaseModel { + @column() + public username: string + } + + const user = new User() + const fn = () => user.fill({ username: 'virk', isAdmin: true } as any) + assert.throw( + fn, + 'Cannot define "isAdmin" on "User" model, since it is not defined as a model property', + ) + }) + + test('set extra properties via fill when allowExtraProperties is true', (assert) => { class User extends BaseModel { @column() public username: string } const user = new User() - user.fill({ username: 'virk', isAdmin: true } as any) + user.fill({ username: 'virk', isAdmin: true } as any, true) assert.deepEqual(user.$attributes, { username: 'virk' }) assert.deepEqual(user.$extras, { isAdmin: true }) }) @@ -1545,7 +1559,7 @@ test.group('BaseModel | fill/merge', (group) => { user.age = 22 assert.deepEqual(user.$attributes, { age: 22 }) - user.fill({ username: 'virk', isAdmin: true } as any) + user.fill({ username: 'virk' }) assert.deepEqual(user.$attributes, { username: 'virk' }) }) @@ -1562,11 +1576,11 @@ test.group('BaseModel | fill/merge', (group) => { user.age = 22 assert.deepEqual(user.$attributes, { age: 22 }) - user.merge({ username: 'virk', isAdmin: true } as any) + user.merge({ username: 'virk' }) assert.deepEqual(user.$attributes, { username: 'virk', age: 22 }) }) - test('do not set properties with explicit undefined values', (assert) => { + test('set properties with explicit undefined values', (assert) => { class User extends BaseModel { @column() public username: string @@ -1580,7 +1594,7 @@ test.group('BaseModel | fill/merge', (group) => { assert.deepEqual(user.$attributes, { age: 22 }) user.merge({ username: 'virk', age: undefined }) - assert.deepEqual(user.$attributes, { username: 'virk', age: 22 }) + assert.deepEqual(user.$attributes, { username: 'virk', age: undefined }) }) test('invoke setter when using fill', (assert) => {