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 a footgun in before-update #196

Merged

Conversation

johnswanson
Copy link
Contributor

@johnswanson johnswanson commented Nov 21, 2024

In a very precise condition, we would emit an UPDATE clause without
any WHERE condition at all:

  • the before-update method's result does not have a primary key

  • the where clause in an update! call matches multiple rows

  • some part of the update map will change all matched rows

  • another part of the update map will change only a subset of matched
    rows

If all these conditions are met, the bug is triggered and we emit an
UPDATE without any WHERE clause at all.

This fixes the issue by making sure to get the primary keys from the
realized row rather than from the result of
(before-update (merge realized-row changes)). I also added tests as
well as an assertion that a primary key is actually present, since its
absence is so dangerous.

@johnswanson johnswanson requested a review from camsaul as a code owner November 21, 2024 22:13
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.56%. Comparing base (edee4e0) to head (118c6aa).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #196      +/-   ##
==========================================
- Coverage   83.60%   83.56%   -0.04%     
==========================================
  Files          37       37              
  Lines        2501     2501              
  Branches      212      213       +1     
==========================================
- Hits         2091     2090       -1     
  Misses        198      198              
- Partials      212      213       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@johnswanson johnswanson force-pushed the jds/remove-footgun-in-before-update branch from e273d39 to 2103be5 Compare November 22, 2024 12:27
In a very precise condition, we would emit an `UPDATE` clause without
any `WHERE` condition at all:

- the `before-update` method's result does not have a primary key

- the `where` clause in an `update!` call matches multiple rows

- some part of the update map will change all matched rows

- another part of the update map will change only a subset of matched
rows

If all these conditions are met, the bug is triggered and we emit an
UPDATE without any `WHERE` clause at all.

This fixes the issue by making sure to get the primary keys from the
realized row rather than from the result of
`(before-update (merge realized-row changes))`. I also added tests as
well as an assertion that a primary key is actually present, since its
absence is so dangerous.
Copy link
Owner

@camsaul camsaul left a comment

Choose a reason for hiding this comment

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

This seems like it's changing a lot of behavior, the old code realized the row and then used that as row everywhere, the new code keeps realized-row separately but doesn't thread that thru to everything else like it did before.

It probably doesn't make a difference since under the hood the underlying transient Row object should know it was already realized elsewhere and return the realized version but I think by making the change I suggested it would make the code a little clearer and eliminate a little bit of overhead

@johnswanson
Copy link
Contributor Author

This seems like it's changing a lot of behavior, the old code realized the row and then used that as row everywhere, the new code keeps realized-row separately but doesn't thread that thru to everything else like it did before.

It probably doesn't make a difference since under the hood the underlying transient Row object should know it was already realized elsewhere and return the realized version but I think by making the change I suggested it would make the code a little clearer and eliminate a little bit of overhead

Doh, yeah - sorry about that!

This test passes for me on MariADB 11.3 but fails on MariaDB 11.4, looks
like they just changed the error message slightly.
@johnswanson
Copy link
Contributor Author

@camsaul it looks like MariaDB has changed the error format from

Unknown column 'venue_name' in 'SET'

vs

Unknown column 'venue_name' in 'field list'

in the test case between 11.3 and 11.4 - locally things pass if I run mariadb:11.3 but fail if I run mariadb:11.4. Just pushed a commit that should fix that.

@johnswanson johnswanson requested a review from camsaul November 26, 2024 14:48
@camsaul camsaul merged commit ea7fe29 into camsaul:master Nov 26, 2024
8 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