Fix Column Order Bug #5

2019-06-12T04:41:09+02:00

tiger5226 commented

2019-06-12 04:41:09 +02:00

(Migrated from github.com)

Added objectID position tracking. When the template is generated the ordering of conflicting unique columns is alphabetical. This means we cannot assume the objectID can always be appended. It might need to be inserted at a specific position. This adds support for tracking the position and inserting it at the correct position so the delete query correctly deletes the conflicts.

tiger5226 commented

2019-06-12 04:55:23 +02:00

(Migrated from github.com)

@lyoshenka I was investigating an issue where @tzarebczan got an error on merging two users each with wallet table entries. The wallet table has a Many to 1 relationship with the user table, with uniqueness on user_id and version. I traced the code and apparently the templating used in SQLBoiler writes the conflicting unique columns in alphabetical order. This is problematic because the wallet table is the first table to have unique keys where the user_id is before the "other"(version) columns, and we append the user_id for the delete query which makes the arguments out of order. As a result the delete query does not delete the conflict because there is no match.

What I did to solve this was to keep track of the index of the object id based on the pre-compliment slice, then insert the value in the query argument slice where it is supposed to be rather than appending or prepending it. This should solve all future issues where there are more than 2 columns and the object id column could be anywhere in the array depending on the alphabetical order.

@tzarebczan We do not have to merge this right away as it is only impacts the wallet table rows conflict since this is the only table with the columns out of the assumed order. So we can fix the wallet table issue first, handle these merge errors manually in the meantime and once finished we can merge this to fix the issue. @nikooo777 We just have to be aware of this bug when we add tables with uniqueness columns. We just got lucky that this was the first table where the assumed order did not match the alphabetical order.

