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

[Bug] Got wrong value in Find/Scan with JOIN and same column name #5711

Closed
StephanoGeorge opened this issue Sep 21, 2022 · 12 comments
Closed
Assignees
Labels
type:with reproduction steps with reproduction steps

Comments

@StephanoGeorge
Copy link
Contributor

GORM Playground Link

go-gorm/playground#516

Description

Got wrong value in Find/Scan with JOIN and same column name.

See pull request. Thanks!

@a631807682
Copy link
Member

  1. Gorm will not parse sql, so it cannot determine which model the same column name belongs to, you need to use an alias.
  2. Similarly, when the embedded struct uses the same field name, it is impossible to know the corresponding relationship between it and the column, unless you specify it by tag.

@StephanoGeorge
Copy link
Contributor Author

@a631807682 But according to #5142, GORM should handle this, isn't it?

@a631807682
Copy link
Member

@StephanoGeorge Indeed, this seems to be supported, it seems to be a match attempt, but as soon as the order is changed, the test must fail, so I don't recommend it, but I'll follow up on it.

https://github.com/go-gorm/gorm/blob/master/tests/scan_test.go#L179

@StephanoGeorge
Copy link
Contributor Author

StephanoGeorge commented Sep 21, 2022

Thanks! Select("people.*, addresses.*") will make result column name has table name as prefix, so the Scan can be done without ambiguity

@a631807682
Copy link
Member

a631807682 commented Sep 21, 2022

It only works if the defined field and the query column are exactly the same.
It cause errors if we select more column like:

Select("people.*,people.id, addresses.*")

It cause errors if we defined more field like your case do:

type User struct {
	gorm.Model
	Name      string
	Pets      []Pet	// cause error
	Languages []Language `gorm:"many2many:user_language;"` // cause error
}

type Result struct {
	User
	UserLanguage
	Language
	Pet
}

https://github.com/go-gorm/gorm/blob/master/scan.go#L206

I think this is an unsolvable problem, so this may not be a fully implemented feature.

@StephanoGeorge
Copy link
Contributor Author

StephanoGeorge commented Sep 22, 2022

I think GORM can parse Select("user.*", "user_language.skilled", "language.*") if each string in Select satisfy `?(.+)`?\..+, and scan first 6 fields to Result.User, then scan skilled to Result.UserLanguage, and so on. What do you think?

@a631807682
Copy link
Member

a631807682 commented Sep 22, 2022

I don't think that's a good idea

  1. Parsing SQL affects efficiency, even if users are sure their SQL is fine.

  2. This requires the support of all drivers, even for different versions, to ensure consistency with the database parser.

  3. Even if we parse the SQL, we still don't know their correspondence, for example, I have a sharding table, does it need to be filled into user?

Select("user_1.* ...")

@StephanoGeorge
Copy link
Contributor Author

StephanoGeorge commented Sep 22, 2022

  1. I meant only parsing strings in Select, they are only table or column names.
  2. GORM already needs to support all drivers for SELECT, INSERT, UPDATE, DELETE.
  3. If we have sharding table, we will Select("user.*"), GORM will find which table we will use, right?

@StephanoGeorge
Copy link
Contributor Author

@a631807682 I can make a PR if you accept this

@a631807682
Copy link
Member

@StephanoGeorge Welcome, the maintainer will review it

@StephanoGeorge
Copy link
Contributor Author

I found that GORM doesn't have to parse Select().
@jinzhu Please review the PR.

@a631807682
Copy link
Member

complete via a3cc6c6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:with reproduction steps with reproduction steps
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants