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

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

Open
ikhoon opened this issue Jul 18, 2024 · 6 comments · Fixed by #999
Open

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

ikhoon opened this issue Jul 18, 2024 · 6 comments · Fixed by #999
Assignees
Labels

Comments

@ikhoon
Copy link
Contributor

ikhoon commented Jul 18, 2024

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.

* <p>Pushes a commit.
*/
@Post("/projects/{projectName}/repos/{repoName}/contents")
@RequiresWritePermission
public CompletableFuture<PushResultDto> push(

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 ikhoon added the defect label Jul 18, 2024
@chickenchickenlove
Copy link
Contributor

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

@ikhoon
Copy link
Contributor Author

ikhoon commented Jul 18, 2024

Sure. Thanks for your interest!

@ikhoon
Copy link
Contributor Author

ikhoon commented Aug 16, 2024

#999 didn't handle all API. For example, we still need to specify @Consume to the below.

*/
@Post("/projects")
@StatusCode(201)
@ResponseConverter(CreateApiResponseConverter.class)
public CompletableFuture<ProjectDto> createProject(CreateProjectRequest request, Author author) {

@ikhoon ikhoon reopened this Aug 16, 2024
@chickenchickenlove
Copy link
Contributor

try {
final ResponseHeadersBuilder builder = headers.toBuilder();
if (builder.contentType() == null) {
builder.contentType(MediaType.JSON_UTF_8);
}
final JsonNode jsonNode = Jackson.valueToTree(resObj);
if (builder.get(HttpHeaderNames.LOCATION) == null) {
final String url = jsonNode.get("url").asText();
// Remove the url field and send it with the LOCATION header.
((ObjectNode) jsonNode).remove("url");
builder.add(HttpHeaderNames.LOCATION, url);
}

Sorry, i missed it.
May i create new PR to fix it?

@chickenchickenlove
Copy link
Contributor

@ikhoon nim,
#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.

*/
@Post("/projects")
@StatusCode(201)
@ResponseConverter(CreateApiResponseConverter.class)
public CompletableFuture<ProjectDto> createProject(CreateProjectRequest request, Author author) {

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?

@ProducesJson
@RequiresReadPermission
@RequestConverter(CommitMessageRequestConverter.class)
public class ContentServiceV1 extends AbstractService {

@ikhoon
Copy link
Contributor Author

ikhoon commented Aug 30, 2024

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

CreateProjectRequest will be converted by the global JacksonRequestConverterFunction.

new JacksonRequestConverterFunction(new ObjectMapper()),

If a content-type is not specified, JacksonRequestConverterFunction won't convert the body into CreateProjectRequest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants