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

upsert support #13

Open
geekflyer opened this issue Jan 29, 2018 · 7 comments
Open

upsert support #13

geekflyer opened this issue Jan 29, 2018 · 7 comments

Comments

@geekflyer
Copy link

Hi I'm looking into upsert support for pogi.

API wise it could be either a seperate method like upsert and upsertAndGet or just an additional parameter on the existing insert and insertAndGet methods.

Postgres 9.5+ has native upsert support via the ON CONFLICT DO UPDATE clause which can be appended to inserts, see: https://www.postgresql.org/docs/9.5/static/sql-insert.html

What do you think of adding support for upsert into pogi?

@holdfenytolvaj
Copy link
Owner

You are right we should add this, however cannot give a deadline for this now... :/

@holdfenytolvaj
Copy link
Owner

Sorry for the delay,

await users.upsert({username: 'bob', password:'123'}, ['username']);           //using column name
await users.upsert({username: 'bob', password:'123'}, "users_name_key"); //using constraint name
await users.upsert({id:1, username: 'bob', password:'123'});                        //default is the primary key

would work for you?

@geekflyer
Copy link
Author

geekflyer commented Jun 3, 2019

Looks, good, but I don't like the api in the second example. I would rather make the "constraint" variant a bit more explicit and pass in the constraint name as an options object, i.e.:

await users.upsert({username: 'bob', password:'123'}, {constraint: "users_name_key"});

or

await users.upsert({username: 'bob', password:'123'}, {uniqueConstraint: "users_name_key"});

Just makes this a bit more explicit and distinguishable from the column names variant.

@holdfenytolvaj
Copy link
Owner

I've committed a new version, it still uses the original suggestion but it can change of course (before we add it to the documentation).

I like the explicit things, however in this case is it needed? It looks like just makes the line longer.
And in that case the columns should be part of the option as well, like

await users.upsert({username: 'bob', password:'123'}, {columns: ["name"]});

the benefit would be however that we could add the
"where"
and
custom "set" options as well if it is an object.

@holdfenytolvaj
Copy link
Owner

Changed to the recommendation. Thanks a lot!
thus it is:

await users.upsert({username: 'bob', password:'123'}, {constraint: "users_name_key"});
await users.upsert({username: 'bob', password:'123'}, {columns: ["name"]});
await users.upsert({username: 'bob', password:'123'}); //using primary key

@geekflyer
Copy link
Author

geekflyer commented Jun 3, 2019

I don't think it makes the line that much longer, especially if it's just constraint.
My concern is mainly about the readability of the code: when another engineer reads the code line and is not entirely familiar with the pogi api he might just wondering what that last parameter is and to what it refers. In case of columns that is relatively obvious because it's an array and it refers to column names that most folks are familiar with, even if they're not familiar with pogi. However in case of constraints that is not obvious. I definitely don't remember or know most of the constraints names we have defined on our tables out of my head, neither do the other engineers, which means an engineer which is neither super familiar with pogi, nor somehow remembers all the unique constraint names will do a lot of head scratching before he figures out what that last parameter refers to.

@holdfenytolvaj
Copy link
Owner

Absolutely make sense.

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

2 participants