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

Deprecate upsert? #796

Closed
MaxGabriel opened this issue Mar 8, 2018 · 7 comments
Closed

Deprecate upsert? #796

MaxGabriel opened this issue Mar 8, 2018 · 7 comments

Comments

@MaxGabriel
Copy link
Member

MaxGabriel commented Mar 8, 2018

The upsert function inserts or updates an existing record. It works fine if there's one uniqueness constraint, but if there are two it doesn't know which to use, and throws an exception. This is pretty lame: you wouldn't think that adding a new uniqueness constraint would cause a runtime exception in a related area of your code. Users should just use upsertBy, or if their database supports a way of not specifying it, use that functionality (e.g. the MySQL module exposes insertOnDuplicateKeyUpdate; I think Postgres requires specifying the column(s)).

This would resolve issues like #768 and avoid all the documentation of this caveat in #785.

@psibi
Copy link
Member

psibi commented Mar 8, 2018

In-spite of these caveats, I would still like to use upsert and don't want it to be deprecated.

It works fine if there's one uniqueness constraint, but if there are two it doesn't know which to use, and throws an exception.

I would like an API in which we can explicitly specify which uniqueness column it should select.

@MaxGabriel
Copy link
Member Author

I would like an API in which we can explicitly specify which uniqueness column it should select.

Sorry should have clarified: we already have that, it’s upsertBy. We should just direct everyone to use that

@psibi
Copy link
Member

psibi commented Mar 8, 2018

In that case, I'm fine with it being deprecated. Never actually used upsertBy and I always thought it had the same caveat as upsert. 👍

@parsonsmatt
Copy link
Collaborator

We could keep upsert on the case that there's a single primary key with a bit of type magic. Not sure if it's worth the hassle.

@pythonissam
Copy link

@MaxGabriel @psibi
How's the status of this issue? Sadly enough, I haven't received any response with my PR for this two months...

@bitemyapp
Copy link
Contributor

bitemyapp commented Jun 17, 2018

@pythonissam I'd lean toward deprecating upset. I think I know the kind of design @parsonsmatt alluded to and I'm not a fan. I've been working with some 9-5 code lately that uses Groundhog's equivalent to an implicit upset and it has not made my life easier or simpler.

I'm going to comment separately on your repsert PR.

@pythonissam
Copy link

pythonissam commented Jun 23, 2018

@bitemyapp

I'd lean toward deprecating upset.

All right. But I think there still exists bugs around these functions. I'm going to organize my thoughts and re-submit issues separately.

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

5 participants