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

using with upsert #88

Closed
northcoders-chris opened this issue Jan 26, 2015 · 13 comments
Closed

using with upsert #88

northcoders-chris opened this issue Jan 26, 2015 · 13 comments

Comments

@northcoders-chris
Copy link

I am using upsert for adding documents and updating them. I got an error if I used before.upsert with the package, is it possible to do this?

@matb33
Copy link
Collaborator

matb33 commented Jan 28, 2015

I haven't yet implemented support for upsert. No good reason other than me not using upsert in a project yet... but I definitely will implement it for collection-hooks. This serves as a good reminder :)

@fusepilot
Copy link

Upsert is really useful, so this would be great to have.

@jonjamz
Copy link
Contributor

jonjamz commented Feb 17, 2015

+1

1 similar comment
@sgtpepper43
Copy link

+1

@faceyspacey
Copy link

+10

@matb33
Copy link
Collaborator

matb33 commented Jun 19, 2015

I have upsert done but need to write more tests! If anyone wanted to help me with the last mile, it's a great way to get your name into this package! It's all in the upsert branch

@dmitrijs-balcers
Copy link

+1

@dmitrijs-balcers
Copy link

@matb33 Can you specify the set of tests that should be written, so the branch would be merge? I really need hooks for upsert, for the project I'm working in.

@mizzao
Copy link
Contributor

mizzao commented Aug 12, 2015

I'm confused. Does upsert currently not use any hooks, or does it just use the update and insert hooks in some funny way?

#16 suggests that update would just be called with {upsert: true} and the update hooks used. So what is the missing functionality?

I'm also using upsert, so would be happy to write these tests if @matb33 could clarify exactly what it is we're fixing :)

@rclai
Copy link
Contributor

rclai commented Aug 12, 2015

Pretty much upserts only trigger update hooks and not insert hooks.

@mizzao
Copy link
Contributor

mizzao commented Aug 12, 2015

b69bfda suggests that a before.upsert hook will always be fired, and an after.update OR after.insert hook fired afterward depending on the outcome. Is that what you would expect to happen?

Is it ever possible to have a {multi: true} kind of thing for upsert operations, and if so how would we deal with that?

@rclai
Copy link
Contributor

rclai commented Aug 12, 2015

The introduction of explicit upsert hooks kind of makes things more complicated.

I was expecting the implementation to trigger only the before.update hook and then after.update or after.insert depending on the outcome.

@matb33
Copy link
Collaborator

matb33 commented Aug 12, 2015

When I designed the upsert hooks, it only made sense to me that before.upsert would be called as opposed to before.update -- the operation isn't necessarily going to be an update so having the hook trigger before.update seemed unintuitive. The collection API exposes an upsert method after all, and the end-user shouldn't have to care how that implementation works, i.e. update with {upsert: true}.

That's why I introduced a before.upsert, as that's as clear as it gets.

As far as tests go, I don't quite recall what I felt was missing. Also I think the docs need to be updated for upsert.

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

9 participants