fix: don't crash when trying to add an already registrer type

[Angelinsky7]

This is a naive pr to try to solve #2679
all tests pass (but the one that was already failing before my change)
i would gladly add a test to cover this, but i would need some hints to know were to begin.

[martincostello]

Thanks for contributing - if you'd like to continue with this pull request, please rebase against the default branch to pick up our new CI. Then we can look at some suggestions to help you add a test.

[Angelinsky7]

@martincostello hello, thanks for your answer... i did a rebase against master

[martincostello]

Please add a unit test for this fix.

[martincostello]

Suggested change

	if(conflictingType != type)
	if (conflictingType != type)

[Angelinsky7]

@martincostello ok, what's the best course of action ? Do i create a new WebSites with only the problematic endpoint (that should not crash anymore) ? Do i add a new endpoint / controller (in that case in which WebSites, Projects ? )

Thanks for your help.

[martincostello]

The lowest-effort approach (hopefully) should be to add to one of the existing SwaggerGen tests that declares an endpoint in code and then checks the behaviour of what was generated. If you add a test that throws an exception without your change, then passes after it's made with the correct eventual behaviour should make us happy from a regression testing point of view 😄

I would consider adding a new project a "last resort" as they're relatively heavyweight and are typically used for end-to-end scenarios, rather than testing specific scenarios.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.69%. Comparing base (0108842) to head (9e72993).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2695   +/-   ##
=======================================
  Coverage   91.69%   91.69%           
=======================================
  Files          91       91           
  Lines        3022     3023    +1     
  Branches      519      520    +1     
=======================================
+ Hits         2771     2772    +1     
  Misses        251      251           

Flag	Coverage Δ
Linux	91.69% <100.00%> (+<0.01%)	⬆️
Windows	91.69% <100.00%> (+<0.01%)	⬆️
macOS	90.34% <100.00%> (-1.36%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Angelinsky7]

@martincostello so.... after struggling to create a unit test to reproduce the issue i've finally discovered the real issue.... (and i need your input for this)

So the culprit is Asp.Versioning.OData.ApiExplorer (in my case) that is building a dynamic proxy type over some (no all) types....
for example, when using a Type MyType in a parameter and the same Type in another parameter like List<MyType> or Dictionnary<int, MyType>....
But sometimes for no reason at all (that i can understand) it also build a dynamic proxy type for normal type in different context.

So, the type are effectively the same BUT not in the same Assembly ! So it's why i ended to have this exception : MyType already used...(and it's a correct error)

In the TryLookupByType function, the TryGetValue call is matching a real type and correctly don't find the Namespace.MyType IN DynamicAssembly because there is only the Namespace.MyType IN StaticAssembly already registered. But later in RegisterType when checking for schemaId with Namespace.MyType it find it again and crash.

Of course, my CustomSchemaIds config was (a more complex version of) options.CustomSchemaIds(type => type.FullName);
if i'm changing this to options.CustomSchemaIds(type => type.AssemblyQualifiedName); nothing crash !

But...

Now, i've got duplicate types (and ugly and unusable names)....

with this method :

public String GetSchemaId(Type modelType) {
    String id = DefaultSchemaIdSelector(modelType);

    if (!_schemaNameRepetition.ContainsKey(id)) {
        _schemaNameRepetition.Add(id, new List<String>());
    }

    List<String> modelNameList = _schemaNameRepetition[id];
    String fullName = modelType.AssemblyQualifiedName ?? "";
    if (!String.IsNullOrEmpty(fullName) && !modelNameList.Contains(fullName)) {
        modelNameList.Add(fullName);
    }

    Int32 index = modelNameList.IndexOf(fullName);

    return $"{id}{(index >= 1 ? index.ToString() : "")}";
}

i could save myself... but now i've got MyType and MyType1.... and they are exactly the same !

---

So now the question ? Is this something ok for you ? I would argue that the same Namespace.Type in 2 different Assembly ARE NOT the same type... but in this case i know there are the same and that one is just a copy of the original one with the same exact properties....

So i would like to say that RegisterType should not crash in this case.... and i understand why Havunen/DotSwashbuckle2 implemented this the way it did :

 public void RegisterType(Type type, string schemaId)
        {
if (_reservedIds.ContainsValue(schemaId))
            {
                var conflictingType = _reservedIds.First(entry => entry.Value == schemaId).Key;

                throw new InvalidOperationException(
                    $"Can't use schemaId \"${schemaId}\" for type \"${type}\". " +
                    $"The same schemaId is already used for type \"${conflictingType}\"");
            }
}

 // TODO: This method could be optimized by comparing schemas without serializing them to JSON
        // But it should not be a big deal since it's only called when there's a conflict
        private static bool IsSchemaEqual(OpenApiSchema a, OpenApiSchema b)
        {
            var aJson = SerializeSchemaAsString(a);
            var bJson = SerializeSchemaAsString(b);

            return string.Equals(aJson, bJson, StringComparison.Ordinal);
        }

        private static string SerializeSchemaAsString(OpenApiSchema a)
        {
            using var textWriter = new StringWriter(CultureInfo.InvariantCulture);
            var jsonWriter = new OpenApiJsonWriter(textWriter);
            a.SerializeAsV3(jsonWriter);
            return textWriter.ToString();
        }

but i really don't like this, and it make me think that there is something not correct...

The sad thing, is either way the patch does correctly what it does: not crashing for the same type, but it could also not crash in some case were it should have...

So the question: Do we want to consider the two Namespace.MyType the same if they live in 2 different assemblies ? (loaded at the same time, which seem to me really hard to get in another situation than this one) and if not do you have an idea how to solve this differently ?
How can we handle this situation when we have types mixed with dynamic proxy types (like with entityframework proxies for example or in my case Asp.Versioning.OData.ApiExplorer proxies ???

Thanks for your time and support ! (i hope i was clear enough)

[martincostello]

Thanks for the analysis @Angelinsky7.

I think out-of-the-box we'd probably want it to behave the way it does, which is to treat different types as different 😆

However, it would also be good if there were a way for someone in your situation to be able to override the behaviour somehow so that if you really want to, you can get the behaviour your want through customisation.

@commonsensesoftware - any comments to add here regarding how API versioning comes into play?

[Angelinsky7]

We could have something like CustomSchemaIds for the TryLookupByType and we maybe could customize the TryGetValue call.

Like that, instead of being forced to have exactly the same type, we could have a custom method to find the correct type instead of the "proxy" one.

If @martincostello you're ok with the idea i could try to propose something in this PR.

[commonsensesoftware]

First up, I agree with @martincostello. We don't want to hide duplicate types; it's a mistake. OpenAPI isn't meant to represent the concept of a full-qualified name or namespace as not all languages support that. The name of the type and the model it represents in the API should be unique and stand on its own.

The API Explorer extensions for API Versioning with OData does something special out of convenience. This feature is referred to as Model Substitution. OData uses an Entity Data Model (EDM) to define what a model truly looks like over the wire for an API. This is not necessarily a 1:1 mapping of the CLR-defined type. In support of API Versioning, there is a single EDM per API version. It is commonplace to define a single CLR type, but map/expose a different or limited set of data members in the EDM for a particular API version. This provides a great convenience for OData users to define a single model type that can be used in multiple scenarios and API versions. Most OpenAPI generators, like Swashbuckle, derive the model definition solely by type. In non-OData scenarios, this currently requires you to create a different type per API version, which is kind of lame.

Since the EDM defines what will be transmitted over the wire, the API Explorer extensions use the EDM to define the model for you. The way that this works is that it compares the EDM definition to the underlying CLR type. If the two shapes match, then no work is performed. If CLR type has more members than the EDM defined, the API Explorer extensions create a new, non-executable type in a dynamic assembly. In the purest sense, it is not a proxy. You cannot execute the generated code; however, Reflection can be used on the defined types, which will make OpenAPI generators like Swashbuckle happy and unaware of this behavior as it looks like any other type. The dynamically generated types use the same namespace, type name, and will copy all of the same attributes defined on the original type as well. It's worth nothing that the EDM can also map an alternate namespace and/or type name (ex: My.App.Models.OrderV1 → Store.Order), which makes sense because that is a server implementation detail. An assembly is generated for each EDM and API version combination. This is required because the same type may appear in multiple API versions which could conflict with each other if they are not in separate assemblies.

Another edge case is OData action parameters. An OData action is always a POST and the body is defined purely in EDM. The backing code receives a ODataActionParameters, but this is nothing more than Dictionary<string, object>. As you are likely well-aware, that doesn't yield anything useful for OpenAPI or Swashbuckle. The same dynamic type approach is used to create a pseudo type based on the EDM definition so there is a model to hand over to Swashbuckle. It is oblivious to the fact that things are realized as a dictionary on the server side.

An area where this can fall down is if you use different OData route prefixes and the same model names exist in the same EDM and API version. For example, you have something like /api/orders and /admin/orders and they both define Order, which may or may not be the same type. IMHO this is categorically wrong and should never be done; an order is an order. If you have two different representations of Order in the EDM, this can lead to duplicate types being generated by Swashbuckle. There are at least two ways around this:

1. Just don't do it; refactor
2. Map one of the entities with an alternate name; for example, the admin orders entity set can rename the CLR type Order as AdminOrder in the EDM
   • This will give the model a unique name
   • The only difference is the name to the client
   • If Order is exactly same in both cases, then this isn't necessary nor should it be an issue; however, it could be a landmine later if the paths diverge

I'm not 100% if this is your case, but that is certainly a case where the behavior you are describing can happen. There isn't a great defense against this upfront. OData route prefixes should not be treated as sub applications or as a mechanism to logically segregate parts of an API. I get that you can, but that's not really what it was meant to do. At the end of the day, the entire API and its model is collated into a single set that is traversed by the API Explorer, grouped by API version, and has an OpenAPI document generated by Swashbuckle. Based on the OP, I believe some variant of this behavior is what is causing the issue.

[Angelinsky7]

@codecov-commenter thanks for your clarification and sorry to have use the terms "proxy" (it was easier than your great explanation)

i can manage to refactor my code for almost all cases your present (it's what i did for some of my issues) but in some cases, i have some NON-odata endpoints (that i cannot transform to ODATA for some reasons) sharing some CLR Type. Now, the only workaround i have is to create an new empty CLR Type extending the "odata" Type to make "Swashbuckle happy"...

i understand that this is not "correct" but sadly i don't have found any other way around.

[commonsensesoftware]

While it might not be the right way or, at least, what I would do, it should be possible to do things however you want. My hunch seems to confirm that you are in that edge case. I see that you attached a repro in the related issue. I will see if I can make it work over the next couple of days and echo it back. You can tweak and changes things to suite your needs.

After that, we can reassess whether pursuing this PR makes sense. I suspect this is less of a Swashbuckle issue and more of a configuration problem that Swashbuckle cannot directly solve. It might, however, be beneficial to provide a better diagnostic message in the exception and/or some other facility. When I've had to troubleshoot this issue myself, it's not always obvious which source types led to the ambiguous duplication.