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

Return all columns not in both insert and update columns when doing upsert #1306

Merged
merged 2 commits into from
Jan 9, 2024
Merged

Return all columns not in both insert and update columns when doing upsert #1306

merged 2 commits into from
Jan 9, 2024

Conversation

adsa95
Copy link
Contributor

@adsa95 adsa95 commented Sep 13, 2023

I would argue that a model should be completely up to date with the database after an upsert, however as demonstrated in #1296 this is currently not always the case. This PR is a proposed fix for this. By returning all columns not present in both the insert and update column sets we make sure that the model is always up to date with the database after an upsert, no matter if an insert or update was performed.

As updateColumns and insertColumns are both Columns structs that might have different types (infer, whitelist etc) I did not find it suitable to add a method on the Columns struct to calculate this and do it different depending on type. Instead opting to just always calculate the return columns the same way.

Although the biggest issue in my example in #1296 was that the ID (primary key) was not updated when generated in go code and not in the database, I still think all properties on the model should be updated to match the database when an upsert is performed. Hence not just adding a check to make sure primary keys are in the return set.

Happy to hear feedback on this!

Note: This PR also relies on the SetIntersect() method being added to the strmangle package. See volatiletech/strmangle#11

@stephenafamo
Copy link
Collaborator

The PR on strmangle has been merged, so you should update sqlboiler to depend on v0.0.6 of strmangle so that the user's version gets updated once they update sqlboiler and regenerate the models with it.

Once that is done, I'll be happy to merge this. Thank you.

@adsa95
Copy link
Contributor Author

adsa95 commented Jan 9, 2024

This should now be updated! Thanks for looking at this PR :)

@stephenafamo stephenafamo merged commit d24671e into volatiletech:master Jan 9, 2024
2 checks passed
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