[core][go] Generate inline models that were not previously generated

[fantavlik]

Description of the PR

Core Changes

• Refactor InlineModelResolver.java to recursively identify inline schemas that require their own model definitions.
  Examples of inline schemas that were not being generated that need their own models: enums, objects with defined properties, and composed schemas.
  Examples of inline locations that are now being checked recursively for inline models: within object properties, within object additionalProperties, within definitions of composed schemas (similar approach to Better handling of inline composed schemas in InlineModelResolver #2112), within definitions of "not", and within array items.
• Add more contextual naming rather than InlineObject, schemas defined inline within an operation section will get prefixes with the operation id, schemas within an array items section will get the parent model name + Items suffix, etc. Schemas with "title" fields will still use the title if defined.

Go changes (affecting all Go-based generators as changes were made within AbstractGoCodegen.java)

• Prefix all enum values with model name, this is needed to avoid collisions within the package
• Fix issue where numerical enums were quoted
• Fix issue where numerical enums would begin with underscore

This is a followup from #2494 - I attempted to fix this issue using Go templates at first but realized that a core change could fix these kinds of issues across all languages by refactoring the InlineModelResolver.java.

This also takes the support added by #2112 and extends that implementation to be recursive (I changed the suffixes slightly to be AllOf, rather than _allOf but otherwise the support should be the same) the suffixes now match AllOf, OneOf, AnyOf since this seems like the better convention.

Models like this which have many inline schemas that should be resolved into their own separate schemas/models were losing information (e.g. declaring enums as simple strings or declaring arrays of models arrays of simple objects).

Example:

Cat:
  description: Complex model with many inline models defined within it.
  type: object
  properties:
    myFood:
      oneOf: # inline oneOf
        - type: object
          title: Food
          properties:
            ingredients:
              type: array
              items:
                type: string
            pounds:
              type: integer
          required:
            - fooProp
    myHabitat:
      allOf: # inline allOf
        - type: object
          title: HabitatGeography
          properties:
            lat:
              type: number
              format: float
            long:
              type: number
              format: float
            continent:
              type: string
              enum:
                - Africa
                - Antarctica
                - Asia
                - Europe
                - North America
                - Oceania
                - South America
          required: [lat, long]
        - type: object
          description: Weather of the habitat defined here
          properties:
            rainfallInches:
              type: number
            averageTemperatureCelsius:
              type: number
    myTaxonomy:
      anyOf: # inline anyOf
        - description: Referenced schema
          type: object
          title: Species
          properties:
            name:
              type: string
            genus:
              type: string
            karyotype:
              type: string
        - description: Referenced schema
          type: object
          title: Order
          properties:
            name:
              type: string
            class:
              type: string
    cuteness: # inline enum
      type: integer
      enum: [1,3,5]
    kittens:
      type: array
      items: # inline array where the items are inline models
        allOf:
          - $ref: '#/components/schemas/Cat'
    preferences:
      type: object
      properties:
        favoriteToy: # inline object with properties defined
          type: string
      additionalProperties: # inline object with additionalProperties containing inline models
        title: Metadata
        type: string
        enum: [hidden,createdOn,createdBy,modifiedOn,modifiedBy]

Sample output before these changes (Go):

// Complex model with many inline models defined within it.
type Cat struct {
	MyFood OneOfobject `json:"myFood,omitempty"`
	MyHabitat map[string]interface{} `json:"myHabitat,omitempty"`
	MyTaxonomy AnyOfobjectobject `json:"myTaxonomy,omitempty"`
	Cuteness int32 `json:"cuteness,omitempty"`
	Kittens []Cat `json:"kittens,omitempty"`
	Preferences map[string]string `json:"preferences,omitempty"`
}
...
// OneOfobject does not exist - not generated
// AnyOfobjectobject does not exist - not generated

New output (Go):

// Complex model with many inline models defined within it.
type Cat struct {
	MyFood CatMyFood `json:"myFood,omitempty"`
	MyHabitat CatMyHabitat `json:"myHabitat,omitempty"`
	MyTaxonomy CatMyTaxonomy `json:"myTaxonomy,omitempty"`
	Cuteness CatCuteness `json:"cuteness,omitempty"`
	Kittens []CatKittensItems `json:"kittens,omitempty"`
	Preferences CatPreferences `json:"preferences,omitempty"`
}
...
type CatMyFood struct {
	Ingredients []string `json:"ingredients,omitempty"`
	Pounds int32 `json:"pounds,omitempty"`
}
...
type CatMyHabitat struct {
	Lat float32 `json:"lat"`
	Long float32 `json:"long"`
	Continent CatMyHabitatAllOfContinent `json:"continent,omitempty"`
	RainfallInches float32 `json:"rainfallInches,omitempty"`
	AverageTemperatureCelsius float32 `json:"averageTemperatureCelsius,omitempty"`
}
...
type CatMyHabitatAllOfContinent string

// List of CatMyHabitatAllOfContinent
const (
	CAT_MY_HABITAT_ALL_OF_CONTINENT_AFRICA CatMyHabitatAllOfContinent = "Africa"
	CAT_MY_HABITAT_ALL_OF_CONTINENT_ANTARCTICA CatMyHabitatAllOfContinent = "Antarctica"
	CAT_MY_HABITAT_ALL_OF_CONTINENT_ASIA CatMyHabitatAllOfContinent = "Asia"
	CAT_MY_HABITAT_ALL_OF_CONTINENT_EUROPE CatMyHabitatAllOfContinent = "Europe"
	CAT_MY_HABITAT_ALL_OF_CONTINENT_NORTH_AMERICA CatMyHabitatAllOfContinent = "North America"
	CAT_MY_HABITAT_ALL_OF_CONTINENT_OCEANIA CatMyHabitatAllOfContinent = "Oceania"
	CAT_MY_HABITAT_ALL_OF_CONTINENT_SOUTH_AMERICA CatMyHabitatAllOfContinent = "South America"
)
...
type CatMyTaxonomy struct {
	Name string `json:"name,omitempty"`
	Genus string `json:"genus,omitempty"`
	Karyotype string `json:"karyotype,omitempty"`
	Class string `json:"class,omitempty"`
}
...
type CatCuteness int32

// List of CatCuteness
const (
	CAT_CUTENESS_1 CatCuteness = 1
	CAT_CUTENESS_3 CatCuteness = 3
	CAT_CUTENESS_5 CatCuteness = 5
)
...
type CatKittensItems struct {
	MyFood CatMyFood `json:"myFood,omitempty"`
	MyHabitat CatMyHabitat `json:"myHabitat,omitempty"`
	MyTaxonomy CatMyTaxonomy `json:"myTaxonomy,omitempty"`
	Cuteness CatCuteness `json:"cuteness,omitempty"`
	Kittens []CatKittensItems `json:"kittens,omitempty"`
	Preferences CatPreferences `json:"preferences,omitempty"`
}
...
type CatPreferences struct {
	FavoriteToy string `json:"favoriteToy,omitempty"`
}
...
// Note: Go doesn't currently support complex additionalProperties, but this model is still generated:
type CatPreferencesMetadata string

// List of CatPreferencesMetadata
const (
	CAT_PREFERENCES_METADATA_HIDDEN CatPreferencesMetadata = "hidden"
	CAT_PREFERENCES_METADATA_CREATED_ON CatPreferencesMetadata = "createdOn"
	CAT_PREFERENCES_METADATA_CREATED_BY CatPreferencesMetadata = "createdBy"
	CAT_PREFERENCES_METADATA_MODIFIED_ON CatPreferencesMetadata = "modifiedOn"
	CAT_PREFERENCES_METADATA_MODIFIED_BY CatPreferencesMetadata = "modifiedBy"
)

I've only regenerated samples for Go so far but pretty much every language will have new models defined that were missing before.

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[fantavlik]

Issues that I have not tested but I believe are addressed in this PR: #2518
#2360
#2495

[fantavlik]

cc @wing328 for core changes and @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) for go-specific changes

