override-with-generic-response shouldn't shallow copy

[Mumeii]

Is your feature request related to a problem? Please describe :

Hi

I'm using v1.6.6 of docapi.

I've noticed that org.springdoc.core.GenericResponseService#getGenericMapResponse is making a shallow copy of the generic responses.

It's bugging me while I'm trying to write a customizer :

Its purpose is to add extra examples entries to the contents of already existing 4xx and 5xx generic entries.

Those generic entries are 500 and 400 ones, automatically build by Springdoc, out of my centralized @ControllerAdvice.

Those default entries are fine to report exceptions that could occur either from :

• 500 : the lower layers, such as database connexion troubles
• 400 : the client side, whenever Spring received a problematic request and throw an exception before the controllers been called

But I also have domaine related exceptions, that are reported in the throw part of my controller's signature.

Because each of those domain exceptions are coming along with a @ResponseStatus, the point of my customizer is to use their associated informations to add extra examples to those 400 and 500 entries, whenever the status code match, of course 😉

But because of the shallow copy, when my customizer add an extra example to one controller endpoint's Content part, it's also adding it to all other ones ... 😛

Do you have a clue how to workaround this trouble ?

Describe the solution you'd like

Either :

• have org.springdoc.core.GenericResponseService#getGenericMapResponse performing a deep copy instead of a shallow one
• or if it's not possible, have an access to a clone function, in order to manage this replication myself in my customizer

Describe alternatives you've considered

None

[bnasslahsen]

@Mumeii,

Can you provide a Minimal, Reproducible Example - with HelloController that reproduces the limitation you are facing ?

[Mumeii]

Hi @bnasslahsen !

Do you have a reference to a sample / sandbox project that I could clone to quickly reproduce my trouble without disclosing my project sources ?

[bnasslahsen]

@Mumeii;

You have so many samples available in the project tests:

• https://github.com/springdoc/springdoc-openapi/tree/master/springdoc-openapi-webmvc-core/src/test/java/test/org/springdoc/api/v30

[Mumeii]

Ha ! Ok, didn't understood what HelloController was all about 😝

I'll try to do it tonight so.

[Mumeii]

Hi @bnasslahsen

Sorry, didn't had time before tonight to make a test proposal

Here is the related PR

As you can see in test 199, the customizer implementation is aiming to feed specific example cases, in each example maps of /first and /second path

But because both share the same Contant object; build out from the @ControlerAdvise; the two specific examples end up been injected in both sides.

From my POV, everything generated from the @ControlerAdvise should be deep cloned first before been injected into a path section.

[bnasslahsen]

@Mumeii,

You can set springdoc.override-with-generic-response=false, and then add your own implementation for these endpoints.

[Mumeii]

@bnasslahsen :

yes, I know, but my goal is to document 60+ endpoints.

That's why having springdoc.override-with-generic-response=true is very convenient for generic cases : I.E. unexpected errors from lower layers library, for example database or network ressources not available, of exceptions raised by spring before the request reach its targeted endpoint's controller, such a validation or conversion exceptions.

My goal writing a Customizer is simply to manage a handful of cases, where the endpoint's signature specify business related exceptions.

In that case I get the annotation placed on those exception classes, in order to add extra examples.

So right now, to have everything working fine, I had to code myself a clone like process in the Customizer, in order to duplicate the generic case entries ... which I think is extra cumbersome 😋

Would make life much easier for users of OperationCustomizer class if a cloned version was directly placed in each content sections.

[bnasslahsen]

@Mumeii,

The problem is that the swagger-core objects are not by default cloneable.
Can you share where you have implemented the clone ?
I have added a fix for it in parrallel

[Mumeii]

@bnasslahsen :

I didn't implemented a clone as is.

Instead, I've made a very custom code, that knit a brand new ApiResponse, taking a bit out of the one passed in parameter to customize methodn and adding some new instances of underlying objects here and there.

