[core] Add x-is-free-form vendor extension

[jimschubert]

This adds an x-is-free-form vendor extension to allow users to skip our
"free-form" logic which would previously prevent object schemas with no
properties to be considered "free-form". The previous behavior was due
in part to Swagger Parser not exposing additionalProperties: false to
us (which should be similar behavior to this extension).

A free-form object is considered a dynamic object with any number of
properties/types. DefaultGenerator does not allow for generation of
models considered free-form. However, a base type with no properties and
no additional properties is allowed by OpenAPI Specification and is
meaningful in many languages (e.g. "marker interfaces" or abstract
closed types).

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
• Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @OpenAPITools/generator-core-team

[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.

[jeff9finger]

I think the definition of a free form object should be reconsidered here. Here is the definition from above:

A free-form object is considered a dynamic object with any number of properties/types.

I have a case where object has no properties (and therefore no discriminator property), but is referenced from another model's allOf. In this case, I don't think the referenced object should be considered free-form.

But perhaps I don't have the complete picture.

Here is a code snippet that I have in my branch - ModelUtils#isFreeFormObject(). I added the section with the comments "also check to see if this object is referenced from another allOf, anyOf, oneOf"

    public static boolean isFreeFormObject(OpenAPI openAPI, Schema schema) {
        if (schema == null) {
            // TODO: Is this message necessary? A null schema is not a free-form object, so the result is correct.
            once(LOGGER).error("Schema cannot be null in isFreeFormObject check");
            return false;
        }

        // not free-form if allOf, anyOf, oneOf is not empty
        if (schema instanceof ComposedSchema) {
            ComposedSchema cs = (ComposedSchema) schema;
            List<Schema> interfaces = ModelUtils.getInterfaces(cs);
            if (interfaces != null && !interfaces.isEmpty()) {
                return false;
            }
        }
 
        // also check to see if this object is referenced from another allOf, anyOf, oneOf
        Map<String, Schema> schemas = getSchemas(openAPI);
        if (schemas != null && !schemas.isEmpty()) {
            for (Schema s : schemas.values()) {
                if (!s.equals(schema)) {
                    if (s instanceof ComposedSchema) {
                        ComposedSchema cs = (ComposedSchema) s;
                        List<Schema> interfaces = ModelUtils.getInterfaces(cs);
                        if (isReferenceToSchema(openAPI,interfaces, schema)) {
                            break;
                        }
                    }
                }
            }
        }

        // has at least one property
        if ("object".equals(schema.getType())) {
            // no properties
            if ((schema.getProperties() == null || schema.getProperties().isEmpty())) {
                Schema addlProps = getAdditionalProperties(openAPI, schema);
                // additionalProperties not defined
                if (addlProps == null) {
                    return true;
                } else {
                    if (addlProps instanceof ObjectSchema) {
                        ObjectSchema objSchema = (ObjectSchema) addlProps;
                        // additionalProperties defined as {}
                        if (objSchema.getProperties() == null || objSchema.getProperties().isEmpty()) {
                            return true;
                        }
                    } else if (addlProps instanceof Schema) {
                        // additionalProperties defined as {}
                        if (addlProps.getType() == null && (addlProps.getProperties() == null || addlProps.getProperties().isEmpty())) {
                            return true;
                        }
                    }
                }
            }
        }

        return false;
    }

    private static boolean isReferenceToSchema(OpenAPI openAPI, List<Schema> schemas, Schema schema) {
        if (schemas != null) {
            for (Schema s : schemas) {
                String ref = s.get$ref();
                if (StringUtils.isNotEmpty(ref)) {
                    Schema referencedSchema = getSchema(openAPI, getSimpleRef(ref));
                    if (schema.equals(referencedSchema)) {
                        return true;
                    }
                }
            }
        }
        return false;
    }

Does this violate the semantics of a "free-form object"?

[jimschubert]

@jeff9finger I should be at least partially addressing your concern about free-form definition in this PR by removing the requirement that a model be considered a model if it has at least one property.

I like the suggestion to also consider if the schema is referenced in a composed schema, but I'll need to look at it more to understand if there are any hidden scenarios.

[jimschubert]

@wing328 any idea how to tackle this efficiently?

I think we could disallow "Free-form" objects as being allowed in oneOf/anyOf/allOf because we don't generate free-form objects. That proposed solution could only help.

I still think we should have the vendor extension because as a user I'd rather have an empty model created than a compilation error for a valid OpenAPI document.

The issue that I have is that all of these result in properties=null and additionalProperties=null:

definitions:
  Command:
    title: Command
    description: The base object for all command objects.
    type: object
    additionalProperties: false

definitions:
  Command:
    title: Command
    description: The base object for all command objects.
    type: object
    properties: {}
    additionalProperties: {}

definitions:
  Command:
    title: Command
    description: The base object for all command objects.
    type: object
    properties: {}
    additionalProperties: null

imho the 3 schemas above should not be considered free-form objects, but we currently can't ensure this because swagger-parser doesn't properly handle the false condition and returns nulls when the schemas have no definitions.

[jeff9finger]

Use FREE_FORM_EXPLICIT to match Java style for constants?

[jimschubert]

I would like to do this whenever we get more caught up on pull requests, but matched other constant styles in the file to avoid potential merge conflicts elsewhere.

We do track Sonar and this is a rule that's evaluated.

[jeff9finger]

@jimschubert Thanks for the analysis.

imho the 3 schemas above should not be considered free-form objects

So, only schemas that don't define type: object nor additionalProperties would be considered free-form, unless the vendor extension is true?

[jimschubert]

@jeff9finger for clarification, here are the rules I think would make sense:

• Additional properties that are empty with null properties would be "free-form"
• Additional properties = null and properties = null would be "free-form"
• All others are valid objects that generate, providing a warning if additional properties is non-empty

The issue I'm seeing is that swagger-parser isn't giving us that empty object state, and it's not handling additionalProperties=false. I'll need to dig into it a little more to see if there's a workaround.

[jimschubert]

pinging @OpenAPITools/generator-core-team again

[spacether]

So I disagree with the description that this schema is not free form:

definitions:
  Command:
    title: Command
    description: The base object for all command objects.
    type: object
    properties: {}
    additionalProperties: {}

You mention that our codegen incorrectly sets properties = null and additionalProperties = null for this case.
additionalProperties: {} should generate an empty Schema and allow any string key names with any type values for this model. This is what currently happens for v3 specs when additionalProperties is absent or True.
Our codegen does this because swagger parser does not correctly handle additionalProperties for v2 specs, right?

Free form objects could be generated and used as marker interfaces/abstract closed types.
How about:

• turning on free form model generation all the time or only when it is used as an interface schema
• adding a pre-processor or postprocessor step which fixes the additionalProperties results for v2 specs
  • additionalProperties absent -> additionalProperties = {} (consistent with v3 parsing + the swagger spec)
  • additionalProperties = True -> additionalProperties = {} (consistent with v3 parsing + the swagger spec)
  • additionalProperties = False -> additionalProperties = null (consistent with v3 parsing + the swagger spec)

This makes it so:

• additionalProperties handling will be consistent across v2 and v3 input specs
• we don't have to add another CLI flag
• interface types will automatically work for our users without them having to learn about/use another CLI option

I also like the Idea of seeing if a free-form schema is used in another schema, and if it is, generating it based upon that usage.

Shoot, do we not have a pre-processing step before specs are parsed?
I'm not able to find one in our code base and v2 specs come back from SwaggerParseResult result = new OpenAPIParser().readLocation(inputSpec, authorizationValues, options);
as v3 specs, and none of the original v2 spec spec json/yaml information is preserved as a schema field.

@sebastien-rosset do you have any thoughts on this issue?

[sebastien-rosset]

I think the definition of a free form object should be reconsidered here. Here is the definition from above:

A free-form object is considered a dynamic object with any number of properties/types.

I have a case where object has no properties (and therefore no discriminator property), but is referenced from another model's allOf. In this case, I don't think the referenced object should be considered free-form.

@jeff9finger , there is also a "isAnyType" method, would this help for your use case? IsAnyType means the type can be any valid JSON type, i.e. it can be the object, number, array, string or boolean. With OAS 3.1, it can also be the null type. On the other hand, isFreeForm means the type must be object. So an array, number, string, boolean or null type are not considered free-form objects. I'm just starting the read this thread, I don't have the full context yet.

[sebastien-rosset]

So I disagree with the description that this schema is not free form:

definitions:
  Command:
    title: Command
    description: The base object for all command objects.
    type: object
    properties: {}
    additionalProperties: {}

I'm slowly catching up with this thread, @spacether I agree with you the above is a free-form object (without having read the entire thread). That's because 1) the type is object and 2) there are no explicit (declared) properties and no restrictions on the additional properties. So basically the value can be any arbitrary map.