[fantavlik]

Would love to get approval of this approach before I start fixing tests (which I am happy to do) cc core team:
@wing328 (2015/07)
@jimschubert (2016/05)
@cbornet (2016/05)
@jaz-ah (2016/05)
@ackintosh (2018/02)
@JFCote (2018/03)
@jmini (2018/04)

[grokify]

@fantavlik

Can you explain more how this handles more than one schema? In the example, anyOf is the only example with more than one schema, but it would be useful all the examples to have more than one schema.

In the myTaxonomy example you provided, it's unclear how the CatMyTaxonomy struct is generated to have a Name property since Species and Order aren't defined in the example.

Example Spec:

Cat:
  type: object
  properties:
    myTaxonomy:
      anyOf: # inline anyOf
        - $ref: '#/components/schemas/Species'
        - $ref: '#/components/schemas/Order'

Example Generated Code:

type CatMyTaxonomy struct {
	Name string `json:"name"`
}

Here are some example schema definitions for Species and Order for discussion.

• Would structs for Species and Order be generated?
• How would the differing properties, such as Species.genus and Order.class be implemented in the CatMyTaxonomy struct?

Species:
  description: Referenced schema
  type: object
  properties:
    name:
      type: string
    genus:
      type: string
    karyotype:
      type: string
Order:
  description: Referenced schema
  type: object
  properties:
    name:
      type: string
    class:
      type: string

