Fixing JwtCreator builder when setting headers as a map #320
Conversation
There was a problem hiding this comment.
Thanks for catching this.
If you call Map#putAll with a null value it will throw NPE. I think it's missing the null check and the "remove claim" scenario. Please add that and the corresponding test cases as follow:
- when calling
withHeaderwith null value it should not throw NPE but return the builder instance anyway. - when calling
withHeaderwith a map with a claim that was already set, it should overwrite its value. - when calling
withHeaderwith a map with a claim with null value, it should not add it and remove it if present in theheaderClaimsfield (this is the same behavior we have when adding body-level claims)
|
@lbalmaceda thank you for a quick reply, this makes sense, I will update it and commit a new change |
…provided with a null value in the map also added unit test for this behaviour
|
@lbalmaceda could you please have another look at this change? I will really appreciate if we can have this fixed in the next version. thanks. |
|
@maxbalan 😐Sorry I missed this. I've set up a reminder and will review it tomorrow. |
| @@ -66,12 +66,24 @@ private JWTCreator(Algorithm algorithm, Map<String, Object> headerClaims, Map<St | |||
|
|
|||
| /** | |||
| * Add specific Claims to set as the Header. | |||
| * If provided map is null then nothing is changed | |||
| * If provided map contains a header with null value then that header will be removed from the header claims | |||
There was a problem hiding this comment.
Small rewrite
| * If provided map contains a header with null value then that header will be removed from the header claims | |
| * If provided map contains a claim with null value then that claim will be removed from the header |
| * | ||
| * @param headerClaims the values to use as Claims in the token's Header. | ||
| * @return this same Builder instance. | ||
| */ | ||
| public Builder withHeader(Map<String, Object> headerClaims) { | ||
| this.headerClaims = new HashMap<>(headerClaims); | ||
| if (headerClaims == null) |
There was a problem hiding this comment.
Please use braces {} for this even if it's a one liner, to follow the project code style
| @Test | ||
| public void shouldRemoveHeaderIfTheValueIsNull() throws Exception { | ||
| Map<String, Object> header = new HashMap<String, Object>(); | ||
| header.put("test", null); |
There was a problem hiding this comment.
The claim must first be set in order to be unset. Use a claim that you know is being set to the header.
…move confusions in the test readability
|
@lbalmaceda Hi, I fixed the pr as per your suggestions I hope this removes some of the ambiguity and adds more to tests readability, please have another look |
Co-Authored-By: Luciano Balmaceda <[email protected]>
|
@lbalmaceda looks like when I applied your suggestions for test name change it dismissed the pr approve |
currently when using JwtCreator when setting headers as a map it is overwriting existing headers map, which forces to use
withHeader(Map<String, Object> headerClaims)first then any other headers.e.g.
JWT.create().withKeyId(ID).withHeader(map)JWT.create().withHeader(map).withKeyId(ID)