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

change SwaggerIndexTransformer and SwaggerConfig design to ease customization #745

Closed
natrem opened this issue Jun 24, 2020 · 8 comments · Fixed by #755
Closed

change SwaggerIndexTransformer and SwaggerConfig design to ease customization #745

natrem opened this issue Jun 24, 2020 · 8 comments · Fixed by #755
Labels
enhancement New feature or request

Comments

@natrem
Copy link
Contributor

natrem commented Jun 24, 2020

Is your feature request related to a problem? Please describe.
When people want to extend the behavior of Swagger UI transformers (SwaggerIndexTransformer declared in org.springdoc.webmvc.ui.SwaggerConfig and org.springdoc.webflux.ui.SwaggerConfig they have to inherit from your specialized classes instead of just using the provided abstract support class (AbstractSwaggerIndexTransformer).

I think that the design could be improved and the signatures used in SwaggerConfig classes modified to ease work for reuse.

In my case I want to inject css file and js file (used for OIDC) to the document. I created a support class similar to:

public class MySwaggerIndexTransformerSupport extends AbstractSwaggerIndexTransformer {
// other stuff

	public Resource transform(Resource resource, ResourceTransformerChain resourceTransformerChain) {
// I mask the IOException management
// use methods from AbstractSwaggerIndexTransformer 
// execute my own transformation
// return resource
	}
}

And to use it I create a Bean:

/*
 * Keep the parent only to deactivate autoconfiguration from springdoc
 */
public class MySwaggerIndexTransformer extends SwaggerIndexTransformer implements ResourceTransformer {

	private MySwaggerIndexTransformerSupport support;

    public MySwaggerIndexTransformer(SwaggerUiConfigProperties swaggerUiConfig, SwaggerUiOAuthProperties swaggerUiOAuthProperties, ObjectMapper objectMapper) {
        super(swaggerUiConfig, swaggerUiOAuthProperties, objectMapper);
		support = new MySwaggerIndexTransformerSupport(swaggerUiConfig, swaggerUiOAuthProperties, objectMapper);
    }

    @Override
    public Mono<Resource> transform(ServerWebExchange serverWebExchange, Resource resource, ResourceTransformerChain resourceTransformerChain) {
        return Mono.just(support.transform(resource, resourceTransformerChain));
    }

}

Describe the solution you'd like
I have 2 proposals based on this use case.

  1. introduce an interface SwaggerIndexPageTransformer declared in common module and used in SwaggerConfig classes
	@Bean
	@ConditionalOnMissingBean
	SwaggerIndexPageTransformer ...

The inconvenience is that it cannot extend ResourceTransformer since ResourceTransformer exist for servlet and reactive resources. Since transformation only uses the Resource you could declare a reduced signature:
Resource transform(Resource resource);

  1. change AbstractSwaggerIndexTransformer to a support class used by composition instead of inheritance to simplify reuse

With this, the specialized behavior would not have to inherit the current SwaggerIndexTransformer but 'only' need to implement an interface.

Describe alternatives you've considered

  • Instead of introducing a new interface (that would break the current contract of the SwaggerConfig classes you could use a named bean:
@Bean
@ConditionalOnMissingBean(name="swaggerUiIndexPageTransformer")
ResourceTransformer indexPageTransformer(...) {...}

I think this is less readable than the proposed evolution and it also breaks the current contract.

The other possibility I see is to declare the AbstractSwaggerIndexTransformer class in signature (instead of the specialized implementation

Additional context
I'm willing to work on this if you agree on these changes.

@bnasslahsen
Copy link
Contributor

bnasslahsen commented Jun 26, 2020

@natrem,

You can just extend from, one of the following classes depending on your case (webflux or not)

  • org.springdoc.webflux.ui.SwaggerIndexTransformer
  • org.springdoc.webmvc.ui.SwaggerIndexTransformer

And just declare your new class as a Spring Bean. You will be able to add your additional configuration (CSS, ...)
Wouldn't that be enough?

@natrem
Copy link
Contributor Author

natrem commented Jun 26, 2020

@bnasslahsen

it is working indeed.
The point bothering me is that I have to extend a class (SwaggerIndexTransformer) that I do not use.
I add transformations that can be used in both reactive and mvc mode (by different applications).
Same as you I want to factorize the behavior. I used an utility class for this. It extends AbstractSwaggerIndexTransformer to reuse features from it.
The result is that my classes extending an implementation of SwaggerIndexTransformer do not use this inheritance - except to negate your Spring Boot conditions.

This is why I'm not fond of the inheritance and suggest an interface instead:

  • SwaggerIndexPageTransformer extends ResourceTransformer would exist in both servlet and reactive packages.
  • SwaggerIndexTransformer implements SwaggerIndexPageTransformer too
  • the @ConditionalOnMissingBean would be on an interface instead of a class

@bnasslahsen
Copy link
Contributor

@natrem,

When you say:

The result is that my classes extending an implementation of SwaggerIndexTransformer do not use this inheritance - except to negate your Spring Boot conditions.

You override the transform i suppose. And this this one of the advantages of the inheritance.
I am not sure its completely true, when you say that you don't use inheritance.

  • What will be the content of the interface: SwaggerIndexPageTransformer ?

@natrem
Copy link
Contributor Author

natrem commented Jun 26, 2020

@bnasslahsen,
I override transform as I would implement it on an interface.
I say I don't use inheritance because I do not call super.

The implementation class is:

public class MySwaggerIndexTransformer extends SwaggerIndexTransformer implements ResourceTransformer {
  private MySwaggerIndexTransformerSupport support;
  public MySwaggerIndexTransformer(SwaggerUiConfigProperties swaggerUiConfig, SwaggerUiOAuthProperties swaggerUiOAuthProperties, ObjectMapper objectMapper, MyOptions options) {
        super(swaggerUiConfig, swaggerUiOAuthProperties, objectMapper);
	support = new MySwaggerIndexTransformerSupport(swaggerUiConfig, swaggerUiOAuthProperties, objectMapper, options);
  }
  @Override
  public Mono<Resource> transform(ServerWebExchange serverWebExchange, Resource resource, ResourceTransformerChain resourceTransformerChain) {
        return Mono.just(support.transform(resource, resourceTransformerChain));
    }
}

This class only delegates to the support class:

// extend to use methods from abstract class
public class MySwaggerIndexTransformerSupport extends AbstractSwaggerIndexTransformer {
	private final MyOptions options;
	public MySwaggerIndexTransformerSupport(SwaggerUiConfigProperties swaggerUiConfig, SwaggerUiOAuthProperties swaggerUiOAuthProperties, ObjectMapper objectMapper, MyOptions options) {
		super(swaggerUiConfig, swaggerUiOAuthProperties, objectMapper);
		this.options = options;
	}

	public Resource transform(Resource resource, ResourceTransformerChain resourceTransformerChain) {
		try {
			return transformNoCheck(resource);
		} catch (Exception e) {
			throw new SpringDocUIException("Failed to transform Index", e);
		}
	}

	// does not handle exception to ease readability
	private Resource transformNoCheck(Resource resource) throws IOException {
		// first we execute code from super class (same result as the current SwaggerIndexTransformer)
		final AntPathMatcher antPathMatcher = new AntPathMatcher();
		boolean isIndexFound = antPathMatcher.match("**/swagger-ui/**/index.html", resource.getURL().toString());
		if (isIndexFound) {
			String html = readFullyAsString(resource.getInputStream());

			if (swaggerUiConfig.isDisableSwaggerDefaultUrl()) {
				html = overwriteSwaggerDefaultUrl(html);
			}
			if (!CollectionUtils.isEmpty(swaggerUiOAuthProperties.getConfigParameters())) {
				html = addInitOauth(html);
			}
			// my methods for customization: add css, js script and change page title
			// these methods are not displayed here
			html = headAppender.appendToHead(html);
			html = overwriteTitle(html);

			return new TransformedResource(resource, html.getBytes());
		} else {
			return resource;
		}
	}

I first replicate the behavior of implementations of SwaggerIndexTransformer (although written a bit differently) then I apply my own transformations.
I could call super.transform from MySwaggerIndexTransformer but that would mean I read the resulting Resource once again. I feel it's more efficient to apply all transfirmations at once.

When I say I don't use inheritance that means I implement the method inherited from ResourceTransformer without calling the parent implementation. I use the inheritance advantage in the support class MySwaggerIndexTransformerSupport but not in MySwaggerIndexTransformer. Is it more clear?

To answer your last question, SwaggerIndexPageTransformer would simply exist to have a type more precise than
ResourceTransformer (to ease @ConditionalOnBean usage):

interface SwaggerIndexPageTransformer extends ResourceTransformer { 
// no other method
}

With this the condition would be:

	@Bean
	@ConditionalOnMissingBean
	SwaggerIndexPageTransformer indexPageTransformer(...) {...}

@bnasslahsen
Copy link
Contributor

Inheritance, is not only about the call of super. There are other advantages / benefits of inheritance (Extensibility indeed).

I understand, that additionnally, you are proposing to add two marker(empty) interfaces (one for webflux and another for webmvc).

In the existing implementation, if a user wants to add his own, he can simply override SwaggerIndexTransformer, and decide or not to benefit from the other methods in AbstractSwaggerIndexTransformer.

This is a visual explanation of the existing implementation.

swagger-index-transformer-design

If you can do the same, this will make your proposal much clear, about what are the advantages, the impacts on the springdoc-ui and also what will an application need to do to override with its own implemenation.

I have used online.visual-paradigm , but you can use any UML tool you feel confortable with to describe your proposal.

@natrem
Copy link
Contributor Author

natrem commented Jun 29, 2020

@bnasslahsen thanks for taking the time to discuss my idea (I'm sure you have plenty on your plate already).

I did a similar job with the extensions I propose:
image

If people want to customize only webmvc or webflux (because they use only one) then they could extend the class you offer.
If they want to customize both they will surely create an abstract class to factorize intelligence (exactly as you did yourself). This class would logically inherit from AbstractSwaggerIndexTransformer.
The main advantage of the add interface (not simply a marker since it extends ResourceTransformer) is that it does not force to extend your concrete implementation.

I would be tempted to rename the classes and call the interface SwaggerIndexTransformer but that would cause much unnecessary impact.
In my mind the impact (in the code) is limited to adding the two interfaces and use them in the autoconfiguration classes

@bnasslahsen
Copy link
Contributor

Hi @natrem,

I see your point. This makes your enhancement much clear.
You can propose a PR in this sense.
Thanks for your contribution.

@natrem
Copy link
Contributor Author

natrem commented Jun 29, 2020

As they say a picture is worth a thousand words.
Sorry for the lack of initial clarity and loss of time.

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

Successfully merging a pull request may close this issue.

2 participants