Not at all a scholar way to implement a clone so ...

Here is the corresponding code :

import io.swagger.v3.oas.models.Components;
import io.swagger.v3.oas.models.Operation;
import io.swagger.v3.oas.models.examples.Example;
import io.swagger.v3.oas.models.media.Content;
import io.swagger.v3.oas.models.media.MediaType;
import io.swagger.v3.oas.models.media.Schema;
import io.swagger.v3.oas.models.responses.ApiResponse;
import io.swagger.v3.oas.models.responses.ApiResponses;
import lombok.RequiredArgsConstructor;
import org.springdoc.core.customizers.OperationCustomizer;
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Service;
import org.springframework.web.bind.annotation.ResponseStatus;
import org.springframework.web.method.HandlerMethod;

import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Consumer;
import java.util.stream.Stream;

import static io.swagger.v3.core.util.AnnotationsUtils.resolveSchemaFromType;
import static java.lang.String.valueOf;
import static java.util.Arrays.stream;
import static java.util.Optional.ofNullable;
import static org.apache.commons.lang3.StringUtils.isNotBlank;
import static org.springframework.http.MediaType.APPLICATION_JSON;
import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE;

@Service
@RequiredArgsConstructor
public class OperationResponseCustomizer implements OperationCustomizer {

    public static final Schema<?> ERROR_DTO_SCHEMA = resolveSchemaFromType(
        ErrorDto.class,
        new Components(),
        null
    );

    private static boolean hasResponseStatusAnnotation( final Class<?> clazz ) {
        return clazz.getAnnotation( ResponseStatus.class ) != null;
    }

    @Override
    public Operation customize( final Operation operation,
                                final HandlerMethod handlerMethod )
    {
        final ApiResponses apiResponses = operation.getResponses();

        // get from method signature the exceptions that have an associated ResponseStatus annotation :
        final Stream<Class<?>> methodExceptionsWithResponseStatus = stream( handlerMethod
            .getMethod()
            .getExceptionTypes() )
            .filter( OperationResponseCustomizer::hasResponseStatusAnnotation );

        final Consumer<Class<?>> decorateResponse = apiResponseDecorator( apiResponses );

        methodExceptionsWithResponseStatus.forEach( decorateResponse );

        return operation;
    }


    private Consumer<Class<?>> apiResponseDecorator( final ApiResponses apiResponses )
    {
        return exceptionClass ->
        {
            final ResponseStatus rs = exceptionClass.getAnnotation( ResponseStatus.class );

            final Map<String, Example> examples = getExampleMap( exceptionClass, rs );

            final HttpStatus exceptionStatus = rs.value();

            final ApiResponse response = getApiResponse( examples, exceptionStatus );

            final String statusCodeAsString = valueOf( exceptionStatus.value() );

            final Optional<ApiResponse> formerApiResponseOpt = ofNullable(
                apiResponses.put( statusCodeAsString, response ) );

            final Consumer<ApiResponse> mergeIntoNewApiResponse = getIntoNewApiResponseMerger(
                examples,
                response
            );

            formerApiResponseOpt.ifPresent( mergeIntoNewApiResponse );
        };

    }


    private static Consumer<ApiResponse> getIntoNewApiResponseMerger( final Map<String, Example> examples,
                                                                      final ApiResponse newResponse )
    {
        return formerResponse ->
        {
            // keep former response description, as it can come from controller's @ApiResponse :
            newResponse.setDescription( isNotBlank( formerResponse.getDescription() )
                ? formerResponse.getDescription()
                : newResponse.getDescription()
            );

            // we don't provide following fields in current code, so we can
            // take them back from former response as is :
            newResponse.setLinks(       formerResponse.getLinks() );
            newResponse.setHeaders(     formerResponse.getHeaders() );
            newResponse.set$ref(        formerResponse.get$ref() );
            newResponse.setExtensions(  formerResponse.getExtensions() );

            // for content, it's more complicated :

            final Consumer<Content> contentPartMerger = getContentPartMerger( examples, newResponse );

            ofNullable( formerResponse.getContent() )
                .ifPresent( contentPartMerger );
        };
    }


    private static Consumer<Content> getContentPartMerger( final Map<String, Example> examples,
                                                           final ApiResponse newResponse )
    {
        return formerContent -> 
        {
            final Content newContent = newResponse.getContent();

            // we take back as is all content types that are *NOT* APPLICATION_JSON :
            formerContent
                .entrySet()
                .stream()
                .filter( e -> ! APPLICATION_JSON_VALUE.equals( e.getKey() ) )
                .forEach( e -> newContent.put(
                    e.getKey(),
                    e.getValue())
                );

            final Consumer<MediaType> mergeMediaTypePart = getMediaTypePartMerger(
                examples,
                newContent );

            ofNullable( formerContent.get( APPLICATION_JSON_VALUE ) )
                .ifPresent( mergeMediaTypePart );
        };
    }

    private static Consumer<MediaType> getMediaTypePartMerger( final Map<String, Example> examples,
                                                               final Content newContent )
    {
        return formerJsonMediaType ->
        {
            final MediaType mediaType = newContent.get( APPLICATION_JSON_VALUE );

            // Regarding the schema entry, we have nothing to do :
            // in the case of the media type return upon exceptions, only ErrorDto schema
            // is valid, so we can keep the one we've created

            // for the two following fields, as we don't define them in this code
            // we can keep the ones form former json media type :
            mediaType.setExtensions(    formerJsonMediaType.getExtensions() );
            mediaType.setEncoding(      formerJsonMediaType.getEncoding() );

            // if there is a singe example returned by getExample ...
            // there shouldn't be any available in getExamples
            if( formerJsonMediaType.getExample() != null ) {
                mediaType
                    .getExamples()
                    .put(
                        "Generic case",
                        new Example().value( formerJsonMediaType.getExample() )
                    );
            } else {
                // else, maybe they are entries to merge from the getExamples() call :
                ofNullable( formerJsonMediaType.getExamples() )
                    .stream()
                    .map( Map::entrySet )
                    .flatMap( Set::stream )
                    .forEach( entry -> examples.put(
                        entry.getKey(),
                        entry.getValue()
                    ));
            }
        };
    }


    private static ApiResponse getApiResponse( final Map<String, Example> examples,
                                               final HttpStatus exceptionStatus)
    {
        return new ApiResponse()
            .description( exceptionStatus.getReasonPhrase() )
            .content(
                new Content().addMediaType(
                    APPLICATION_JSON.toString(),
                    new MediaType()
                        .schema( ERROR_DTO_SCHEMA )
                        .examples(examples)
                )
            );
    }


    private static Map<String, Example> getExampleMap( final Class<?> exceptionClass,
                                                       final ResponseStatus rs )
    {
        final ErrorDto errorDtoSample = ErrorDto.builder()
            .error( exceptionClass.getSimpleName() )
            .errorDescription( "An error description related to that kind of exception" )
            .build();

        final Example example = new Example().value( errorDtoSample );

        if( isNotBlank( rs.reason() ) )
            example.description( rs.reason() );

        final String exampleLabel = "Upon " + exceptionClass.getSimpleName();

        final Map<String, Example> examples = new LinkedHashMap<>();

        examples.put(
            exampleLabel,
            example
        );

        return examples;
    }

}

[bnasslahsen]

@Mumeii totally agree !
Can you test with with the latest SNAPSHOT and let me know if its working for you?

[Mumeii]

will try tonight

[Mumeii]

yup, the test is running fine when trying it on my side, and as long as they are no huge performance constraints associated to the GenericResponseService#getGenericMapResponse method, the objectMapper.readValue(objectMapper.writeValueAsString()) trick is nice.

Thanks for this evol !