@lyoshenka I was investigating an issue where @tzarebczan got an error on merging two users each with wallet table entries. The `wallet` table has a Many to 1 relationship with the `user` table, with uniqueness on user_id and version. I traced the code and apparently the [templating](https://github.com/lbryio/sqlboiler/blob/master/templates/23_merge.tpl#L64) used in SQLBoiler writes the [conflicting unique columns](https://github.com/lbryio/internal-apis/blob/master/app/model/user.go#L7051-L7056) in alphabetical order. This is problematic because the wallet table is the first table to have unique keys where the user_id is before the "other"(version) columns, and [we append the user_id for the delete query](https://github.com/lbryio/sqlboiler/blob/master/templates/singleton/boil_queries.tpl#L140) which makes the arguments out of order. As a result the delete query does not delete the conflict because there is no match. What I did to solve this was to keep track of the index of the object id based on the pre-compliment slice, then insert the value in the query argument slice where it is supposed to be rather than appending or prepending it. This should solve all future issues where there are more than 2 columns and the object id column could be anywhere in the array depending on the alphabetical order. @tzarebczan We do not have to merge this right away as it is only impacts the wallet table rows conflict since this is the only table with the columns out of the assumed order. So we can fix the wallet table issue first, handle these merge errors manually in the meantime and once finished we can merge this to fix the issue. @nikooo777 We just have to be aware of this bug when we add tables with uniqueness columns. We just got lucky that this was the first table where the assumed order did not match the alphabetical order.

tiger5226 commented

2019-06-12 05:01:06 +02:00

(Migrated from github.com)

BTW, something else to consider here is the possibility of the argument reversal deleting the wrong wallet table entry. In theory this could cause us to lose wallet data. However, I believe this is impossible since these queries are part of a transaction and if it deletes the wrong row, the conflict still exists and the transaction will be rolled back. I don't see a plausible scenario where we delete the wrong row and the merge is still successful.

lyoshenka (Migrated from github.com) approved these changes 2019-06-12 16:31:10 +02:00

lyoshenka (Migrated from github.com) left a comment

I'm only 90% sure I understand what the issue is but I appreciate your detailed writeup and this sounds like a reasonable solution.

templates/singleton/boil_queries.tpl

					
				@ -86,0 +86,4 @@

				    for i, column := range conflict.columns {

				        if column == conflict.objectIdColumn {

				            objectIDIndex = i

				            break

lyoshenka (Migrated from github.com) commented

2019-06-12 16:24:21 +02:00

you can break after this

you can `break` after this

lyoshenka commented

2019-06-12 16:34:02 +02:00

(Migrated from github.com)

I'm guessing this bug snuck in when we added code to deal with multi-column relations, right?

tiger5226 (Migrated from github.com) reviewed 2019-06-13 05:26:03 +02:00

templates/singleton/boil_queries.tpl

					
				@ -86,0 +86,4 @@

				    for i, column := range conflict.columns {

				        if column == conflict.objectIdColumn {

				            objectIDIndex = i

				            break

tiger5226 (Migrated from github.com) commented

2019-06-13 05:26:03 +02:00

woops..I thought I did that...like literally was thinking it and never added it lol.

tiger5226 commented

2019-06-13 05:38:18 +02:00

(Migrated from github.com)

Yes. We always assumed the position of the user_id column in the query argument list and the conflicting column came second. See here. I swapped them around in the query update I did. The real problem came in when I made the assumption on the ordering of the columns automatically generated. The old way it was just hardcoded, user_id first check, first complimented conflict second, so it never depended on the ordering because it was only using the first conflicting one.

We did not see it until the wallet table was added and in use due to the alphabetical ordering.

Yes. We always assumed the position of the `user_id` column in the query argument list and the conflicting column came second. See [here](https://github.com/lbryio/sqlboiler/blob/d180a095ca6a5baa53027109fb1f2937d9890f6f/templates/singleton/boil_queries.tpl#L73). I swapped them around in the query update I did. The real problem came in when I made the assumption on the ordering of the columns automatically generated. The old way it was just hardcoded, user_id first check, first complimented conflict second, so it never depended on the ordering because it was only using the first conflicting one. We did not see it until the wallet table was added and in use due to the alphabetical ordering.

nikooo777 commented

2019-07-01 15:16:07 +02:00

(Migrated from github.com)

BTW, something else to consider here is the possibility of the argument reversal deleting the wrong wallet table entry. In theory this could cause us to lose wallet data. However, I believe this is impossible since these queries are part of a transaction and if it deletes the wrong row, the conflict still exists and the transaction will be rolled back. I don't see a plausible scenario where we delete the wrong row and the merge is still successful.

please let's add a test for this

> BTW, something else to consider here is the possibility of the argument reversal deleting the wrong wallet table entry. In theory this could cause us to lose wallet data. However, I believe this is impossible since these queries are part of a transaction and if it deletes the wrong row, the conflict still exists and the transaction will be rolled back. I don't see a plausible scenario where we delete the wrong row and the merge is still successful. please let's add a test for this

nikooo777 commented

2023-05-27 04:38:14 +02:00

(Migrated from github.com)

I regret stalling this PR now hahaha.

I have been dealing with this issue for a whole month now where generating the models it just randomly and not always reorders the columns in alphabetical order.
I still don't really understand the problem here and tbh I'm a bit afraid of merging this PR without understanding it.

I regret stalling this PR now hahaha. I have been dealing with this issue for a whole month now where generating the models it just randomly and not always reorders the columns in alphabetical order. I still don't really understand the problem here and tbh I'm a bit afraid of merging this PR without understanding it.

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix_column_order_bug:fix_column_order_bug

git checkout fix_column_order_bug

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout master

git merge --no-ff fix_column_order_bug

git checkout fix_column_order_bug

git rebase master

git checkout master

git merge --ff-only fix_column_order_bug

git checkout fix_column_order_bug

git rebase master

git checkout master

git merge --no-ff fix_column_order_bug

git checkout master

git merge --squash fix_column_order_bug

git checkout master

git merge --ff-only fix_column_order_bug

git checkout master

git merge fix_column_order_bug

git push origin master

Sign in to join this conversation.

No reviewers

lyoshenka

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/sqlboiler#5