[fantavlik]

Good call @grokify - the example I gave didn't do a good job of demonstrating the change, let me try and rework it to be fully inline.

For your specific question about CatMyTaxonomy the myTaxonomy field has been pulled into its own schema under #/components/schemas as if it had originally been defined like:

components:
  schemas:
    CatMyTaxonomy:
      anyOf: # inline anyOf
        - $ref: '#/components/schemas/Species'
        - $ref: '#/components/schemas/Order'
    Cat:
      type: object
        properties:
          myTaxonomy:
            - $ref: '#/components/schemas/CatMyTaxonomy'

So I'm not changing anything about how anyOf are generated, just making sure that models defined inline (in this case within the myTaxonomy property of Cat) are generated the same as if they were defined in the schemas section (as with the CatMyTaxonomy snippet above).

The missing referenced schemas are:

    Food:
      type: object
      properties:
        ingredients:
          type: array
          items:
            type: string
        pounds:
          type: integer
      required:
        - fooProp
    Habitat:
      type: object
      properties:
        lat:
          type: string
        long:
          type: string
      required: [lat, long]
    Species:
      type: object
      properties:
        name:
          type: string
      required: [name]
    Order:
      type: object
      properties:
        name:
          type: string
      required: [name]

Normally, structs would be generated for Species and Order. Would these still be generated?

Yes indeed, the key to the change I've made is that the anyOf model for the Cat.myTaxonomy taxonomy is being generated whereas before these changes a reference to a missing AnyOfSpeciesOrder model was being generated.

How would these differing properties, such as Species.genus and Order.class be implemented in the CatMyTaxonomy struct?

So just updating the example to use these for Species and Order:

Species:
  description: Referenced schema
  type: object
  properties:
    name:
      type: string
    genus:
      type: string
    karyotype:
      type: string
Order:
  description: Referenced schema
  type: object
  properties:
    name:
      type: string
    class:
      type: string

Yields the following output which is consistent with how the go generator already does anyOf (a struct that is the union of all properties):

model_cat_my_taxonomy.go:

type CatMyTaxonomy struct {
	Name string `json:"name,omitempty"`
	Genus string `json:"genus,omitempty"`
	Karyotype string `json:"karyotype,omitempty"`
	Class string `json:"class,omitempty"`
}

while still outputting the Species and Order models:
model_species.go:

// Referenced schema
type Species struct {
	Name string `json:"name,omitempty"`
	Genus string `json:"genus,omitempty"`
	Karyotype string `json:"karyotype,omitempty"`
}

model_order.go:

