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
Offset issue resolved for scanning results back into struct #5227
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jinzhu Is there a reason this was merged when it failed to pass on Postgres? We're currently using this on CockroachDB (Postgres protocol compatible), after upgrading to 1.23.5 we started getting this error:
After downgrading to 1.23.4, the issue went away. We were doing a simple scan from a raw query
If we need to put together a test case, I can take the time, however I'm curious why this was merged when it caused the Postgres tests to fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clarkmcc the tests for Postgres were failing for a different case, specifically
TestSmartMigrateColumn
which did not invoke anyscan
code from here. And the failure was intermittent. All the tests do pass with the code merged in with the latest test runs.However, I can clearly see how the panic is occurring. The fix would be having a check that the index exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this could explain the panic, but after further investigation, we could only reproduce it on this query which performs an inner join on the same table as the original select, which means the resulting recordset had a
parent
and achildren
copy of each column.Changing the query to this resolved the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a PR ready for a duplicate issue fix on a
distinct
query. #5369See if that resolves your issue. I don't think it will. But you can run a fork with the check that the index exists before running the
for
loop and see if that resolves your issue. Otherwise, would have to dig to see what's causing the panic and also need a test case for it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking a bit more about the issue, I believe you are right. It can occur for inner join on the same table.
What this PR originally sought to solve was, that if there are two tables with identical column names, before this fix, the fields would be returned with the same values across different tables while scanning the result back to the struct.
So this offset fix was picked right from GORM V1 where this used to work https://github.com/jinzhu/gorm/blob/master/scope.go#L491
But running inner join on the same table would cause it to panic since the number of fields are the same. So the fix would be to not offset when running join on the same table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works because the defined schema is consistent with the database fields, when we use raw and scan, it is often inconsistent, which causes the array to go out of bounds.
I think we can skip this part because we can't determine whether we want to use the value of
children
orparent
in this case.Edit
It seems to be a similar problem, I updated #5369, you can try if it solves your issue.