[spacether]

FYI, I just submitted a PR to fix additionalProperties handling in v2 specs in swagger-parser at: swagger-api/swagger-parser#1414
If merged we need:

• a later PR to have the latest mater version of swagger-parser use the new v1 branch of swagger-parser which parses v2 specs.
• a PR in our project to pull in the latest version of swagger-parser (master branch) +
• update our modelutils to define freeform object type using the correct results from additional properties

If merged swagger-api/swagger-parser#1414 will mean that additionalProperties will be handed the same in v2 and v3 specs in our project.
For those who don't know OpenapiGenerator converts all input v2 specs into v3 specs under the hood.

[jimschubert]

@spacether did you test this branch with that proposed change? I think this PR might still be necessary for example where specs are generated by tooling and that tooling is based on older versions of swagger which don't properly handle additionalProperties.

[spacether]

@spacether did you test this branch with that proposed change? I think this PR might still be necessary for example where specs are generated by tooling and that tooling is based on older versions of swagger which don't properly handle additionalProperties.

No I haven't tested this branch with my proposed change yet.
I don't know how to because the latest version of swagger parser uses a pom.xml release version to pull in v2 parsing code. And my PR is not yet released.

Are these PRs coupled?
My PR makes it so the results from v2 and v3 specs relative to including additionalProperties is consistent.
So if you have tests samples for v3 specs, and they work then my PR is not coupled to this one.

