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

JettyClientHttpRequest#applyHeaders throws IllegalArgumentException when adding header with value null #11010

Open
steinsag opened this issue Dec 1, 2023 · 3 comments
Labels
Bug For general bugs on Jetty side Stale For auto-closed stale issues and pull requests

Comments

@steinsag
Copy link

steinsag commented Dec 1, 2023

Spring Web 6.1 uses Jetty 12. Jetty 12 changed the implementation of

org.springframework.http.client.reactive.JettyClientHttpRequest#applyHeaders

from (Jetty 11)

public Mutable add(String name, String value)
{
    if (value != null)
        return add(new HttpField(name, value));
    return this;
}

to

default Mutable add(String name, String value)
{
    if (value == null)
        throw new IllegalArgumentException("null value");
    return add(new HttpField(name, value));
}

This now breaks Spring Web 6.1.

A similar issue was reported in: #10508

I initially reported this issue to Spring project, but they say this is a regression in Jetty: spring-projects/spring-framework#31714

@steinsag steinsag added the Bug For general bugs on Jetty side label Dec 1, 2023
@sbordet
Copy link
Contributor

sbordet commented Dec 1, 2023

@steinsag What are you trying to achieve by setting an HTTP header to null?

HTTP headers are strings, they can be empty, but null is not a valid value.
If you want to set an empty String, use add(name, "").

Otherwise you should filter out invalid values, like you filter out non-numbers for HTTP headers that only take numerical values such as Content-Length.

This change was done in 12 to make the behavior of HttpFields consistent: in Jetty 11, if you used add(HttpHeader, String) instead of add(String, String), you would get IllegalArgumentException for null values.
So null was treated differently depending on the signature of the method.

In Jetty 12 we made the handling of null values consistent.

@steinsag
Copy link
Author

steinsag commented Dec 1, 2023

Our use-case are including some optional trace IDs, which we might have received from an upstream consumer of our service. In the past, we could include them without filtering null values. Jetty simply ignored such null values.

As Jetty is now rejecting null values, it breaks several of our services and we would need to add logic to only set those trace headers if they have a valid value.

String someOptionalTraceId = ??? <--- could be null, but also a valid string
...
Map<String, String> someHeaders = new HashMap<>();
someHeaders.put("X-TRACE-ID", someOptionalTraceId);

WebClient build = WebClient.builder().build();
build.get().uri("http://example.com/").headers(
        httpHeaders -> someHeaders.forEach(httpHeaders::add)
);

Copy link

github-actions bot commented Dec 1, 2024

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Stale For auto-closed stale issues and pull requests
Projects
None yet
Development

No branches or pull requests

2 participants