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

HDOT-1743 Allow to expose polymorphic types in Swagger API #2063

Merged
merged 2 commits into from
Oct 8, 2020

Conversation

avas
Copy link
Contributor

@avas avas commented Oct 7, 2020

Problem

Currently, there is no way to expose derived types in Swagger API description. For example, consider the following code:

// Models definition in ...Core assembly

public abstract class BaseObject
{
    // ...
}

public class DerivedObject : BaseObject
{
    // ...
}

public class AnotherDerivedObject : BaseObject
{
    // ...
}

// Controller method that returns these models
public ActionResult<IList<BaseObject>> GetObjects()
{
    var result = new[] 
    {
        new DerivedObject(),
        new AnotherDerivedObject()
    };

    return Ok(result);
}

In this example, only the BaseObject will be exposed in Swagger. DerivedObject and AnotherDerivedObject will be missing from JSON with API definition, and the only way to expose them is to add some other API method that will explicitly accept or return any of these derived types.

On the other hand, we don't want to enable polymorphism globally, as this might break existing API (e.g. any action from vc-module-customer that works with a Member class).

Solution

Since the creation of VC platform v3, polymorphism support in Swashbuckle improved significantly. Now we actually can use it in actions, and the resulting document will be suitable for AutoRest (e.g. to generate API clients for storefront). An example mentioned above can be reworked like this to expose derived models:

using Swashbuckle.AspNetCore.Annotations;

[SwaggerSubType(typeof(DerivedObject)]
[SwaggerSubType(typeof(AnotherDerivedObject)]
public abstract class BaseObject
{
    // ...
}

public class DerivedObject : BaseObject
{
    // ...
}

public class AnotherDerivedObject : BaseObject
{
    // ...
}

This will expose BaseObject, DerivedObject and AnotherDerivedObject in Swagger API description (despite the fact that GetObjects() method still has only the base type in its signature), and it won't break other API. However, to use this approach, Swashbuckle configuration code needs to be changed a bit.

Proposed of changes

So, I did the following things to achieve this:

  1. Installed the Swashbuckle.AspNetCore.Annotations package;
  2. Enabled annotation-based control of inheritance and polymorphism. Parameters for the EnableAnnotations() method have the following meaning:
    • enableAnnotationsForPolymorphism allows to use the SwaggerSubType attribute to specify expected derived classes;
    • enableAnnotationsForInheritance retains models inheritance hierarchy. For example, if we generate a Swagger API documentation for the example mentioned above with this parameter set to false, it will not contain the BaseObject type - instead, DerivedObject and AnotherDerivedObject will include its properties. With this parameter set to true, the hierarchy will be preserved.

Additional context (optional)

This approach works and does not break any existing clients generated by AutoRest. However, it has some limitations:

  1. If BaseObject has any abstract or virtual properties that are overridden in derived types, Swashbuckle will include these properties both to BaseObject and to derived types. However, AutoRest does not understand that and produces an error like FATAL: System.InvalidOperationException: Found incompatible property types , for property 'someVirtualProperty' in schema inheritance chain. A solution for this would be to avoid using abstract and virtual properties for such types.
  2. This approach only works if all of derived types are located in the same module - it won't allow to extend this list from other modules. To overcome this, we might need to make a custom sub-type selector - its code would be based on the existing implementation in Swashbuckle, but use AbstractTypeFactory instead of custom attributes to find descendant types. However, this might require extending the TypeInfo, so that we could explicitly specify what types can be exposed in Swagger API.

Make sure these boxes are checked:

@mvktsk
Copy link
Contributor

mvktsk commented Oct 7, 2020

Review task https://virtocommerce.atlassian.net/browse/VP-5447 has been created

@avas avas changed the title HDOT-1753 Allow to expose polymorphic types in Swagger API HDOT-1743 Allow to expose polymorphic types in Swagger API Oct 7, 2020
Copy link
Contributor

@akak1977 akak1977 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested for side effects by comparing cumulative swagger-documents (with/without changes).
They are equal.

@akak1977 akak1977 merged commit 591a212 into dev Oct 8, 2020
@akak1977 akak1977 deleted the HDOT-1753-allow-polymorphism-in-swagger branch October 8, 2020 06:45
@sonarcloud
Copy link

sonarcloud bot commented Oct 8, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_265) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

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

Successfully merging this pull request may close these issues.

None yet

5 participants