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

Can specify separation character for getHeaderString #1134

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkarg
Copy link
Contributor

@mkarg mkarg commented Oct 3, 2022

In an earlier discussion @jansupol and me proposed to allow specifying the separation character for getHeaderString, as some header values actually may contain commas, so being able to separate them later bears the need to have non-comma separation.

@mkarg mkarg added this to the 4.0 milestone Oct 3, 2022
@mkarg mkarg requested a review from jansupol October 3, 2022 08:31
@mkarg mkarg self-assigned this Oct 3, 2022
Copy link
Contributor

@chkal chkal left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@mkarg
Copy link
Contributor Author

mkarg commented Oct 3, 2022

@jansupol Kindly requesting your review, as this idea originated from a discussion with you. :-)

@mkarg
Copy link
Contributor Author

mkarg commented Nov 1, 2022

@jansupol WDYT?

@jansupol
Copy link
Contributor

jansupol commented Nov 3, 2022

Thinking about it, does it not clash a bit with a HeaderDelegate, which is used to convert the Object to String (and String to Object). So the methods on ClientRequest and ContainerReponse should use the HeaderDelegate.

On one hand, the HeaderDelegate can be enriched with the separator, but on the other hand, the Header delegate should be aware of the proper separator type. So the separator on these outgoing sides (and partially on HttpHeaders for the outgoing side, like filters) looks like it asks the implementation to reparse the HeaderDelegate result with the new separator, but the original separator is not known (the HeaderDelegate knows it).

@mkarg
Copy link
Contributor Author

mkarg commented Dec 29, 2022

@jansupol I do not think that this has anything to do with HeaderDelegate (or I misunderstood your comment). The reason is that HeaderDelegate is dealing with one single header value, so it is invoked multiple times when receiving a list of values (like X-My-Header val1;val2;val3;val4;val5). Which separation character (here: semicolon) is wanted is completely decided by the application, because the header could be a custom one (here: X-My-Header), so HeaderDelegate simply cannot know which separation character is to be used. This PR just replaces the hard-coded "always comma" limitation by a "application can decide" freedom, which has not the least to do with HeaderDelegate.

@mkarg mkarg marked this pull request as ready for review January 22, 2023 18:17
@spericas
Copy link
Contributor

@jansupol @mkarg Any thoughts about this PR? Keep it or close it?

@jansupol
Copy link
Contributor

Jersey HeaderDelegates are application-specific, as we have SPI to register user-specific delegates, leading to @mkarg 's "application can decide" freedom. The Spec itself does not have the ability.

Maybe the other frameworks do have this ability, too and it could be added to the spec?

@mkarg
Copy link
Contributor Author

mkarg commented Dec 12, 2023

keep

@spericas spericas modified the milestones: 4.0, 5.0 Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

4 participants