Skip to content

Use reference types for Enum properties #451

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

Conversation

rockgecko-development
Copy link

This fixes #153 by using reference types for Enum properties.
Enums in query/path url parameters are NOT updated to use reference types. In OAS v2, enum query/url params must be inline. v3 supports using $ref. To implement this, we need to have the target version in StringEnumTypeVisitor.PayloadVisit / ParameterVisit to determine whether to return a $ref or inline enums, which would require some structural changes. Maybe there's a better way to do that, perhaps after the visitors while rendering the doc, but I would need some more guidance.

Breaking changes:

If the project contains multiple Enum type definitions with the same name (for example, #153 (comment) ), these will now conflict. One possible workaround would be to provide a type name callback/delegate, eg Func<Type, string> GetTypeName, where consumers of this lib who have this conflict could provide an alternative type name, eg by including the parent namespace.

@ghost
Copy link

ghost commented Jun 20, 2022

CLA assistant check
All CLA requirements met.

@justinyoo
Copy link
Contributor

Thanks, @rockgecko-development for the long wait! Because it's too big to ingest, I need to take some times to take a look. I'll get back to you once I complete review.

Also, as you mentioned, it might require breaking changes. Let's keep the conversation open and going.

@rockgecko-development
Copy link
Author

Thanks, let me know what you need to get this across the line.
There also will be breaking changes on the consumer side, if using a codegen tool. Currently, most codegen would give enum property types a name derived from the containing model name. This will now change to the original C# name (which is the whole intent of this PR).
(url/query enums will remain inline for now, and codegen would continue to derive the name from the endpoint and parameter name).

@justinyoo
Copy link
Contributor

In #153, @level120 mentioned that we need to figure out #248 first. If we follow that path, does this impact on your PR as well?

@rockgecko-development
Copy link
Author

#248 is referring to fields with the same name as the type. That doesn't seem specifically relevant to enums.
#153 is about the conflict if users have nested enums with simple, reused names like Status defined inside multiple classes. That would cause the kind of conflict I mentioned in my OP.
I haven't seen any suggested solutions other than the one I proposed above: provide a type name callback/delegate, eg Func<Type, string> GetTypeName in OpenApiConfigurationOptions, where consumers of this lib who have this conflict could provide an alternative type name, eg by including the parent namespace.

To implement this I would need some guidance from you in how to access this new config from within StringEnumTypeVisitor, as the visitors currently do not seem to have any context or configuration.

@justinyoo
Copy link
Contributor

#610 has taken care of this issue.

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

Successfully merging this pull request may close these issues.

Reuse enum definition multiple times
2 participants