Skip to content

Commit

Permalink
Add @ConsumseJson to contents API. (#999)
Browse files Browse the repository at this point in the history
### Motivation:
- When a client sends a request with a JSON body but does not configure the `content-type` in header, Central Dogma returns the response`: {"exception":"java.lang.IllegalArgumentException","message":"No suitable request converter found for a @RequestObject 'CommitMessageDto'"}`. This response is quite ambiguous, making it difficult for the client to understand the problem.


### Modifications:
- Add `@ConsumesJson` to each method that uses `ChangesRequestConverter.class`. 

### FYI
1. If `@RequestConverter` is declared on method signatures, Armeria will add an object Resolver. ([link](https://github.com/line/armeria/blob/e2b298dd2f54243e8801540f13e90166f75578b5/core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L501-L508))
2. Then, when Armeria receives a request from client, Armeria will try to resolve the request.  ([link](https://github.com/line/armeria/blob/e2b298dd2f54243e8801540f13e90166f75578b5/core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L829-L862))
3. `ChangesRequestConverter` tries to parse body from request. Actually, `ChangeRequestConverter` delegates to `JacksonRequestConvertFunction`.
4. `JacksonRequestConverter` validates contents type. ([here](https://github.com/line/armeria/blob/e2b298dd2f54243e8801540f13e90166f75578b5/core/src/main/java/com/linecorp/armeria/server/annotation/JacksonRequestConverterFunction.java#L98-L106))
5. If there is no `content-type` or `content-type` is not `application/json`, `JacksonRequestConverter` will call `RequestConverterFunction.fallthrough()`. 

With this flow, client will receive response `: {"exception":"java.lang.IllegalArgumentException","message":"No suitable request converter found for a @RequestObject 'CommitMessageDto'"}`.

If `@ConsumsJson` is delcared to each method having `ChangesRequestConverter.class` on their method signature, Armeria will not try to resolve request without header `Content-Type: application/json`.


### Result:
- Client will receive 415 response.
```sh
$ curl -X POST localhost:8080/my-post \
-d '{"name": "John Doe", "email": "[email protected]"}'
>>
Status: 415
Description: Unsupported Media Type
```
- Closes #987.
  • Loading branch information
chickenchickenlove authored Aug 13, 2024
1 parent e08333b commit 1ec504c
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import com.linecorp.armeria.common.HttpStatus;
import com.linecorp.armeria.common.util.Exceptions;
import com.linecorp.armeria.server.ServiceRequestContext;
import com.linecorp.armeria.server.annotation.ConsumesJson;
import com.linecorp.armeria.server.annotation.Default;
import com.linecorp.armeria.server.annotation.Get;
import com.linecorp.armeria.server.annotation.Param;
Expand Down Expand Up @@ -187,6 +188,7 @@ private static String normalizePath(String path) {
* <p>Pushes a commit.
*/
@Post("/projects/{projectName}/repos/{repoName}/contents")
@ConsumesJson
@RequiresWritePermission
public CompletableFuture<PushResultDto> push(
ServiceRequestContext ctx,
Expand Down Expand Up @@ -226,6 +228,7 @@ private CompletableFuture<Revision> push(long commitTimeMills, Author author, Re
* <p>Previews the actual changes which will be resulted by the given changes.
*/
@Post("/projects/{projectName}/repos/{repoName}/preview")
@ConsumesJson
public CompletableFuture<Iterable<ChangeDto<?>>> preview(
ServiceRequestContext ctx,
@Param @Default("-1") String revision,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

import com.fasterxml.jackson.databind.JsonNode;

Expand Down Expand Up @@ -407,6 +409,39 @@ void getFile() {
assertThatJson(actualJson).isEqualTo(expectedJson);
}

@ParameterizedTest
@CsvSource({
"application/json, 200",
"text/plain, 415",
"text/xml, 415",
"application/xml, 415",
"application/x-www-form-urlencoded, 415"
})
void getFileWithVariusContentType(String mediaType, int expectedStatusCode) {
// Given:
final WebClient client = dogma.httpClient();
final String body =
'{' +
" \"path\" : \"/foo.json\"," +
" \"type\" : \"UPSERT_JSON\"," +
" \"content\" : {\"a\": \"bar\"}," +
" \"commitMessage\" : {" +
" \"summary\" : \"Add foo.json\"," +
" \"detail\": \"Add because we need it.\"," +
" \"markup\": \"PLAINTEXT\"" +
" }" +
'}';

final RequestHeaders headers = RequestHeaders.of(HttpMethod.POST, CONTENTS_PREFIX,
HttpHeaderNames.CONTENT_TYPE, mediaType);

// When:
final AggregatedHttpResponse aRes = client.execute(headers, body).aggregate().join();

// Then:
assertThatJson(aRes.headers().status().code()).isEqualTo(expectedStatusCode);
}

@Test
void getFileWithJsonPath() {
final WebClient client = dogma.httpClient();
Expand Down

0 comments on commit 1ec504c

Please sign in to comment.