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

[WIP] Returns the newly created object #22

Closed
wants to merge 1 commit into from
Closed

[WIP] Returns the newly created object #22

wants to merge 1 commit into from

Conversation

jonataswalker
Copy link

Discussion in #21

Verified

This commit was signed with the committer’s verified signature.
rithvikvibhu Rithvik Vibhu
if (util.isNumber(res)) return res
return res ? res.length : 0
})
if (name && /^insert/i.test(query.toString())) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than use regex on the string, knex queries have a _method property that says what it is. I use it here.

Copy link
Author

@jonataswalker jonataswalker Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange query._method logs always undefined. Both clients. @citycide I need some help here.

Copy link
Author

@jonataswalker jonataswalker Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I get what I need with instance.knex.queryBuilder()._method. Both clients. Is that ok?

Too fast. That's not true. Stuck here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pfft, bummer, it's not there for inserts. Really wish knex would be consistent about that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we stay with regex?

if (util.isNumber(res)) return res
return res ? res.length : 0
})
}
}

return instance.pool.acquire().then(db => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the non-native sql.js backend gets handled. You're only handling the isNative case right now, but trilogy has to maintain parity between both.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's not ready yet.

return helpers.runQuery({
query,
instance: this.ctx,
name: this.name,
Copy link
Owner

@haltcase haltcase Feb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of adding this just to be used in a single location, especially when I'm sure the table name could be pulled out of the knex query object when needed. We'll have to check what properties are on the query object though but I'm pretty sure there's one for the table being operated on.

edit: looks like you can get the table name as query._single.table

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, great. I'll change back to the original.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also stuck here. query._single is undefined.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my other comment, knex unfortunately is not consistent about this so it's not there for inserts...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to proceed then?

export function findLastObject (instance, table) {
return new Promise((resolve, reject) => {
let queryInfo = `PRAGMA table_info("${table}")`
let queryId = `SELECT seq FROM sqlite_sequence WHERE name="${table}"`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you already saw it but just in case, ref. #21 (comment).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but you know what? (I guess you're referring that I'm not using option 2 (select last_insert_rowid())) I realized that's not possible to know what is the column name to use with findOne(). So I think we should give up if option 1 fails. Or what would be the search parameters?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the columns could be the parameters - we just need to return one that meets all of them. If there's not a unique or primary key that's the best we can do.

The only thing we really need to go out of our way here for I think is autoincrements, because we don't know what that is until it's been inserted.

let queryInfo = `PRAGMA table_info("${table}")`
let queryId = `SELECT seq FROM sqlite_sequence WHERE name="${table}"`

instance.knex.raw(queryInfo).then(info => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing queries directly like this only works when isNative.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. I'll try db.exec() when !isNative then.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or use runQuery() which would work for both.

@haltcase
Copy link
Owner

haltcase commented Feb 2, 2017

I may have solved this using a similar but simpler take on it... I'll push up a separate PR tonight so we can both take a look.

@haltcase
Copy link
Owner

haltcase commented Feb 3, 2017

@jonataswalker check out #24 and let me know any thoughts.

@haltcase
Copy link
Owner

haltcase commented Feb 3, 2017

Closing in favor of #24 which is merged and pending release. Thanks again for your efforts here!

@haltcase haltcase closed this Feb 3, 2017
@jonataswalker jonataswalker deleted the return-doc branch February 3, 2017 19:31
@haltcase haltcase added the type: feature Request for a new feature or enhancement. label Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Request for a new feature or enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants