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.
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
Issue #4128 - test the decoding of OpenId Credentials #4166
Issue #4128 - test the decoding of OpenId Credentials #4166
Changes from 7 commits
153c404
cee3552
8b7c131
218505b
dd8914d
0c73cac
06b7925
2b0e1f5
7d94370
67ec3d2
f784135
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the safest of casts. I'd check that the keys are strings using code like we've done in previous PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a Map with keys that are not strings even be valid JSON. If our JSON parser is capable of returning invalid JSON then we should fix that in the parser rather than testing the return type is valid everywhere it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point, and do not oppose this change given that argument. However, I still would do the conversion at run-time to a
Map<String, Object>
and not cast it to that. We're not in a high throughput case here. We can tolerate 250ms for the entire code flow + login (say 6 request/responses) in most cases I've seen. So, I'd err on the side of clean code that relies on the compiler for type checking. As I said though, your argument is hard to dispute.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method returns a typed map. These types should be checked as well before returning.