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

[FIX] Fix Activesupport json encode for hash with duplicate keys #50489

Merged

Conversation

maniSHarma7575
Copy link
Contributor

@maniSHarma7575 maniSHarma7575 commented Dec 30, 2023

Fix: #50481

Motivation / Background

This Pull Request has been created because to fix #50481

The output for the hash is inconsistent when using ActiveSupport::JSON.encode with the duplicate key in RAILS_VERSION=7.1.0. Specifically, if we examine the result below, the key 'a' appears twice in the output JSON for the same key 'a'.

For RAILS_VERSION=7.0.8, it behaves as expected. Key is only once with later value.

For example:

ActiveSupport::JSON.encode({ 'a' => 1, :a => 2 })

Result:

"{\"a\":1,\"a\":2}"

This behavior is not observed in RAILS_VERSION=7.0.8.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #50481]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@maniSHarma7575 maniSHarma7575 changed the title [FIX] Fix Activesupport json encode for hash [FIX] Fix Activesupport json encode for hash with duplicate keys Dec 30, 2023
@maniSHarma7575 maniSHarma7575 force-pushed the 50481-fix-activesupport-json-encode branch from 87caac1 to 2700716 Compare December 30, 2023 14:43
@maniSHarma7575 maniSHarma7575 force-pushed the 50481-fix-activesupport-json-encode branch from 2700716 to 94a4adb Compare December 30, 2023 14:53
@maniSHarma7575
Copy link
Contributor Author

cc: @byroot

@byroot byroot merged commit 7b9e9ee into rails:main Dec 30, 2023
3 of 4 checks passed
byroot added a commit that referenced this pull request Dec 30, 2023
…-json-encode

[FIX] Fix Activesupport json encode for hash with duplicate keys
@byroot
Copy link
Member

byroot commented Dec 30, 2023

Thanks, I backported it to 7-1-stable. We should fix it in flori/json too though.

@maniSHarma7575 maniSHarma7575 deleted the 50481-fix-activesupport-json-encode branch December 31, 2023 01:08
@maniSHarma7575
Copy link
Contributor Author

Thanks, I backported it to 7-1-stable. We should fix it in flori/json too though.

Thanks for merging it @byroot T . This is my first contribution to rails. Looking forward to more.

Yeah, I'll do it in flori/json as well.

@maniSHarma7575
Copy link
Contributor Author

Thanks, I backported it to 7-1-stable. We should fix it in flori/json too though.

@byroot I have fixed it in flori/json as well. Could you kindly review it?

flori/json#564

@jhawthorn
Copy link
Member

Sorry I missed this change at the time, I don't think we should have merged this. At some point we have to have a limit on how much double checking Rails does of the user's input and I think this is well within the area we should be "garbage in/garbage out".

Being forced to support this really paints us into a corner in how we can implement this.

I'd also argue that I'd actually prefer { 'a' => 1, :a => 2 } be turned into the questionably valid JSON as "{\"a\":1,\"a\":2}" rather than either "{\"a\":1}" or "{\"a\":2}" as it would allow me to see that there is an issue with my program rather than hiding the error and making an arbitrary choice.

@byroot
Copy link
Member

byroot commented May 22, 2024

Yeah feel free to revert.

rafaelfranca added a commit that referenced this pull request May 22, 2024
…esupport-json-encode"

This reverts commit 7b9e9ee, reversing
changes made to 590a675.

Reason: #50489 (comment)
rafaelfranca added a commit that referenced this pull request May 22, 2024
…esupport-json-encode"

This reverts commit 7b9e9ee, reversing
changes made to 590a675.

Reason: #50489 (comment)
rafaelfranca added a commit that referenced this pull request May 22, 2024
…esupport-json-encode"

This reverts commit 7b9e9ee, reversing
changes made to 590a675.

Reason: #50489 (comment)
@rafaelfranca
Copy link
Member

I reverted to not block the release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants