Skip to content

Allow option for adding Iterable<String> header values#10331

Merged
klustria merged 7 commits into
helidon-io:mainfrom
klustria:klustria_10294-iterable-string-header-values
Jul 14, 2025
Merged

Allow option for adding Iterable<String> header values#10331
klustria merged 7 commits into
helidon-io:mainfrom
klustria:klustria_10294-iterable-string-header-values

Conversation

@klustria

@klustria klustria commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

Description

Allow option for adding Iterable header values

Documentation

If enhancement: provide description with example code/config snippet or pointer to issue with the description

2 additional APIs were added in HeaderValues:

  1. create(HeaderName name, Iterable values)
  2. create(String name, Iterable values)

example:

final String[] ordinalNumbers = {"First", "Second", "Third"};
Iterable<String> ordinals = Arrays.asList(ordinalNumbers);

Header header  = HeaderValues.create("ordinals", o);

If feature: summarize feature and provide pointer to doc issue
Currently no documentation for HeaderValues usage, but if one exist, add the above example.

If no doc impact: None

@klustria klustria requested review from tjquinno and tomas-langer July 7, 2025 19:29
@klustria klustria self-assigned this Jul 7, 2025
@oracle-contributor-agreement oracle-contributor-agreement Bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jul 7, 2025
@klustria klustria linked an issue Jul 8, 2025 that may be closed by this pull request
@tomas-langer

Copy link
Copy Markdown
Member

We already have methods for a Collection, why is Iterable needed? Please describe the use case.

Comment thread http/http/src/main/java/io/helidon/http/HeaderValueIterable.java Outdated
Comment thread http/http/src/main/java/io/helidon/http/HeaderValues.java Outdated
Comment thread http/http/src/main/java/io/helidon/http/HeaderValueIterable.java Outdated
@tjquinno

tjquinno commented Jul 8, 2025

Copy link
Copy Markdown
Member

We already have methods for a Collection, why is Iterable needed? Please describe the use case.

See the linked issue description which mentions gRPC's Metadata type as a specific example that makes header values available this way. I have edited the description to add a link to the Javadoc.

Given that dealing with headers can be in performance-critical sections of code, it could be useful to be able to avoid creating a throw-away collection from an Iterable.

@tjquinno tjquinno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some general comments. Possible changes.

Comment thread http/http/src/main/java/io/helidon/http/HeaderValueIterable.java Outdated
Comment thread http/http/src/main/java/io/helidon/http/HeaderValueIterable.java Outdated
…ableValueBase because an Iterable is not writable.
@klustria klustria requested review from tjquinno and tomas-langer July 8, 2025 16:28

@tjquinno tjquinno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Another change, in the handling of the size this time.

Comment thread http/http/src/main/java/io/helidon/http/HeaderValueIterable.java Outdated
@klustria klustria requested a review from tjquinno July 9, 2025 00:03
@tjquinno

tjquinno commented Jul 10, 2025

Copy link
Copy Markdown
Member

Given the outcome of the discussion of what behavior we should implement, would it make sense to just have

public static Header create(HeaderName name, Iterable<String> values) {
    return create(name, StreamSupport.stream(values.spliterator(), false).toList());
}

and not even have a separate implementation for HeaderValueIterable?

This provides the convenience of the added factory method and therefore saves developera from having to create a list or array explicitly themselves, but the resulting Header still exhibits the snapshot behavior we want.

Although this would not create the list lazily upon first need, and doing that might have saved a little performance, there would be real savings only if the header values were never accessed which seems unlikely.

@klustria

Copy link
Copy Markdown
Contributor Author

Given the outcome of the discussion of what behavior we should implement, would it make sense to just have

public static Header create(HeaderName name, Iterable<String> values) {
    return create(name, StreamSupport.stream(values.spliterator(), false).toList());
}

and not even have a separate implementation for HeaderValueIterable?

This provides the convenience of the added factory method and therefore saves developera from having to create a list or array explicitly themselves, but the resulting Header still exhibits the snapshot behavior we want.

Although this would not create the list lazily upon first need, and doing that might have saved a little performance, there would be real savings only if the header values were never accessed which seems unlikely.

Yes, the code would be something like that. I'm casting if it is a collection so conversion is not needed, but otherwise, I'll use that same statement to convert non-collection type Iterables.

@tjquinno

tjquinno commented Jul 11, 2025

Copy link
Copy Markdown
Member

Yes, the code would be something like that. I'm casting if it is a collection so conversion is not needed, but otherwise, I'll use that same statement to convert non-collection type Iterables.

Ah, yes, of course. Sounds good.

Comment thread http/http/src/test/java/io/helidon/http/HeaderValueIterableTest.java Outdated
@klustria

Copy link
Copy Markdown
Contributor Author

@tomas-langer @tjquinno , I have finalized the changes. Can you please review?

@klustria klustria merged commit 49dd5dc into helidon-io:main Jul 14, 2025
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OCA Verified All contributors have signed the Oracle Contributor Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4.x Add ability to create a Header from an Iterable<String>

5 participants