Is this PR functionally equivalent to turning on generateAliasAsModel?
Aren't these object models that include any types aliases to Map?
Could that generated alias map model meet people's marker interface needs?

[jimschubert]

So I disagree with the description that this schema is not free form:

definitions:
  Command:
    title: Command
    description: The base object for all command objects.
    type: object
    properties: {}
    additionalProperties: {}

I'm slowly catching up with this thread, @spacether I agree with you the above is a free-form object (without having read the entire thread). That's because 1) the type is object and 2) there are no explicit (declared) properties and no restrictions on the additional properties. So basically the value can be any arbitrary map.

To clarify the issues:

1. OAS defines additionalProperties as:
   
   meaning if additionalProperties does not exist, it should default to true (meaning all objects are freeform)
2. swagger-parser does not have a case for additionalProperties: false
3. users have historically gotten around this in ours and other tooling by defining an empty object as additionalProperties: {}
4. OAI does not explicitly say that a Schema with no properties is inherently free-form, if anything this is implied by the defaulted additionalProperties: true for which there is no option for false, null in the object model implies that it hadn't been set (and therefore defaults to true); this leaves us only with the empty object option
5. JSON Schema requires that when additionalProperties is an object, the resulting object validates against all properties in the object:
   
6. The additionalProperties: false case is fundamentally broken anyway.

So, I think a vendor extension is the cleanest approach.

[jimschubert]

Let's merge this and revisit when Justin's PR in swagger-parser gets merged.