-
Notifications
You must be signed in to change notification settings - Fork 183
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
HttpSerializers client set Accept header #2023
Conversation
Motivation: HttpSerializers currently set the Content-Type header but not the Accept header. The Accept header provides the server a hint as to what type of contnet should be returned. Modifications: - Modify implementations in HttpSerializers to set the Accept header Result: Clients provide info to servers about what type of content is accepted in a response.
@@ -62,34 +63,41 @@ | |||
|
|||
private static final HttpSerializerDeserializer<Map<String, List<String>>> FORM_ENCODED_UTF_8 = | |||
new DefaultHttpSerializerDeserializer<>(new FormUrlEncodedSerializer(UTF_8), | |||
headers -> headers.set(CONTENT_TYPE, APPLICATION_X_WWW_FORM_URLENCODED_UTF_8), | |||
headers -> headers.set(CONTENT_TYPE, APPLICATION_X_WWW_FORM_URLENCODED_UTF_8) | |||
.set(ACCEPT, APPLICATION_X_WWW_FORM_URLENCODED), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a trade-off here is if folks want to accept different/multiple payload types (e.g. to handle errors differently) we will overwrite the header value here. we have some options:
- use
set
(folks could manually adjust headers if necessary, however serializers don't compose as nicely, but maybe safer to be explicit about accept values) - use
add
(if there is preexisting type that the user intends to override then we may keep ACCEPT headers that aren't really supported)
folks could also create their own serializers if they don't want this accept value forced on them...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While HTTP allows multiple header entries with the same name, there is a risk that some other peers won't recognize it the same way as a single Accept
header with a list of values.
Users can also insert a filter, that will set the correct values after serialization. Not good to split the logic to override our value though. We can also consider adding an overload for all special serializers (like formUrlEncodedSerializer
) that takes a user-defined function for headers. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this to be a user-choice vs an library one.
@idelpivnitskiy's suggestion makes sense, I believe we can make an effort to "add" a header (I am leaning towards add), but any user configuration should take precedence. Creating your own serializer is still an option, but I think its too much to ask for just setting ACCEPT headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is safer to start with set
for now bcz that is all we know for sure is supported here. We have options to evolve as described above (filter - not ideal to disconnect code, allow users to customize serializer add/set behavior).
Motivation: HttpSerializers currently set the Content-Type header but not the Accept header. The Accept header provides the server a hint as to what type of contnet should be returned. Modifications: - Modify implementations in HttpSerializers to set the Accept header Result: Clients provide info to servers about what type of content is accepted in a response.
Motivation:
HttpSerializers currently set the Content-Type header
but not the Accept header. The Accept header provides
the server a hint as to what type of contnet should
be returned.
Modifications:
Accept header
Result:
Clients provide info to servers about what type of
content is accepted in a response.