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 method doesn't trigger any hooks for mysql adapter #461

Open
bahung1221 opened this issue May 7, 2019 · 3 comments
Open

upsert method doesn't trigger any hooks for mysql adapter #461

bahung1221 opened this issue May 7, 2019 · 3 comments

Comments

@bahung1221
Copy link
Contributor

bahung1221 commented May 7, 2019

Hi,

I found that the upsert method won't trigger any hook if the adapter support updateOrCreate method (mysql in my case). I debugged and found the reason in below code:

if (this.schema.adapter.updateOrCreate) {

if (this.schema.adapter.updateOrCreate) {
        const inst = new Model(data);
        this.schema.callAdapter('updateOrCreate', Model.modelName, stripUndefined(inst.toObject(true)), (err, data) => {
            if (data) {
                return inst.reload(callback);
            }
            callback(err, null);
        });
    } else {
    ...
}

I think it should trigger save/update hooks like other methods,
If you need, i can create a PR for it,

Thank you very much for a nice job!

@1602
Copy link
Owner

1602 commented May 7, 2019

Yep, that seems like a missing call, I'd appreciate PR, thank you!

bahung1221 added a commit to bahung1221/jugglingdb that referenced this issue May 7, 2019
@bahung1221
Copy link
Contributor Author

Hi,

I fixed it by make it work same behavior for all adapter,
It will make the workflow of upsert (createOrUpdate) method more consistency.
I think it is the best way because if we call updateOrCreate method of adapter, we can't know that it was create or update.
Pull request: #462

Thank you

1602 added a commit that referenced this issue May 8, 2019
#461 Call 'find' and then 'update/create' instead call 'updateOrCreat…
@1602
Copy link
Owner

1602 commented May 8, 2019

Merged. One test is failing, though. I'll try to find some time to investigate why.

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