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

HttpSerializers client set Accept header #2023

Merged
merged 1 commit into from
Dec 22, 2021

Conversation

Scottmitch
Copy link
Member

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:
- 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),
Copy link
Member Author

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...

Copy link
Member

@idelpivnitskiy idelpivnitskiy Dec 14, 2021

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?

Copy link
Contributor

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.

Copy link
Member Author

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).

@Scottmitch Scottmitch merged commit 8a65e57 into apple:main Dec 22, 2021
@Scottmitch Scottmitch deleted the serial_accept branch December 22, 2021 01:30
idelpivnitskiy pushed a commit that referenced this pull request Jan 11, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants