Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doesn't work with custom primary keys? #89

Closed
SAGV opened this issue Jan 23, 2017 · 7 comments
Closed

Doesn't work with custom primary keys? #89

SAGV opened this issue Jan 23, 2017 · 7 comments
Assignees

Comments

@SAGV
Copy link

SAGV commented Jan 23, 2017

I've just started with Adonis but i've faced a strange issue.

So the docs state that:

Each model needs to have a primary key which is set to id by default. Value for primary key is auto-populated by Lucid whenever you save a new model to the database. Also, the primary key is required to resolve model relations.

Does it mean that I can't create my own primary keys and fill the keys myself?

For example, I have a Lock model:

'use strict'

const Lucid = use('Lucid')

class Lock extends Lucid {

  static get primaryKey () {
    return 'bluetooth_id'
  }
}

module.exports = Lock

Where I want bluetooth_id to be the primary key.

Then I made a migration:

'use strict'

const Schema = use('Schema')

class LocksTableSchema extends Schema {

  up () {
    this.create('locks', (table) => {
      table.string('bluetooth_id').unique().primary()
      table.timestamps()
    })
  }

  down () {
    this.drop('locks')
  }

}

module.exports = LocksTableSchema

And a simple blueprint:

Factory.blueprint('App/Model/Lock', (fake) => {
  return {
    bluetooth_id: fake.string({length: 20, pool: '0123456789abcdef'})
  }
})

All of it is also in the DatabaseSeeder class:


const Factory = use('Factory')

class DatabaseSeeder {

  * run () {
    yield Factory.model('App/Model/Lock').create(5)
  }

}

module.exports = DatabaseSeeder

And the result is:
2017-01-23_14-44-35

But apparently, Lucid fails to insert rows when bluetooth_id is filled by me (or not autoincremented or has a type of string)

Even if it states seeded database successfully, actually, zero lines are inserted. Migration does make the correct schema (which I can easily fill using my own SQL-queries) but the table stays empty, no rows are inserted when either seed or just a simple create is called. And no errors produced.

Upd: Here is the repo. Feel free to clone and run. You can also POST /locks with a json containing bluetooth_id so see that Lock.create also fails

Could you give me a hand with it?

@SAGV SAGV changed the title Doesn't work with custom primary keys?! Doesn't work with custom primary keys? Jan 23, 2017
@thetutlage
Copy link
Member

So it is more of a feature than a bug and here's why

ORM's need a way to track which model is new and which model already exists in the database. Lucid believes that the primary key will be auto incrementing within the database and once the insert query is done, it will use the return value as the value of that primary key.

In your case the primary key is not auto-incrementing and instead set by you. So according to Lucid this row already exists in the database.

Action Plan

Instead of relying on the primary key value, I will make use of a flag to determine same.

@SAGV
Copy link
Author

SAGV commented Jan 23, 2017

@thetutlage thank you for the quick feedback!

Yeah, I saw you're also using primary key to determine, for example, whether the row has been put to database or not

  /**
   * tells whether a model has been persisted to the
   * database or not.
   *
   * @return {Boolean}
   *
   * @public
   */
  isNew () {
    return !this.$primaryKeyValue
  }

I will make use of a flag to determine same.

What you mean by that?

@hajnalben
Copy link

A tip: you can use the Lucid Model's insert() method instead of save() to force insertion with custom id-s. I had the same issue and it solved it for me.

@thetutlage
Copy link
Member

thetutlage commented Jan 23, 2017

You can try this by installing adonis-lucid from github and lemme know if everything works fine. Also you have to set a flag on your model as defined below.

class Lock extends Lucid {

  static get primaryKey () {
    return 'bluetooth_id'
  }

  static get incrementing () {
    return false
  }

}

I'll wait for a day or two to release it to npm.

@SAGV
Copy link
Author

SAGV commented Jan 24, 2017

@thetutlage yes, this solution works! Thank you!

@hajnalben
Copy link

The solution works but I have found a related problem. The exists flag may be set upon update too. Now it defaults to false.

@thetutlage
Copy link
Member

Before updating the model whether you will fetch it from database or you will create a new instance and persist it to database and both of these operations will set the exists property to true

thetutlage added a commit that referenced this issue Jul 16, 2017
when primaryKey is not incrementing, one needs to define it as an explicit flag. Which will aware

lucid to setup correct value for primaryKey and make everything work as expected

Closes #89
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants