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

Enable to convert request body to expected result type regardless of content-type #1745

Merged

Conversation

Projects
None yet
5 participants
@jongmin92
Copy link
Contributor

commented Apr 25, 2019

Related-Issue: #1584

Motivation:

Currently, it is possible to convert the request body to byte[], HttpData, String, CharSequence only for a specific content-type.
If the converters can convert the body to byte[], HttpData, String, CharSequence regardless of content-type, a user can easily convert the request body to specific result type what they want.

Modification:

  • In JacksonRequestConverterFunction, pass the request to the next RequestConverterFunction if it failed to deserialize its content.
  • In ByteArrayRequestConverterFunction and StringRequestConverterFunction, the request body can be converted to expectedResultType regardless of content-type.

Result:

A user can convert the request body to byte[], HttpData, String and CharSequence regardless of content-type.

@jongmin92 jongmin92 requested review from hyangtack, minwoox and trustin as code owners Apr 25, 2019

@CLAassistant

This comment has been minimized.

Copy link

commented Apr 25, 2019

CLA assistant check
All committers have signed the CLA.

@codecov

This comment has been minimized.

Copy link

commented Apr 25, 2019

Codecov Report

Merging #1745 into master will decrease coverage by 0.06%.
The diff coverage is 53.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1745      +/-   ##
============================================
- Coverage     71.81%   71.74%   -0.07%     
+ Complexity     8436     8432       -4     
============================================
  Files           761      761              
  Lines         33871    33875       +4     
  Branches       4168     4167       -1     
============================================
- Hits          24323    24305      -18     
- Misses         7439     7451      +12     
- Partials       2109     2119      +10
Impacted Files Coverage Δ Complexity Δ
...er/annotation/JacksonRequestConverterFunction.java 73.68% <0%> (-8.67%) 9 <0> (+3)
...ver/annotation/StringRequestConverterFunction.java 50% <50%> (-7.15%) 3 <0> (ø)
.../annotation/ByteArrayRequestConverterFunction.java 63.63% <66.66%> (+13.63%) 3 <2> (ø) ⬇️
.../linecorp/armeria/internal/IdleTimeoutHandler.java 80% <0%> (-20%) 4% <0%> (-1%)
...orp/armeria/internal/InboundTrafficController.java 51.61% <0%> (-16.13%) 10% <0%> (-2%)
...lient/circuitbreaker/CircuitBreakerHttpClient.java 69.69% <0%> (-3.04%) 10% <0%> (-1%)
...spring/web/reactive/ArmeriaServerHttpResponse.java 75% <0%> (-1.97%) 28% <0%> (-1%)
...om/linecorp/armeria/client/HttpSessionHandler.java 59.48% <0%> (-1.73%) 28% <0%> (-1%)
.../main/java/com/linecorp/armeria/server/Server.java 79.9% <0%> (-1.48%) 17% <0%> (ø)
.../armeria/client/endpoint/dns/DnsEndpointGroup.java 83.15% <0%> (-1.06%) 13% <0%> (-1%)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b58e26...5615b34. Read the comment docs.

@trustin trustin added the new-feature label Apr 25, 2019

@@ -68,6 +68,10 @@ public Object convertRequest(ServiceRequestContext ctx, AggregatedHttpMessage re
final MediaType contentType = request.contentType();
if (contentType != null && (contentType.is(MediaType.JSON) ||
contentType.subtype().endsWith("+json"))) {
if (expectedResultType == String.class) {

This comment has been minimized.

Copy link
@hyangtack

hyangtack Apr 26, 2019

Contributor

In JSON specifications, a JSON string value, such as "something", can also be a JSON text. So this might make a wrong result that a user doesn't expect.
So how about trying to convert the content to a JSON object and passing it to the next RequestConverterFunction if it failed?
In this way, you might inject the content as a byte array as well. (as described in the original issue #1584) Also, it may handle XML document, too.
Please let me know if my comment is unclear. 😃

@trustin

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Gentle ping, @jongmin92 😉

@jongmin92

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Thank for the ping!
I will apply the comments by this week (__)

@jongmin92 jongmin92 force-pushed the jongmin92:enable_jackson_request_convert_string branch from ee9a9c8 to e3fa442 May 4, 2019

@jongmin92

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Thanks for detailed comments!
I applied comments.
But I don't ensure whether I applied comments well.

I have a question to @hyangtack
Is it ok the way that tries to convert the content to a JSON object and passing it to the next RequestConverterFunction if it failed?

I think like that. First, To do return as byte[] or HttpData, I should check expectedResultType. But As I understand your comment, Don't I need to check expectedResultType?

If I implement based on your comment, I will add RequestConverterFunction.fallthrough(); instead of this line to pass content to the next RequestConverterFunction.

Is it sure I understand it well?

@@ -68,6 +69,13 @@ public Object convertRequest(ServiceRequestContext ctx, AggregatedHttpMessage re
final MediaType contentType = request.contentType();
if (contentType != null && (contentType.is(MediaType.JSON) ||
contentType.subtype().endsWith("+json"))) {
if (expectedResultType == byte[].class) {
return request.content().array();

This comment has been minimized.

Copy link
@trustin

trustin May 8, 2019

Member

You need to make a copy of the array if the length does not match:

final byte[] array = request.content().array();
final int length = request.content().length();
if (array.length == length) {
    return array;
} else {
    return Arrays.copyOf(array, length);
}

@hyangtack I'm curious if we need to try to release the HttpData here. What do you think?

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 8, 2019

Contributor

When the content needs to be converted with RequestConverterFunctions, the AnnotatedHttpService aggregates the request from here like the below:

        final CompletableFuture<AggregatedHttpMessage> f =
                aggregationRequired(aggregationStrategy, req) ? req.aggregate()
                                                              : CompletableFuture.completedFuture(null);

So it seems not necessary to release the HttpData because the aggregator (actually AbstractStreamMessage) already did it.

This comment has been minimized.

Copy link
@jongmin92

jongmin92 May 8, 2019

Author Contributor

Thanks for comments. (__)
I have two questions.

  1. Does it mean I don't need to handle when expectedResultType is the HttpData?
  2. I don't know the reason why should I copy of the array if the length does not match.
    More basically, I don't know well why happen the situation that the length does not match.

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 8, 2019

Contributor

I don't know the reason why should I copy of the array if the length does not match.
More basically, I don't know well why happen the situation that the length does not match.

You may refer to here about why we need to check the length.
Actually, the array always has the same length with the content-length because the aggregator aggregates the contents as a single byte array from here.

But we need to follow the API specifications, so it would be better to check the length. The current implementation of ByteArrayRequestConverterFunction also breaks the rule, so please fix it in your PR. :-)

Does it mean I don't need to handle when expectedResultType is the HttpData?

For HttpData, we just need to assign the object reference to the parameter. So I think we don't need to check the length.

@hyangtack

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@jongmin92 Sorry for the late response. :-)

Is it ok the way that tries to convert the content to a JSON object and passing it to the next RequestConverterFunction if it failed?

I think it is okay. And it should be done because we need to convert "text" as a JSON string text.

I think like that. First, To do return as byte[] or HttpData, I should check expectedResultType. But As I understand your comment, Don't I need to check expectedResultType?

You're right. We need to check expectedResultType.
How about handling this as the following way?

  • Call RequestConverterFunction.fallthrough() if the JSON conversion has failed and the expectedResultType is one of HttpData, byte[] or String.
    • For the other types, throw an exception like as it is.
  • Make StringRequestConverterFunction convert the content to String regardless of the content-type.
  • Make ByteArrayRequestConverterFunction convert the content to HttpData or byte[] regardless of the content-type.

In this way, we can convert the content as the expectedResultType regardless of the content-type, so it seems closer to the original issue.

@jongmin92

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@hyangtack
Thanks for the comment :)
I understand it!
I will try to apply this comment ASAP!

@jongmin92 jongmin92 force-pushed the jongmin92:enable_jackson_request_convert_string branch from 4fd1e4d to 50a2b79 May 11, 2019

@jongmin92 jongmin92 force-pushed the jongmin92:enable_jackson_request_convert_string branch from 50a2b79 to e959e61 May 11, 2019

@jongmin92

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@hyangtack
I applied the comments!
Please check once again (__)

@hyangtack
Copy link
Contributor

left a comment

How about adding more tests?

  • Confirm that ByteArrayRequestConverterFunction succeeds to convert JSON text into byte[] or HttpData.
  • Confirm that StringRequestConverterFunction succeeds to convert JSON text into a String. For example, a string of {"a": 1}.
@@ -74,6 +74,11 @@ public Object convertRequest(ServiceRequestContext ctx, AggregatedHttpMessage re
try {
return reader.readValue(content);
} catch (JsonProcessingException e) {
if (expectedResultType == byte[].class ||
expectedResultType == HttpData.class) {

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 13, 2019

Contributor

Don't we need to check String.class, too?

This comment has been minimized.

Copy link
@jongmin92

jongmin92 May 14, 2019

Author Contributor

Oh, I agree with trustin's comment.
Do we need to handle String too?

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 14, 2019

Contributor

Ah, I mean that we need to handle a content like {"a": 1}. It's not a JSON string, it's an object. So it would be failed to convert the content into a string in this converter. Also, it might be handled by the next StringRequestConverterFunction as converting it into a raw string {"a": 1}.

For the case that @trustin explained:
If the content is a JSON string like "value" and the expectedResultType is String.class, the conversion would be succeeded. So I think we don't need to consider the case from here.
And, if I remember correctly, he left the comment because your first code didn't convert a content into a string when the expectedResultType is String.class. It seems a different case.

Does it make sense? Please feel free to leave any question. :-)

if (array.length == length) {
return array;
} else {
return Arrays.copyOf(array, length);

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 13, 2019

Contributor

I think that we need to call Arrays.copyOfRange(array, request.content.offset(), length) because its start offset may or may not be 0 (according to the API description 😆).

@hyangtack
Copy link
Contributor

left a comment

Great job! Left one comment. Thank you, @jongmin92!

@@ -74,6 +74,12 @@ public Object convertRequest(ServiceRequestContext ctx, AggregatedHttpMessage re
try {
return reader.readValue(content);
} catch (JsonProcessingException e) {
if (expectedResultType == byte[].class ||
expectedResultType == HttpData.class ||
expectedResultType == String.class) {

This comment has been minimized.

Copy link
@hyangtack

hyangtack May 15, 2019

Contributor

Oh, we missed CharSequence.class. 😅 It is also handled by StringRequestConverterFunction, so it should be checked from here, too.
And, would you please add the test about that type, too?

@jongmin92

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Thank you for leading me!
Is It also ok these changes?

https://github.com/line/armeria/pull/1745/files#diff-015e919db6e649f771b70d81f440d80cR525

-        // StringRequestConverterFunction will work for the content type of any of 'text'.
+        // StringRequestConverterFunction will work for regardless of the content type.

https://github.com/line/armeria/pull/1745/files#diff-015e919db6e649f771b70d81f440d80cR532

-        // ByteArrayRequestConverterFunction will work for the content type of 'application/octet-stream',
-        // 'application/binary' or none.
+        // ByteArrayRequestConverterFunction will work for regardless of the content type.
if (expectedResultType == HttpData.class) {
return request.content();
if (expectedResultType == byte[].class) {
final byte[] array = request.content().array();

This comment has been minimized.

Copy link
@minwoox

minwoox May 15, 2019

Member

How about extracting request.content() as a variable and reuse it?

@@ -522,15 +522,14 @@ after your request converters, so you don't have to specify any :api:`@RequestCo
@Post("/hello2")
public HttpResponse hello2(MyJsonRequest body) { ... }
// StringRequestConverterFunction will work for the content type of any of 'text'.
// StringRequestConverterFunction will work for regardless of the content type.

This comment has been minimized.

Copy link
@minwoox

minwoox May 15, 2019

Member

work for -> work

@Post("/hello3")
public HttpResponse hello3(String body) { ... }
@Post("/hello4")
public HttpResponse hello4(CharSequence body) { ... }
// ByteArrayRequestConverterFunction will work for the content type of 'application/octet-stream',
// 'application/binary' or none.
// ByteArrayRequestConverterFunction will work for regardless of the content type.

This comment has been minimized.

Copy link
@minwoox

minwoox May 15, 2019

Member

ditto

@minwoox minwoox added this to the 0.86.0 milestone May 15, 2019

@minwoox
Copy link
Member

left a comment

Just a few nits :-)

when(req.content()).thenReturn(HttpData.ofUtf8(JSON_TEXT));

final Object result = function.convertRequest(ctx, req, byte[].class);
Assert.assertTrue(result instanceof byte[]);

This comment has been minimized.

Copy link
@minwoox

minwoox May 15, 2019

Member

Could you use AssertJ instead of TestNG?
assertThat(result).isInstanceOf(byte[].class)

when(req.content()).thenReturn(HttpData.ofUtf8(JSON_TEXT));

final Object result = function.convertRequest(ctx, req, HttpData.class);
Assert.assertTrue(result instanceof HttpData);

This comment has been minimized.

Copy link
@minwoox

minwoox May 15, 2019

Member

ditto

when(req.content(StandardCharsets.UTF_8)).thenReturn(JSON_TEXT);

final Object result = function.convertRequest(ctx, req, JsonRequest.class);
Assert.assertTrue(result instanceof JsonRequest);

This comment has been minimized.

Copy link
@minwoox

minwoox May 15, 2019

Member

ditto

when(req.content(ArmeriaHttpUtil.HTTP_DEFAULT_CONTENT_CHARSET)).thenReturn(JSON_TEXT);

final Object result = function.convertRequest(ctx, req, String.class);
Assert.assertTrue(result instanceof String);

This comment has been minimized.

Copy link
@minwoox

minwoox May 15, 2019

Member

ditto

when(req.content(ArmeriaHttpUtil.HTTP_DEFAULT_CONTENT_CHARSET)).thenReturn(JSON_TEXT);

final Object result = function.convertRequest(ctx, req, CharSequence.class);
Assert.assertTrue(result instanceof CharSequence);

This comment has been minimized.

Copy link
@minwoox

minwoox May 15, 2019

Member

ditto

@minwoox
Copy link
Member

left a comment

Great job, @jongmin92!

@hyangtack

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

@jongmin92 Would you update the PR description for the recent changes? 😃

@jongmin92 jongmin92 changed the title Enable to convert JSON format request to String Enable to convert request body to expected result type regardless of content-type May 16, 2019

@jongmin92

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@hyangtack
I just updated the PR title and description. Could you check it is alright? 😃

@hyangtack

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

@jongmin92 Thank you! It's time to merge this PR! 🎉

@hyangtack hyangtack merged commit a1ea58e into line:master May 16, 2019

4 of 5 checks passed

codecov/patch 53.33% of diff hit (target 71.81%)
Details
Travis CI - Pull Request Build Passed
Details
codecov/project 71.74% (-0.07%) compared to 8b58e26
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@hyangtack

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Thank you for your first contribution, @jongmin92 ! 👍

@jongmin92

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Thanks for leading me! I will try to handle other issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.