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

Remove null empty headers #724

Merged
merged 3 commits into from
Jul 6, 2018
Merged

Conversation

fmcejudo
Copy link
Contributor

@@ -535,6 +535,8 @@ public RequestTemplate headers(Map<String, Collection<String>> headers) {
* @see Request#headers()
*/
public Map<String, Collection<String>> headers() {
headers.forEach((key, value) -> value.removeIf(e -> e == null));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the idea of modifying the existing map...

may be just filter and create new one?!

…he current map. Supporting with tests as it needs to be LinkedHashMap not to lost the sorting
@velo
Copy link
Member

velo commented Jul 5, 2018

@kdavisk6 LGTM, what do you think?

Do you foresee any issues on removing empty headers?


return Collections.unmodifiableMap(
headers.entrySet().stream().filter(h -> h.getValue() != null && !h.getValue().isEmpty())
.collect(toMap(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just use default toMap... toMap(Entry::getKey, Entry::getValue) no need to handle duplicated keys or map creation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I do so, it returns the map in any order, with LinkedHashMap it collects them in same order. In fact that was my first approach and it broke tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of the Map really should not matter, but I don't have a strong opinion on the use of LinkedHashMap. I do agree that there is no need to handle duplicates, that is, unless you want to support that. In that case, I would merge the duplicate key values, since it is acceptable to have more than one value for a header.

@kdavisk6
Copy link
Member

kdavisk6 commented Jul 5, 2018

@velo I've added some feedback, I think this is ok to merge.

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.

3 participants