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

model.create should/could needResponse #21

Closed
jonataswalker opened this issue Jan 19, 2017 · 13 comments
Closed

model.create should/could needResponse #21

jonataswalker opened this issue Jan 19, 2017 · 13 comments
Labels
breaking Breaking changes will occur as part of closing the issue. type: feature Request for a new feature or enhancement.

Comments

@jonataswalker
Copy link

jonataswalker commented Jan 19, 2017

Right after creating a new object it would be helpful if created object returns.

Much better if model.create use returning('id'), otherwise how we can get last inserted record?

@haltcase haltcase added breaking Breaking changes will occur as part of closing the issue. discussion [RFC] Fixes, APIs, or changes need feedback. labels Jan 20, 2017
@haltcase
Copy link
Owner

haltcase commented Jan 20, 2017

You're probably right that it should return the created object... I checked and mongoose, nedb, & linvodb all return the newly created object, so it would make sense for Trilogy to do the same if we want to follow somewhat similar semantics.

One thing to note though is that sqlite doesn't support returning - as far as I know that's only a postgres thing. So the trouble here comes because there's no concept of an id in the true document store sense. Trilogy still uses sqlite where clauses, so the findOne to return your created object would basically use your creation object as its criteria.

So then we can ask... is that sufficient? Maybe there's a better way of grabbing the last inserted object. edit: or if nothing else, Trilogy could add an __id__ field on creation and drop it on return.

Also if we do change this, it would be a breaking change. I'll leave it up for discussion for now on whether it's warranted in this case - I mean, it's an RC but if we're going to hit 1.0 we should hit it with a stable API. I'd rather not jump to 2.0 next week 😄

@jonataswalker
Copy link
Author

Playing/hacking a bit I got the last inserted id with:

// this is inside runQuery()
db.exec("select last_insert_rowid();")

@haltcase
Copy link
Owner

haltcase commented Jan 20, 2017

I'm not able to test right now, does that work with both sqlite & sql.js backends? I'd assume it does since it's a sqlite function but we need to maintain parity between the two.

@jonataswalker
Copy link
Author

Wow, what a pain to install this thing (sqlite3). Giving up for now.

@haltcase
Copy link
Owner

😆 and that my friends is why Trilogy exists. Thanks for trying! I'll get at it too when I have the time

@jonataswalker
Copy link
Author

Finally the light came and I get this beast working.

Yeah, select last_insert_rowid() also works with sqlite3. But I've found this answer and tested it:

instance.knex.raw('select seq from sqlite_sequence where name="some_table"')

Both — sql.js and sqlite3 — work well.

@jonataswalker
Copy link
Author

jonataswalker commented Jan 20, 2017

I can get the newly object created with this:

// with sql.js -- hacking inside runQuery()
const created = parseResponse(db.exec('select seq from sqlite_sequence where name="some_table"'));
const info = parseResponse(db.exec('PRAGMA table_info("some_table")'));

// find the primary key name
let key;
info.some(each => {
  if (each.pk && each.notnull && each.type === 'integer') {
    key = each.name;
    return true;
  }
});

const obj_ = {};
obj_[key] = created[0].seq;
const doc = instance.findOne(name, obj_);

doc.then(r => console.log(r));

Do you think this can turn into a PR, obviously, after some polish?

@haltcase
Copy link
Owner

haltcase commented Jan 20, 2017

Does that only work for autoincrement keys? I think whatever we go with should be universal, so if this fits then I'd definitely appreciate a PR.

edit: Yep, looks like this is autoincrement only.

There is a single row in the sqlite_sequence table for each ordinary table that uses AUTOINCREMENT.

If create is going to return the last created object, it should always do that - this approach would only work for models with an autoincrementing PK.

Based on what I'm finding there, I think your first solution is the one we should chase. Thoughts?

@jonataswalker
Copy link
Author

@citycide I'm in a tight deadline and will return as soon as possible.

@jonataswalker
Copy link
Author

So @citycide let's try to finish this one. To sumarize a desired PR:

@haltcase
Copy link
Owner

haltcase commented Feb 1, 2017

@jonataswalker well since neither seem absolute, a hybrid option might be necessary.

In this order:

  1. If the table has an autoincrementing PK, use it with the 'select seq from sqlite_sequence where name="some_table"' method.

  2. If it doesn't, try "select last_insert_rowid();"

  3. If neither of the above work, make a best effort attempt to select the object using the provided arguments as criteria.

This adds a couple queries but covers as many cases as we can. If we want to avoid the extra queries, we could go back to the original behavior of simply using part 3 from above. It only doesn't work reliably when the schema doesn't have a unique or primary key - and most should.

Edited to say reliably because it would always select and return an object meeting those criteria.

@jonataswalker
Copy link
Author

PR #22 created, not sure if you'll like new changes.

I'm going to get back tomorrow to finish it.

@haltcase haltcase added status: pending release Issue is resolved but waiting to be released. and removed discussion [RFC] Fixes, APIs, or changes need feedback. labels Feb 4, 2017
@haltcase
Copy link
Owner

haltcase commented Feb 4, 2017

Released in v1.0.0.rc-3.

@haltcase haltcase closed this as completed Feb 4, 2017
@haltcase haltcase removed the status: pending release Issue is resolved but waiting to be released. label Feb 4, 2017
@haltcase haltcase added the type: feature Request for a new feature or enhancement. label Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes will occur as part of closing the issue. type: feature Request for a new feature or enhancement.
Projects
None yet
Development

No branches or pull requests

2 participants