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

RowToStructByName Snake Case Collision #2085

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

nolandseigler
Copy link
Contributor

rows.go fieldPosByName currently removes all "_" characters. This causes structs with "db" tags like "account_id" and "account__id" to be treated as the same tag. The index returned by fieldPosByName is always the first field tagged which causes an error like "struct doesn't have corresponding row field account__id".

I have written a test that demonstrates the issue and implemented some proof of concept code to demonstrate a potential fix. All tests pass.

This is my first PR. Please let me know if I am now following the proper procedure. I can certainly open up a discussion or issue as well. Thank you.

@nolandseigler nolandseigler marked this pull request as ready for review July 12, 2024 03:31
@jackc
Copy link
Owner

jackc commented Jul 12, 2024

I suppose this is more correct behavior when a db tag is provided. Though it's hard to see how having a database table where the columns names only differed by underscores is a good idea...

That said, if this change is going to be made it should cover all normalization. That is, if a db tag is provided all name normalization is disabled. I would suggest also doing a simple == comparison instead of the case insensitive compare.

@nolandseigler
Copy link
Contributor Author

I suppose this is more correct behavior when a db tag is provided. Though it's hard to see how having a database table where the columns names only differed by underscores is a good idea...

That said, if this change is going to be made it should cover all normalization. That is, if a db tag is provided all name normalization is disabled. I would suggest also doing a simple == comparison instead of the case insensitive compare.

Great idea. Thanks for the quick response. I will do that right now.

@jackc jackc merged commit 67aa0e5 into jackc:master Jul 12, 2024
14 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