Skip to content

Commit

Permalink
improvement: raise error when trying to merge or fill extra propertie…
Browse files Browse the repository at this point in the history
…s to model
  • Loading branch information
thetutlage committed Apr 18, 2020
1 parent ebee5fb commit 1167b54
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 29 deletions.
4 changes: 2 additions & 2 deletions adonis-typings/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -451,8 +451,8 @@ declare module '@ioc:Adonis/Lucid/Model' {
$consumeAdapterResult (adapterResult: ModelObject, sideloadAttributes?: ModelObject): void
$hydrateOriginals(): void

fill (value: Partial<ModelAttributes<this>>, ignoreUndefined?: boolean): void
merge (value: Partial<ModelAttributes<this>>, ignoreUndefined?: boolean): void
fill (value: Partial<ModelAttributes<this>>, allowNonExtraProperties?: boolean): void
merge (value: Partial<ModelAttributes<this>>, allowNonExtraProperties?: boolean): void
save (): Promise<void>
delete (): Promise<void>
refresh (): Promise<void>
Expand Down
50 changes: 30 additions & 20 deletions src/Orm/BaseModel/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
}
Expand All @@ -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')
}
Expand All @@ -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')
}
Expand Down Expand Up @@ -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
}

Expand All @@ -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

/**
Expand All @@ -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()`
Expand All @@ -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
})
}
}
Expand Down
28 changes: 21 additions & 7 deletions test/orm/base-model.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 })
})
Expand All @@ -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' })
})

Expand All @@ -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
Expand All @@ -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) => {
Expand Down

0 comments on commit 1167b54

Please sign in to comment.