LG-13570 we were parsing JSON as JSON and that was bad#10846
Merged
Conversation
zachmargolis
approved these changes
Jun 20, 2024
Comment on lines
+7
to
+8
| JSON.parse'[{"emails":["ursula@example.com"], | ||
| "issuers":["urn:gov:gsa:openidconnect.profiles:sp:sso:agency_name:app_name"]}]' |
Contributor
There was a problem hiding this comment.
probably easier to have this as a ruby literal than managing a string
Suggested change
| JSON.parse'[{"emails":["ursula@example.com"], | |
| "issuers":["urn:gov:gsa:openidconnect.profiles:sp:sso:agency_name:app_name"]}]' | |
| [ | |
| { | |
| "emails" => ["ursula@example.com"], | |
| "issuers" => ["urn:gov:gsa:openidconnect.profiles:sp:sso:agency_name:app_name"], | |
| }, | |
| ] |
Contributor
Author
There was a problem hiding this comment.
The purpose of this is to point out that this line is a String in our config, and we auto-parse it to JSON.
The comment above is an important addition.
Contributor
There was a problem hiding this comment.
The config parsing is standard across our app, and intended to abstract away the minutiae of parsing the string values.
But if we really want to parse a string in this spec,, I think that the multi-line string literal here is really hard to parse, it would be much clearer using heredoc syntax and adding in the optional parens:
Suggested change
| JSON.parse'[{"emails":["ursula@example.com"], | |
| "issuers":["urn:gov:gsa:openidconnect.profiles:sp:sso:agency_name:app_name"]}]' | |
| JSON.parse(<<~STR) | |
| [{"emails":["ursula@example.com"],"issuers":["urn:gov:gsa:openidconnect.profiles:sp:sso:agency_name:app_name"]}] | |
| STR |
changelog: Bug Fixes, Reports, Resolve double-parsing JSON in Dropoff report (LG-13570)
ajfarkas
added a commit
that referenced
this pull request
Jun 21, 2024
* LG-13570 we were parsing JSON as JSON and that was bad * LG-13570 lint corrections changelog: Bug Fixes, Reports, Resolve double-parsing JSON in Dropoff report (LG-13570)
ajfarkas
added a commit
that referenced
this pull request
Jun 24, 2024
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🎫 Ticket
Link to the relevant ticket:
LG-13570
🛠 Summary of changes
We were seeing the error:
no implicit conversion of Array into Stringon the dropoff report.This was because we're implicitly converting our configs to their described
typeinidentity_config.rb, and then trying to parse that JSON withJSON.parseindrop_off_report.rb.This PR comments our code and removes the offending
parse.📜 Testing Plan
Provide a checklist of steps to confirm the changes.
Thanks to @h-m-m and @nprimak for working on this bug.