// Referenced schema
type Order struct {
	Name string `json:"name,omitempty"`
	Class string `json:"class,omitempty"`
}

[fantavlik]

@grokify I updated the example in the PR description to have multiple entries in the anyOf and allOf and also added more nesting of models to show how many are currently missed by the existing InlineModelResolver.java implementation

[grokify]

Thanks for the update. Much clearer.

The following should be namespaced to prevent collisions.

Other than that it looks reasonable to me.

// Note: Go doesn't currently support complex additionalProperties, but this model is still generated:
type Metadata string

// List of Metadata
const (
	METADATA_HIDDEN Metadata = "hidden"
	METADATA_CREATED_ON Metadata = "createdOn"
	METADATA_CREATED_BY Metadata = "createdBy"
	METADATA_MODIFIED_ON Metadata = "modifiedOn"
	METADATA_MODIFIED_BY Metadata = "modifiedBy"
)

Perhaps something similar to what you have for CatMyHabitatAllOfContinent: CAT_MY_HABITAT_ALL_OF_CONTINENT_AFRICA

// List of Metadata
const (
	CAT_PREFERENCES_METADATA_HIDDEN Metadata = "hidden"
	CAT_PREFERENCES_METADATA_CREATED_ON Metadata = "createdOn"
	CAT_PREFERENCES_METADATA_CREATED_BY Metadata = "createdBy"
	CAT_PREFERENCES_METADATA_MODIFIED_ON Metadata = "modifiedOn"
	CAT_PREFERENCES_METADATA_MODIFIED_BY Metadata = "modifiedBy"
)

[fantavlik]

Great feedback, @grokify

I have updated the namespacing such that this is now generated:

type CatPreferencesMetadata string

// List of CatPreferencesMetadata
const (
	CAT_PREFERENCES_METADATA_HIDDEN CatPreferencesMetadata = "hidden"
	CAT_PREFERENCES_METADATA_CREATED_ON CatPreferencesMetadata = "createdOn"
	CAT_PREFERENCES_METADATA_CREATED_BY CatPreferencesMetadata = "createdBy"
	CAT_PREFERENCES_METADATA_MODIFIED_ON CatPreferencesMetadata = "modifiedOn"
	CAT_PREFERENCES_METADATA_MODIFIED_BY CatPreferencesMetadata = "modifiedBy"
)

also updated the InlineModelResolverTest.java to use the more illustrative example that you suggested (the one in this PR description above)

Let me know if there's anything else needed.

[jimschubert]

I think the work in this PR may resolve some issues around Enum handling. The biggest issue I see is that it results in a breaking issue for consumers of existing clients for every client that supports enums. Take, for instance, this one change of a csharp client generated against this branch:

