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

Offset issue resolved for scanning results back into struct #5227

Merged
merged 1 commit into from Apr 7, 2022

Conversation

Joker666
Copy link
Contributor

@Joker666 Joker666 commented Apr 4, 2022

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

This PR fixes an accidentally introduced bug while merging PR #5143
The modified commit f3e2da5 introduced an offset bug which resulted in nil for scanning results back into the struct.

It's my bad, I did not write extensive test cases which will catch this bug. Now I've updated test cases to account for all known cases that can happen.

close #5142

This fixes the demo PR for the playground - go-gorm/playground#460

@Joker666
Copy link
Contributor Author

Joker666 commented Apr 4, 2022

Hmm, the test TestSmartMigrateColumn is failing for Postgres dialect. I see the same issue for PR #5226

I think the issue is somewhere else

@jinzhu jinzhu merged commit 81c4024 into go-gorm:master Apr 7, 2022
@@ -196,7 +196,7 @@ func Scan(rows Rows, db *DB, mode ScanMode) {
for idx, column := range columns {
if field := sch.LookUpField(column); field != nil && field.Readable {
if curIndex, ok := selectedColumnsMap[column]; ok {
for fieldIndex, selectField := range sch.Fields[curIndex:] {
for fieldIndex, selectField := range sch.Fields[curIndex+1:] {
Copy link
Contributor

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:

panic: runtime error: slice bounds out of range [11:10]

After downgrading to 1.23.4, the issue went away. We were doing a simple scan from a raw query

tx.Raw("...").Scan(&rows).Error

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?

Copy link
Contributor Author

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 any scan 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.

Copy link
Contributor

@clarkmcc clarkmcc May 26, 2022

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 a children copy of each column.

SELECT * FROM entity parent
    INNER JOIN entity children ON children.genealogy @> ARRAY[parent.entity_key]
WHERE parent.entity_key = @entity_key;

Changing the query to this resolved the issue

SELECT children.* FROM entity parent
    INNER JOIN entity children ON children.genealogy @> ARRAY[parent.entity_key]
WHERE parent.entity_key = @entity_key;

Copy link
Contributor Author

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. #5369

See 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.

Copy link
Contributor Author

@Joker666 Joker666 May 26, 2022

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.

Copy link
Member

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 or parent in this case.

Edit

It seems to be a similar problem, I updated #5369, you can try if it solves your issue.

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.

Gorm V2 cannot handle JOIN for tables with the same column name
4 participants