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

Update OpenAPI Spec #84

Merged

Conversation

alexandre-touret
Copy link
Contributor

fix #69

@arey
Copy link
Member

arey commented Jan 31, 2022

The build has failed

@alexandre-touret
Copy link
Contributor Author

I saw that. I will fix that tomorrow

@alexandre-touret
Copy link
Contributor Author

Hi,
I have an issue and a question about the api documentation and the generated code:

When I put this parameter in the maven plugin configuration:

<generateApis>true</generateApis>

I have the template of the APIs generated in the target/generated-sources/openapi/src/main/java/org/springframework/samples/petclinic/rest/api folder/package.

The generated interfaces provide the default methods which should be implemented in the source code .
You could find here an example :

@javax.annotation.Generated(value = "org.openapitools.codegen.languages.SpringCodegen", date = "2022-02-02T18:56:32.827+01:00[Europe/Paris]")
@Validated
@Api(value = "pettypes", description = "the pettypes API")
public interface PettypesApi {

    default Optional<NativeWebRequest> getRequest() {
        return Optional.empty();
    }

    @ApiOperation(value = "Create a pet type", nickname = "addPetType", notes = "Creates a pet type .", response = PetTypeDto.class, tags={ "pettypes", })
    @ApiResponses(value = { 
        @ApiResponse(code = 200, message = "Pet type created successfully.", response = PetTypeDto.class),
        @ApiResponse(code = 304, message = "Not modified."),
        @ApiResponse(code = 400, message = "Bad request.", response = RestErrorDto.class),
        @ApiResponse(code = 404, message = "Owner not found.", response = RestErrorDto.class),
        @ApiResponse(code = 500, message = "Server error.", response = RestErrorDto.class) })
    @RequestMapping(
        method = RequestMethod.POST,
        value = "/pettypes",
        produces = { "application/json" },
        consumes = { "application/json" }
    )
    default ResponseEntity<PetTypeDto> addPetType(@ApiParam(value = "The pet type" ,required=true )  @Valid @RequestBody PetTypeDto petTypeDto) {
        getRequest().ifPresent(request -> {
            for (MediaType mediaType: MediaType.parseMediaTypes(request.getHeader("Accept"))) {
                if (mediaType.isCompatibleWith(MediaType.valueOf("application/json"))) {
                    String exampleString = "null";
                    ApiUtil.setExampleResponse(request, "application/json", exampleString);
                    break;
                }
            }
        });
        return new ResponseEntity<>(HttpStatus.NOT_IMPLEMENTED);

    }

Unfortunately, in the current code, all the posts methods in the implemented controllers (e.g., PetTypesController) inject both the BindingResult and the UriComponentBuilder instances

For instance:

    @PreAuthorize("hasRole(@roles.VET_ADMIN)")
    @RequestMapping(value = "", method = RequestMethod.POST, produces = "application/json")
    public ResponseEntity<SpecialtyDto> addSpecialty(@RequestBody @Valid SpecialtyDto specialtyDto, BindingResult bindingResult, UriComponentsBuilder ucBuilder)

⚠️ Now I have two ways for fixing this issue :

  • I cancel the API default interfaces generation during the build time. ( the easy solution but we loose some of the benefits of the API First approach)
  • I remove the injection of BindingResult and UriComponentBuilder in the controllers. I have to look around in the documentation to get a "clean" solution for that. BTW, I don't know if it's possible .

❓ Tell me your opinion about this concern.

Regards

@arey
Copy link
Member

arey commented Feb 3, 2022

I see the problem. I prefer the 2nd solution.

We could use the UriComponentsBuilder.newInstance() to avoid the ucBuilder parameter.

I'm not sure that the bindingResult parameter is required:

  1. I'm pretty sure that if a binding error is found upstream by Spring MVC, the addSpecialty method is not called. Could you check it? Thus the bindingResult.hasErrors() test could be removed.
  2. The second test condition (specialtyDto == null) could be replace by an exception and a ExceptionHandler.

@alexandre-touret
Copy link
Contributor Author

Hi,
I found a clever workaround for the first point which is in my opinion better .

I can :

  • Centralize the error management in a ControllerAdvice
  • Get the binding result and apply the same logic

What do you think about that ?

@arey
Copy link
Member

arey commented Feb 10, 2022

Sure, great idea, the ExceptionHandler I referenced could take place into a ControllerAdvice

@alexandre-touret
Copy link
Contributor Author

Hi @arey
I think it's ready.
Could you review this PR please?

Regards

@alexandre-touret
Copy link
Contributor Author

alexandre-touret commented Feb 15, 2022

Hi,
I fixed all the raised issues.

@arey arey merged commit a7de099 into spring-petclinic:master Feb 16, 2022
@arey
Copy link
Member

arey commented Feb 16, 2022

Thanks a lot @alexandre-touret

@alexandre-touret alexandre-touret deleted the feature/openapi_spec_completion branch February 16, 2022 12:49
@alexandre-touret
Copy link
Contributor Author

You're welcome ;-)

haraldreingruber-dedalus pushed a commit to haraldreingruber-dedalus/spring-keycloak-angular-exercise that referenced this pull request Sep 22, 2022
…ot/npm_and_yarn/follow-redirects-1.14.7

Bump follow-redirects from 1.14.0 to 1.14.7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complete the Petclinic OpenAPI spec
2 participants