Skip to content

Conversation

Vynbz
Copy link

@Vynbz Vynbz commented Aug 30, 2025

Motivation

As discussed in #2770, the HttpServerResponse interface in vertx-core already provides CharSequence overloads for header methods, but the web client's HttpRequest interface only accepts String parameters. This inconsistency means users need to convert CharSequence values (like Netty's HttpHeaderNames constants) to Strings unnecessarily.

@vietj vietj added this to the 5.1.0 milestone Aug 30, 2025
@Vynbz
Copy link
Author

Vynbz commented Aug 31, 2025

While putHeader(CharSequence, CharSequence) works fine, adding putHeader(CharSequence, Iterable<CharSequence>) causes ambiguous method call errors with existing code. Should I proceed with just the single-value overload, or would you prefer a different approach?

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

@Vynbz thanks for the PR.

*/
@Fluent
@GenIgnore(GenIgnore.PERMITTED_TYPE)
HttpRequest<T> putHeader(CharSequence name, CharSequence value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a default implementation that converts to String?

Copy link
Author

Choose a reason for hiding this comment

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

I've updated the implementation to use interface default methods as you suggested

@tsegismont
Copy link
Contributor

While putHeader(CharSequence, CharSequence) works fine, adding putHeader(CharSequence, Iterable<CharSequence>) causes ambiguous method call errors with existing code. Should I proceed with just the single-value overload, or would you prefer a different approach?

Please go ahead and add the method. You can update WebClientTest for ambiguous method calls.

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @Vynbz

In fact, I was suggesting to add default implementations, so that existing implementations in other projects don't break with the new release.

But in our project, we must implement these new methods in an efficient manner (always converting to string defeats the original purpose of this PR)

@Vynbz
Copy link
Author

Vynbz commented Sep 12, 2025

@tsegismont thanks for review

Just to make sure I understand correctly before I push the changes - you want me to keep the default implementations in the HttpRequest interface, but also keep efficient override implementations in HttpRequestImpl that pass the CharSequence directly to headers().set() without converting to String?

Also regarding the tests, I've updated them to handle the ambiguous method calls by adding CharSequence cast - is this the right approach?

@tsegismont
Copy link
Contributor

@tsegismont thanks for review

Just to make sure I understand correctly before I push the changes - you want me to keep the default implementations in the HttpRequest interface, but also keep efficient override implementations in HttpRequestImpl that pass the CharSequence directly to headers().set() without converting to String?

Exactly

Also regarding the tests, I've updated them to handle the ambiguous method calls by adding CharSequence cast - is this the right approach?

Yes

@Vynbz Vynbz force-pushed the feat/charsequence-header-names branch from 1373084 to 2eda11e Compare September 15, 2025 14:15
@Vynbz
Copy link
Author

Vynbz commented Sep 15, 2025

@tsegismont Done! Changes implemented as discussed and commits squashed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants