Default values not present when using UseAllOfToExtendReferenceSchemas

[mikebeaton]

Background: I'm raising these issues because I need to be able to use string enums in a live project's API, with enum name conversion (specifically, camel casing) and with parameter defaults.

I've been running my own, patched versions of the code - first v4, now v5 - to do this successfully, but I'd love to be able to offer this back and have it merged ... if possible!

• Many thanks for fixing string enum camel casing in v5 (which, as per edit OpenAPI Version String OAI/OpenAPI-Specification#1164, now works great for enum values but not yet for enum defaults)
• I've developed what I think could be a lightweight enough solution to correctly converting enum defaults in Unification of Overlays and Traits OAI/Overlay-Specification#39 - if so, it would mean a working fully solution to the above, using UseInlineDefinitionsForEnums
• UseInlineDefinitionsForEnums is required in the above solution because defaults aren't legal in OpenAPI when using $ref (e.g. here and here), but it occurred to me that (I think...?!) defaults would be legal in the structure produced when using UseAllOfToExtendReferenceSchemas. However they aren't currently supported there: they remain null. I think implementing this is do-able, also in a lightweight manner, and compatible with Unification of Overlays and Traits OAI/Overlay-Specification#39

I'd like to finish that and offer it in another PR so you can see what I'm talking about! If it works (and if you agree that it's acceptably non-invasive, and also legal according to the relevant standards) then it would be a different solution to allowing string enum defaults, using UseAllOfToExtendReferenceSchemas instead of UseInlineDefinitionsForEnums. It would need to be combined with OAI/Overlay-Specification#39 to be a fully working solution (i.e. to also respect name conversions), in which case there would be two fully working, valid solutions to the above problem, available to anyone needing them - which I think would be good!

[domaindrivendev]

Hi @mikebeaton. Thanks for all the work you're putting into this - it's much appreciated, and apologies for my lack of responsiveness in getting back to you. Unfortunately, I can only feasibly give so many hours to this project. But this is an area that needs to be addressed, so if you bare with me I will get back to you as quickly as I can.

For this particular issue, i.e. not setting defaults in the context of UseAllOfToExtendReferenceSchemas, I need some time to dig a little deeper because the SchemaGenerator was designed to handle this case, and if it's not I'd prefer to fix that design rather than temoprarily changing the schema state to work around it.

To help me with this, perhaps you could post a code sample that repro's the issue?

[domaindrivendev]

OK I see where the bug is occurring now. In reality, the OpenApiAnyFactory abstraction isn't quite right at the moment and it's logic is a little brittle, as you've seen with both your issues. I have some ideas to improve the general design of this but in the meantime, I think the appropriate fix would be for the OpenApiAnyFactory to simply honor schemas that have been extended via the allOf keyword. That is, if the provided schema uses AllOf, then the type and format values should be extracted from that instead of it's inline Type and Format properties.

[mikebeaton]

That makes sense, I'll update my suggested code for this issue with that change.

In terms of OpenApiAnyFactory, I was wondering if the converter fn. approach which I came up with for my other issue could be applied more generally, so that e.g. SchemaRepository stores a DataContract for every schema, and each DataContract has a CreateFor function property which is set up when it is created.

So instead of requiring OpenApiAnyFactory, you'd be able to set defaults with something like schema.Default = schemaRepository.CreateFor(schema, value); where:

public class SchemaRepository
{
    \\\ ....

    public IOpenApiAny CreateFor(schema, value) =>
        _dataContracts[schema].CreateFor(value);
}

I'm not sure if that's anything like what you were thinking of, or sounds like it makes any sense?

[mikebeaton]

I've made the change you suggested to OAI/OpenAPI-Specification#1851, but it required a supporting change that I think you may not have been expecting (at least, I wasn't, until I did it - though I should have been).

The contents of AllOf are not schemas, they are references to schemas - so in CreateFor as it was, you can't use them - you have to pass round SchemaRepository quite a bit more, so that you can look up the referenced schema within CreateFor (exactly as I am already doing in my other proposed fix for sorting out conversion of string enum defaults; in that case, so that I can look up the correct enum converter).

For now, I've made this change in PR OAI/OpenAPI-Specification#1851 (for this issue) and merged it into PR OAI/OpenAPI-Specification#1852 (both issues merged) as well. If you look at the new merge commit d88ef1d from the merged PR, you'll see the minimal changes you are probably expecting - which are all that is needed there, because the code for passing SchemaRepository around is already in the merged version. But if you look at the pre-merged update f108f28 in PR OAI/OpenAPI-Specification#1851 (the PR specifically for this issue), you'll see quite a lot more changes (it's exactly the same minimal changes, plus the additional code to pass SchemaRepository around which is now required in both PRs for both issues separately).

[mikebeaton]

Apologies, the Appveyor build has broken. It looks as if my new changes are breaking some of the new tests when merged with the latest version - I'll have to look at this tomorrow.

EDIT: The current master build @ 12c4d25 is already breaking, the changes I've made aren't adding any new breaks.

[domaindrivendev]

Fixed by #1852