diff --git a/samples/client/petstore/csharp/OpenAPIClientNetStandard/src/Org.OpenAPITools/Model/EnumArrays.cs b/samples/client/petstore/csharp/OpenAPIClientNetStandard/src/Org.OpenAPITools/Model/EnumArrays.cs
index 4c0e96fd21..0f588412bb 100644
--- a/samples/client/petstore/csharp/OpenAPIClientNetStandard/src/Org.OpenAPITools/Model/EnumArrays.cs
+++ b/samples/client/petstore/csharp/OpenAPIClientNetStandard/src/Org.OpenAPITools/Model/EnumArrays.cs
@@ -28,69 +28,28 @@ namespace Org.OpenAPITools.Model
     [DataContract]
     public partial class EnumArrays :  IEquatable<EnumArrays>
     {
-        /// <summary>
-        /// Defines JustSymbol
-        /// </summary>
-        [JsonConverter(typeof(StringEnumConverter))]
-        public enum JustSymbolEnum
-        {
-            /// <summary>
-            /// Enum GreaterThanOrEqualTo for value: >=
-            /// </summary>
-            [EnumMember(Value = ">=")]
-            GreaterThanOrEqualTo = 1,
-
-            /// <summary>
-            /// Enum Dollar for value: $
-            /// </summary>
-            [EnumMember(Value = "$")]
-            Dollar = 2
-
-        }
-
         /// <summary>
         /// Gets or Sets JustSymbol
         /// </summary>
         [DataMember(Name="just_symbol", EmitDefaultValue=false)]
-        public JustSymbolEnum? JustSymbol { get; set; }
-        /// <summary>
-        /// Defines ArrayEnum
-        /// </summary>
-        [JsonConverter(typeof(StringEnumConverter))]
-        public enum ArrayEnumEnum
-        {
-            /// <summary>
-            /// Enum Fish for value: fish
-            /// </summary>
-            [EnumMember(Value = "fish")]
-            Fish = 1,
-
-            /// <summary>
-            /// Enum Crab for value: crab
-            /// </summary>
-            [EnumMember(Value = "crab")]
-            Crab = 2
-
-        }
-
-
-        /// <summary>
-        /// Gets or Sets ArrayEnum
-        /// </summary>
-        [DataMember(Name="array_enum", EmitDefaultValue=false)]
-        public List<ArrayEnumEnum> ArrayEnum { get; set; }
+        public EnumArraysJustSymbol? JustSymbol { get; set; }
         /// <summary>
         /// Initializes a new instance of the <see cref="EnumArrays" /> class.
         /// </summary>
         /// <param name="justSymbol">justSymbol.</param>
         /// <param name="arrayEnum">arrayEnum.</param>
-        public EnumArrays(JustSymbolEnum? justSymbol = default(JustSymbolEnum?), List<ArrayEnumEnum> arrayEnum = default(List<ArrayEnumEnum>))
+        public EnumArrays(EnumArraysJustSymbol justSymbol = default(EnumArraysJustSymbol), List<EnumArraysArrayEnumItems> arrayEnum = default(List<EnumArraysArrayEnumItems>))
         {
             this.JustSymbol = justSymbol;
             this.ArrayEnum = arrayEnum;
         }


+        /// <summary>
+        /// Gets or Sets ArrayEnum
+        /// </summary>
+        [DataMember(Name="array_enum", EmitDefaultValue=false)]
+        public List<EnumArraysArrayEnumItems> ArrayEnum { get; set; }

         /// <summary>
         /// Returns the string presentation of the object

Enums defined as inline are no longer inlined (see JustSymbolEnum). An object holding an array of enums don't make a lot of sense to me. For example, the spec document used in the C# generators has this object:

  EnumArrays:
    type: object
    properties:
      just_symbol:
        type: string
        enum:
          - ">="
          - "$"
      array_enum:
        type: array
        items:
          type: string
          enum:
            - fish
            - crab

As both an author and consumer of this document, I'd expect an object called EnumArrays which has two properties, one which is a normal enum and one which is an array of enums, where both of these enums are defined as nested types on EnumArrays. This is how the code previously generated. On this branch, these inlined enums are promoted to first-class models with odd prefix/suffixing of the names. I think for some languages this may make sense, but for other like C# it doesn't make sense and seems to reduce the quality of the generated code.

That said, I'm wondering how hard would it be to have this enum promotion logic as opt-in behind a generation flag? Or, could it be added as another generation option (on CodegenConfig) which generators must override in order to opt-in to the model shuffling?

I believe @jmini has done a lot of work on the inline model resolver, and may have additional comments.

[jimschubert]

nbd, but path.readOperations() will never return null.

[fantavlik]

@jimschubert I see what you mean, including the inline enum within the C# model files was intentional.

What I've observed is that many (most?) languages don't have separate templating support for inline enums, they work for enums that have been promoted to the #/components/schemas section and generate nothing for inline enums.

I think it would be pretty simple to add a flag (and perhaps adjust the default behavior for C# and Typescript and other languages that already support inline enums) such that enums are not promoted to their own schema under #/components/schemas, something like --resolveInlineEnums=false is that something that would address this issue for you?

[zzzuzik]

was this PR abandon?

[fantavlik]

I fixed the issues with Ruby samples, the samples were just out of date and referencing deleted files.

[fantavlik]

Moving on to Go samples failures next. I feel like I'm close? But I also feel like every day an eagle swoops down and eats my liver.

[wing328]

@fantavlik thanks for leading the effort to improve the inline model resolver. How urgent do you need this change to be included in the next release v5.0.1 (we'll release v5.0.0 within hours)?

If it's not urgent, may I suggest putting this PR on hold for the time being while the core team performs another review in early or mid Jan?

[fantavlik]

@fantavlik thanks for leading the effort to improve the inline model resolver. How urgent do you need this change to be included in the next release v5.0.1 (we'll release v5.0.0 within hours)?

If it's not urgent, may I suggest putting this PR on hold for the time being while the core team performs another review in early or mid Jan?

I have been dragging my feet on this but I finally have some more time now and moving forward, I am happy to wait to January.

[wing328]

👌 thanks again for the PR. Happy holidays!

[fantavlik]

Same to you William! Looking forward to finishing this up in the new year.

[kurtmc]

@fantavlik Thank you for working on this, I have been following this PR for a while and I am keen to see it get merged.

Please let me know if there is anything I can do to help. Happy to test out your branch against a bunch of openapi specs that are used in production.

[richardwhiuk]

Same, keen to see this landed - Rust doesn't support anonymous, or inline enums. Let me know if you need any help getting this landed.

[lesam]

If/when this is landed, will there be a flag to turn off strict validation of enum values for languages where they are not currently validated? Otherwise this seems like a potentially breaking change.

[SeanAsby]

Any update on this? I am generating go models from an api spec from a third party and would really like the added safety of typed enums. Please let me know if there is anything I can do to help get this merged.

[longquanzheng]

Hi, is there any updates on this PR? thanks for the hard work, this is really important for me.

[codyolsen]

It looks like this issue has been a can kicked down the road for the last 4 years. @fantavlik you've put a lot of work into this but it looks like the tests are still a blocking issue. Are you still actively invested in this PR?

It looks like from the history on this issue the main problem has been fragmented test failures with the different languages. Would it help if we could split the PR into the different language slices so we can piece meal merge the support in for each language that works? For the ones that still have issues we could let contributors with those language needs/expertise help with the test issues. Perhaps that would help avoid letting perfection become the enemy of progress?

[fantavlik]

Hey all, glad there's is still interest in this and sorry I abandoned this 3 years ago - I'm in a better place now personally to try to get this merged but want to make sure @wing328 and any other maintainers still support moving forward with this before I dig in again.

@codyolsen I had hoped to originally do what you suggest and break the PR apart by language but InlineModelResolver.java affects the inputs to all language templates. I guess one option would be to leave the existing InlineModelResolver.java code in place and feature flag the refactored resolver to allow languages to opt-in. @wing328 let me know if that's something you are open to.

[wing328]

Hi all, we've merged a few PRs to improve inline schema handling: https://github.com/OpenAPITools/openapi-generator/pulls?q=is%3Apr+inline+schema+is%3Aclosed+author%3Awing328

Doc: https://github.com/OpenAPITools/openapi-generator/blob/master/docs/customization.md#inline-schema-naming

Thanks @fantavlik for this PR as a staring point 🙏

We still haven't added an option to generate all enum as models. Hopefully it will be something we can support in 7.x or later.

[longquanzheng]

Hi all,
I actually worked around the issue by not using inline enums later so it’s not blocking me any more . But still very appreciated you all trying to improve it.

#2897 (comment)

[codyolsen]

@wing328 That's great there has been some progress on inline schema handling! Is there a way that we can merge in what @fantavlik has done on this CR to compliment the changes?

[codyolsen]

@longquanzheng That works if you have control of the model, but if it's a 3rd party schema you can't control then you are out of luck.

[wing328]

We still haven't added an option to generate all enum as models. Hopefully it will be something we can support in 7.x or later.

that's done in latest master (upcoming v7.0.0 release)

[wing328]

closing this one as pretty much all enhancements are added to the master

thanks again for this PR as a starting point.