[Bug]: SchemaFilter return properties count zero for subclasses

[coder925]

Describe the bug

From v.6.6.1 an implementation of ISchemaFilter will not return any properties for subclasses:
int propertiesCount = schema.Properties.Count; // 0

This is a regression bug. It works as expected in v6.5.0.

Full implementatoin code of SchemaFilter:

public class MyFilter : ISchemaFilter
{
    public void Apply(OpenApiSchema schema, SchemaFilterContext context)
    {
        var typeName = context.Type.Name;
        Debug.WriteLine($"SchemaFilter called with: {typeName}");

        if (typeName == nameof(CoffeeMachine))
        {
            int propertiesCount = schema.Properties.Count;

            Debug.WriteLine($"Properties count of CoffeeMachine: {propertiesCount}");
            // 0 in Swashbuckle.AspNetCore 6.6.1 - Incorrect
            // 1 in Swashbuckle.AspNetCore 6.5.0 - Correct
        }
    }
}

Expected behavior

Should return 1 for my example:

public class CoffeeMachine : Machine
{
    public string BeanType { get; set; }
}

Actual behavior

Returns 0 for my example:

public class CoffeeMachine : Machine
{
    public string BeanType { get; set; }
}

Steps to reproduce

1. Clone repro repo https://github.com/coder925/swashbuckle-propertiescount
2. Debug project. Add breakpoint in MyFilter.cs.

Exception(s) (if any)

No response

Swashbuckle.AspNetCore version

6.6.1

.NET Version

8.0.303

Anything else?

I have a MarkAllAsRequiredFilter. I detected this issue since all properties of subclasses got optional from v6.6.1.
This was my original code:

    public void Apply(OpenApiSchema model, SchemaFilterContext context)
    {
        var additionalRequiredProps = model.Properties
            .Where(x => !model.Required.Contains(x.Key))
            .Select(x => x.Key);
        foreach (var propKey in additionalRequiredProps)
        {
            model.Required.Add(propKey);
        }
    }

[martincostello]

How is Swashbuckle configured for your app?

There's some specifics documented here for sub-types.

There were also some changes related to subclasses, required behaviour for .NET 8 support and polymorphism since 6.6.x.

Are things still not working the way you'd expect with 6.7.0?

[coder925]

How is Swashbuckle configured for your app?

@martincostello , in my minimal project for reproducing the issue I only have:

builder.Services.AddSwaggerGen(setupAction =>
{
    setupAction.UseAllOfForInheritance();
    setupAction.SchemaFilter<MyFilter>();
});

There's some specifics documented here for sub-types.
There were also some changes related to subclasses, required behaviour for .NET 8 support and polymorphism since 6.6.x.

The code suggested:

   c.SelectSubTypesUsing(baseType =>
    {
        return typeof(Startup).Assembly.GetTypes().Where(type => type.IsSubclassOf(baseType));
    })

did not help unfortunately.

Are things still not working the way you'd expect with 6.7.0?

The problem still exists in 6.7.0.

[martincostello]

Looks like the change in behaviour was caused by #2815.

The question is whether this is a bug due to something missing on that change, or correct behaviour because of that change. I'm still digging into that.

[martincostello]

Repro schema with 6.5.0:

  "components": {
    "schemas": {
      "CoffeeMachine": {
        "type": "object",
        "allOf": [
          {
            "$ref": "#/components/schemas/Machine"
          }
        ],
        "properties": {
          "beanType": {
            "type": "string",
            "nullable": true
          }
        },
        "additionalProperties": false
      },
      "Machine": {
        "type": "object",
        "additionalProperties": false
      }
    }
  }

With 6.7.0:

  "components": {
    "schemas": {
      "CoffeeMachine": {
        "allOf": [
          {
            "$ref": "#/components/schemas/Machine"
          },
          {
            "type": "object",
            "properties": {
              "beanType": {
                "type": "string",
                "nullable": true
              }
            },
            "additionalProperties": false
          }
        ]
      },
      "Machine": {
        "type": "object",
        "additionalProperties": false
      }
    }
  }

[martincostello]

@bkoelman You might know more about this than I do. Is this a bug because something is missing, or is this "as intended" based on the original bug fix and the issue as reported here is depending on a buggy behaviour in a previous version?

[bkoelman]

I'm pretty sure that 6.7.0 is correct. The way I understand the spec:

1. allOf works like a merge, so all properties being merged (ref to base type, plus its own properties) must be inside of it. This includes the property that contains the discriminator value (such as $type)
2. The discriminator definition itself (which indicates which property contains the discriminator value ($type), optionally with a value/schema mapping) exists outside the allOf

Evidence for 1: https://redocly.com/docs/resources/discriminator#common-mistakes
Evidence for 2: https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

See also the full example at https://learn.microsoft.com/en-us/openapi/kiota/models#example-1---using-allof-and-discriminator, which is written by the kiota team. That same team maintains the OpenAPI.NET library that Swashbuckle uses. And they take seat in the OpenAPI standardization committee, so they're pretty likely to get it right. I can confirm that using their sample structure works with both kiota and NSwag C# client generators. Before my PR #2815, neither of them worked.

So to determine the owned properties of a component schema (excluding base properties), the algorithm should be:

• if allOf isn't empty, descent into the last array item to obtain properties
• otherwise: read the properties directly

Hope this clarifies it.

[martincostello]

Thanks I'll read that properly tomorrow.

The bit I'm not sure about is inheritance compared to polymorphism, as the given example for this issue appears to only be inheritance, as there isn't a type discriminator. I see how this makes sense for polymorphism, but it's inheritance that seems less clear to me.

[bkoelman]

In that case, see the first entry at https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/. A discriminator is optional, leaving it out doesn't change the basic rules. Whether using allOf or oneOf, it means that the properties aren't inline but a composition of schemas inside an array. An array entry can define properties inline or ref another schema.

Just as you can't combine ref with inline properties, it doesn't make sense to have to choose between schema A or B, oh and here are some more properties. How are the extra properties supposed to relate to the choice between A or B? If the intent is merging them, wrap the oneOf A/B and the inline properties in an allOf. Alternatively, if the intent is choosing between A/B/extra, all three need to be array elements in a oneOf. This is basic json schema composition.

[coder925]

Thanks for investigating this. If the new behavior is correct, could you please provide guidance on how to adjust my schema accordingly? Specifically, I need to mark all properties as required.

 // The old way: Mark all properties for all classes as Required
 public void Apply(OpenApiSchema model, SchemaFilterContext context)
{
        var additionalRequiredProps = model.Properties
            .Where(x => !model.Required.Contains(x.Key))
            .Select(x => x.Key);
        foreach (var propKey in additionalRequiredProps)
        {
            model.Required.Add(propKey);
        }
}

[martincostello]

Something like this:

public void Apply(OpenApiSchema schema, SchemaFilterContext context)
{
    foreach (var model in schema.AllOf)
    {
        var additionalRequiredProps = model.Properties
            .Where(x => !model.Required.Contains(x.Key))
            .Select(x => x.Key);

        foreach (var propKey in additionalRequiredProps)
        {
            model.Required.Add(propKey);
        }
    }
}

Which outputs this for your repro:

{
  "components": {
    "schemas": {
      "CoffeeMachine": {
        "allOf": [
          {
            "$ref": "#/components/schemas/Machine"
          },
          {
            "required": [
              "beanType"
            ],
            "type": "object",
            "properties": {
              "beanType": {
                "type": "string",
                "nullable": true
              }
            },
            "additionalProperties": false
          }
        ]
      },
      "Machine": {
        "type": "object",
        "additionalProperties": false
      }
    }
  }
}

[martincostello]

Based on @bkoelman's explanation, the behaviour in 6.6.x+ appears to be correct.

That means that unfortunately your code was broken @coder925 because you depended on the bug in previous versions with how this was handled. If you update your filter similar to the example above, you should be back to getting an OpenAPI document for your use case like it was before.