centraldogma icon indicating copy to clipboard operation
centraldogma copied to clipboard

Add `@Consume(content-type)` to all REST services class or methods

Open ikhoon opened this issue 1 year ago • 6 comments

All Central Dogma APIs use JSON (Patch) to exchange data. A Central Dogma server can't handle other types such as XML or YAML.

Currently, the request content-type is not specified in some API methods. https://github.com/line/centraldogma/blob/24603eb149d3945fde2cc1ced71a9efdcbc231fe/server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java#L187-L191 That results in generating a converter not found error message if content-type is missing. For example, a commit is sent without content-type, No suitable request converter is returned from which users don't know what is wrong.

$ curl -XPOST "http://127.0.0.1:36462/api/v1/projects/foo/repos/bar/contents" \
 -H "Authorization: Bearer appToken-***" \
 -d '{
    "commitMessage" : {
        "summary": "hello",
        "detail": "a",
        "markup": "MARKDOWN"
    },
    "changes" : [
        {
            "path" : "/foo0.json",
            "type" : "UPSERT_JSON",
            "content" : {"a": "bar2"}
        }
    ]
 }'
{"exception":"java.lang.IllegalArgumentException","message":"No suitable request converter found for a @RequestObject 'CommitMessageDto'"}%

If @ConsumeJson is added to the method, 405 Method Not Allowed is returned which would make it easier to identify the cause.

ikhoon avatar Jul 18 '24 09:07 ikhoon

@ikhoon nim, i want to take this task! Could you assign this task to me?

chickenchickenlove avatar Jul 18 '24 09:07 chickenchickenlove

Sure. Thanks for your interest!

ikhoon avatar Jul 18 '24 10:07 ikhoon

#999 didn't handle all API. For example, we still need to specify @Consume to the below. https://github.com/line/centraldogma/blob/a4e58931ac98e8b6e9e470033ba04ee60180b135/server/src/main/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1.java#L137-L141

ikhoon avatar Aug 16 '24 08:08 ikhoon

https://github.com/line/centraldogma/blob/a4e58931ac98e8b6e9e470033ba04ee60180b135/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/CreateApiResponseConverter.java#L53-L66 Sorry, i missed it. May i create new PR to fix it?

chickenchickenlove avatar Aug 16 '24 08:08 chickenchickenlove

@ikhoon nim, https://github.com/line/centraldogma/pull/999 didn't handle all API. For example, we still need to specify @Consume to the below.

It seems that this code you mentioned is not about @ConsumesJson. 🤔 Since in this code, @ResponseConverter(not @RequestConverter) is used. Thus, i think it is only related to @Produces. https://github.com/line/centraldogma/blob/a4e58931ac98e8b6e9e470033ba04ee60180b135/server/src/main/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1.java#L137-L141

By the way, i took a look at more, i found this code.(@RequestConverter(CommitMessageRequestConverter.class).) From this code, should we assume that all API in ContentServiceV1 require Content-type: json?

https://github.com/line/centraldogma/blob/e78a569541f6e010ad6e5f985679687cfe50bbfc/server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java#L101-L104

chickenchickenlove avatar Aug 25 '24 08:08 chickenchickenlove

Central Dogma only uses JSON in REST APIs for the content-type.

CreateProjectRequest will be converted by the global JacksonRequestConverterFunction. https://github.com/line/centraldogma/blob/534f6bf8e6e9918d2fcf878612518bd4f25aeee7/server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java#L775 If a content-type is not specified, JacksonRequestConverterFunction won't convert the body into CreateProjectRequest.

ikhoon avatar Aug 30 '24 09:08 ikhoon