Autorest: [WIP] Recognize vendor extensions for primitives

[gitfool]

I recently submitted Swashbuckle PR #776. I'd appreciate any feedback.

Basically, if AutoRest recognized the x-nullable and x-type (or x-type-dotnet) vendor extensions I'm proposing, should they (or something similar) be enabled, then the AutoRest generated C# proxy could remove the type impedance mismatch we have today - whether or not a type is _nullable_ would not have to be conflated with whether or not a type is _required_, and secondly the original C# type could be preserved for C# clients. This should cover use in parameters, request / response types, composite types, arrays etc.

[brjohnstmsft]

@devigned @Azure/adx-autorest-contributors Thoughts about having AutoRest support x-nullable? From the discussion here it seems there is consensus that it will be standardized in OpenAPI at some point.

x-nullable would solve a bunch of issues:

• https://github.com/Azure/autorest/issues/513
• https://github.com/Azure/autorest/issues/885
• https://github.com/Azure/autorest/issues/596

These are causing our team and others a bunch of pain. Solving it would eliminate nearly every case where we have to use x-ms-external in our specs today.

[gitfool]

@brjohnstmsft Please also consider x-type-dotnet as this differentiates _all_ primitive .NET types including the commonly used Decimal, DateTimeOffset (with more fine-grained control than the UseDateTimeOffset command-line option), and TimeSpan system types.

The x-type-dotnet extension complements the x-nullable extension to provide full type fidelity for C# code generators. Generators for other languages would ignore this extension and continue to provide the same "lowest common denominator" types. :stuck_out_tongue_winking_eye:

[fearthecowboy]

@gitfool I'm looking at this and related issues regarding nullability right now.

I'm going to review some of my notes with @devigned if he'd ever get back to his desk...

I think I am going to implement x-nullable in AutoRest, but in a discussion we had here a few days back it was suggested that format would be a better means to express additional type information rather than introducing an additional extension (ie, x-type-dotnet).

[brjohnstmsft]

👍 to x-nullable. Glad to hear you're working on it!

[gitfool]

Support for x-nullable would be much appreciated!

As for using format, that would work too, _but_ IMHO this is less desirable since it replaces rather than augments the type information and so impacts existing compatibility. It would be better to have a single Swagger spec that could be used by any code generator for any language, but which gets enlightened accordingly for this C# special case.

[fearthecowboy]

@giftool >> As for using format

The swagger spec says:

"The extending format for the previously mentioned type. See Data Type Formats for further details."

_sigh_... extending is a such _vague_ word.

FWIW, format is open-ended, and can be used specifically for this sort of thing, and since we've got the type anyway, we're generators have something to fall back on.

And, I've got to deal with the same thing across many languages in AutoRest, and I think it'd be more straightforward to support stuff like char and whatnot by using format

On top of that, I'm not terribly keen on making more extensions if I can avoid it. :dog:

Plus, and I'll deny saying it if you repeat this, but @devigned made a pretty good case for just using format in a meeting the other day...

[gitfool]

I'd love to hear that argument, but in the end if format will do without losing existing type fidelity with other code generators and languages then that's all that matters. The main other types that should be enlightened could be reduced to Char, Decimal, DateTimeOffset and TimeSpan. (The integer types were just for consistency and completeness.)

Also please make sure any x-nullable support is consistent and complete for all uses of a type; i.e. in parameters, request / response types, composite types, arrays etc. A good example is a controller that echoes its input to output, like I have in my Swashbuckle PR.

[fearthecowboy]

Also please make sure any x-nullable support is consistent and complete for all uses of a type

lol... that's pretty much what I'm trying to make sure happens. It's gonna be a bit of fun...

[gitfool]

Let me know how I can help. Maybe grab my test controllers as a starting point since I cover all the cases above, except for dictionaries.

Edit: Grab the source from the PR itself, not the comments in that issue: https://github.com/domaindrivendev/Swashbuckle/pull/776/commits/dd4d261cb850137d083551a493f929823ab05f55

[gitfool]

@fearthecowboy FWIW, I've made some progress on the Swashbuckle side. "Part I" has been merged; although it hasn't been released yet. Schema filters are now applied for _all_ schema types making it much easier to modify them and mark nullables!

I'd appreciate your feedback on "Part II" (Swashbuckle PR #820). In particular, I'll point out the TypeFormatSchemaFilter which refines the well known type formats to match your recent changes to support char (also decimal and duration), and the NullableTypeSchemaFilter which adds x-nullable type information to all schemas.

Also NonNullableAsRequiredSchemaFilter is a stop-gap filter that I've used to coerce AutoRest code-gen to generate non-nullable types for primitives in object properties. This is pretty weak, obviously, conflating nullable with required, so I very much look forward to AutoRest recognizing x-nullable; then this filter can be dropped, and I'd expect all types, including primitives used in requests and responses to be generated correctly! :wink:

[fearthecowboy]

I am just about to implement x-nullable right now.

[gitfool]

@fearthecowboy Just a friendly prod. Any idea when x-nullable support might be merged?

[fearthecowboy]

@gitfool I actually have that in my RefactorCodeBranch , which Zeus be willing, will make it to master tomorrow (I'm just waiting to review the last bit with the python team)
.

[fearthecowboy]

@gitfool support for "x-nullable":true should be in the 1.0-nightly builds at this point. we're going to add support for :false as well, to support a different case. Tests for these are forthcoming.

[brjohnstmsft]

:tada: That's great news @fearthecowboy! Using x-nullable will allow us to replace a lot of custom-tweaked model classes with generated ones. Thanks!

[gitfool]

Unless I'm missing something, "x-nullable":false is going to be the big payoff since primitives are nullable by default unless marked as required, bearing in mind that only applies to object properties; so did this default behavior reverse for primitives in parameters?

I'll try to have a play soon! 😉

[fearthecowboy]

@gitfool Should work exactly as you expect. @olydis did a great job with building a rather large test matrix for this in a wide variety of conditions.