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

fix: nothing should be returned when using on conflict do nothing (#353) #366

Merged
merged 1 commit into from
Nov 20, 2023
Merged

fix: nothing should be returned when using on conflict do nothing (#353) #366

merged 1 commit into from
Nov 20, 2023

Conversation

nirweiner2
Copy link
Contributor

@nirweiner2 nirweiner2 commented Nov 7, 2023

when we use insert .... on conflict do nothing, and there is a conflict, nothing should be returned because no row was inserted. Fix the code accordingly.
See issue #353

@nirweiner2
Copy link
Contributor Author

Hey @oguimbal, I would love to get it reviewed if possible.

Copy link
Owner

@oguimbal oguimbal left a comment

Choose a reason for hiding this comment

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

Hi !

Thanks for the PR, and sorry for the delay.

It seems that you're right on your unit test modification, but I believe there is a mistake in the implementation ?

(almost all tests fail with our version... they pass with the below modification)

src/execution/records-mutations/insert.ts Outdated Show resolved Hide resolved
@nirweiner2
Copy link
Contributor Author

nirweiner2 commented Nov 20, 2023

Hey @oguimbal.
Thank you very much for reviewing this.
I have indeed made a mistake there, it got actually lost during the commit phase in git.
I forced pushed the fix and hopefully it should be okay now.

@oguimbal oguimbal merged commit 714a18f into oguimbal:master Nov 20, 2023
3 checks passed
@oguimbal
Copy link
Owner

Released as [email protected]

Thanks for spotting that !

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

Successfully merging this pull request may close these issues.

2 participants