Skip to content
This repository was archived by the owner on Mar 16, 2025. It is now read-only.

better response type handling #68

Closed
hauner opened this issue May 28, 2021 · 6 comments
Closed

better response type handling #68

hauner opened this issue May 28, 2021 · 6 comments
Labels
enhancement New feature or request
Milestone

Comments

@hauner
Copy link
Member

hauner commented May 28, 2021

If an api has multiple response content types (success & errors) the java response type of the controller method will be Object to allow return values with different java types.

If the errors are handled by throwing an exception there is no need to have an Object response and the controller method could use the type of the success response to improve readability of the interface.

mapping could be like this, using a globally applied option:

# mapping.yaml
openapi-processor-mapping: v2

options:
  response-type: all    # all (default) or success

or like this, allowing endpoint & endpoint/method configuration:

# mapping.yaml
openapi-processor-mapping: v2

map:
  response-type: all    # all (default) or success

all: consider all response types to select the method return type
success: consider only the success response type, i.e. usually 200

@hauner hauner added the enhancement New feature or request label May 28, 2021
@hauner hauner added this to the next milestone May 28, 2021
@JonasGroeger
Copy link

JonasGroeger commented May 29, 2021

In the Spring module this is also annoying since even if you put

map:
  result: org.springframework.http.ResponseEntity

it will generate ResponseEntity<?> and not ResponseEntity<SuccessResponse>.

Could you give me some hints on how you would implement it? I'm willing to write an implementation.

Reference: https://docs.openapiprocessor.io/spring/2021.3/mapping/result.html

hauner added a commit that referenced this issue May 30, 2021
@hauner
Copy link
Member Author

hauner commented May 30, 2021

Yes, I think that has the same cause.

Thanks for the offer to help -)

Here are a few notes to get you started.

I did a few lines on this to get the new option but that's just a minimal start that does not yet do anything. It is on this branch: https://github.com/openapi-processor/openapi-processor-core/tree/response-type-handling

Current idea is to replace the return type of the controller method in the MethodWriter here:

| ${endpointResponse.responseType} ${createMethodName (endpoint, endpointResponse)}(${createParameters(endpoint)});

This is the line that adds the response type to the method. Replacing ${endpointResponse.responseType} with a method that does select the return type based on the new option should solve the issue. If the new option is all use the current behaviour if it is success use the success response type and ignore the error responses.

The code to get the wanted type exists and is currently private in the EndpointResponse:

Note that the identical selection is required to create the import list.

Getting that option is probably the harder part. Converting the mapping.yaml to the internal format requires a couple of steps and is more complex than I like.

If the options is configured like this

# mapping.yaml
openapi-processor-mapping: v2

map:
  response-type: all    # all (default) or success

the MethodWriter will need the MappingFinder to get access to it and then call a similiar function to

to get the value of new mapping option.

The maps: part of the mapping.yaml is converted internally to Mappings. The MappingFinder uses them to find & select mappings.

The new option will need a new implementation of the Mapping interface: https://github.com/openapi-processor/openapi-processor-core/blob/response-type-handling/src/main/kotlin/io/openapiprocessor/core/converter/mapping/Mapping.kt

Converting the yaml to the Mapping happens here:
https://github.com/openapi-processor/openapi-processor-core/blob/response-type-handling/src/main/kotlin/io/openapiprocessor/core/processor/mapping/v2/MappingConverter.kt

I hope this helps and i didn't miss any annoying detail that breaks this approach :)

@hauner
Copy link
Member Author

hauner commented May 30, 2021

collecting the import happens here

private fun collectImports(packageName: String, endpoints: List<Endpoint>): List<String> {

hauner added a commit that referenced this issue Jun 5, 2021
hauner added a commit that referenced this issue Jun 5, 2021
hauner added a commit that referenced this issue Jun 5, 2021
hauner added a commit that referenced this issue Jun 5, 2021
hauner added a commit that referenced this issue Jun 5, 2021
hauner added a commit that referenced this issue Jun 5, 2021
@hauner
Copy link
Member Author

hauner commented Jun 6, 2021

@JonasGroeger Hi, I have it nearly running.... I hope you didn't waste to much on on it.

hauner added a commit that referenced this issue Jun 6, 2021
hauner added a commit that referenced this issue Jun 6, 2021
@hauner hauner modified the milestones: next, 2021.5 Jun 6, 2021
@JonasGroeger
Copy link

JonasGroeger commented Jun 6, 2021

@JonasGroeger Hi, I have it nearly running.... I hope you didn't waste to much on on it.

Please don't feel obligated to implement something just because somebody wants it. It's open source after all.

In case you didnt: all good ;)

@hauner
Copy link
Member Author

hauner commented Jun 6, 2021

It something I like to see myself :-) A colleague mentioned it and then your comment on the issue. Funny is, that it all happend in a couple of days. ;-)

hauner added a commit that referenced this issue Jul 4, 2021
hauner added a commit that referenced this issue Jul 4, 2021
hauner added a commit that referenced this issue Jul 4, 2021
@hauner hauner closed this as completed Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants