Skip to content
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

[BUG] Incorrect assignment of allOf element #6601

Open
6 tasks
mwilby opened this issue Jun 9, 2020 · 1 comment
Open
6 tasks

[BUG] Incorrect assignment of allOf element #6601

mwilby opened this issue Jun 9, 2020 · 1 comment

Comments

@mwilby
Copy link

mwilby commented Jun 9, 2020

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • What's the version of OpenAPI Generator used?
  • Have you search for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Bounty to sponsor the fix (example)
Description

The following type of schema leads to a problem in the generation of model files.

device_type:
  type: object
  allOf:
    - $ref: '#/device_obj_test'
    - type: object
      properties:
        id:
          type: integer
        name:
          type: String
    - properties:
        sensors:
          type: array
          items:
            $ref: '#/sensor_type'

The referenced objects are not relevant, they are just objects defined elsewhere in the schema, so they have already been generated.

Three model files are generated for this object, when it should be two:

  • 'device_type` for the object itself

  • device_type_all_of the correctly generated inline object for the second element of the allOf list

  • device_type_all_of1 An incorrectly generated object containing the sensors property.

The problem is caused by the flattening process in InlineModelResolver. In the method
InlineModelResolver.flattenComponents(OpenAPI) composite schema's are flattened.

I think the intention is to pull out in-line objects and make sure they exist, but it is a little bit confused, so I am not 100% certain its supposed to do something else as well.

The allOf list in the model schema is passed to flattenComposedChildren. And the list contains 3 entries

  • Schema<T> for the reference to device_obj_test and type null

  • ObjectSchema referring to the object defined in the second element of the allOf list, with no reference and type object

  • Schema<T> with no reference and type null, containing a schema description of the properties

The last element, with no type or reference should not be created as a component. However, its properties should be flattened, so that any in-line types that are within the property list are created. The method that does this is

    private void flattenComposedChildren(OpenAPI openAPI, String key, List<Schema> children) {
        if (children == null || children.isEmpty()) {
            return;
        }
        ListIterator<Schema> listIterator = children.listIterator();
        while (listIterator.hasNext()) {
            Schema component = listIterator.next();
            if (component instanceof ObjectSchema || // for inline schema with type:object
                    (component != null && component.getProperties() != null &&
                            !component.getProperties().isEmpty())) { // for inline schema without type:object
                Schema op = component;
                if (op.get$ref() == null && op.getProperties() != null && op.getProperties().size() > 0) {
                    String innerModelName = resolveModelName(op.getTitle(), key);
                    Schema innerModel = modelFromProperty(op, innerModelName);
                    String existing = matchGenerated(innerModel);
                    if (existing == null) {
                        openAPI.getComponents().addSchemas(innerModelName, innerModel);
                        addGenerated(innerModelName, innerModel);
                        Schema schema = new Schema().$ref(innerModelName);
                        schema.setRequired(op.getRequired());
                        listIterator.set(schema);
                    } else {
                        Schema schema = new Schema().$ref(existing);
                        schema.setRequired(op.getRequired());
                        listIterator.set(schema);
                    }
                }
            } else {
                // likely a reference to schema (not inline schema)
            }
        }
    }

There are quite a few problems embedded in this bit of code. There are several locations where new schema objects are created and the values set from the old one, not necessarily wrong, but very error prone. The method modelFromProperty creates a new object with a limited set of the values copied across, among the missing values is the type field, which probably should be copied across, it should be reviewed as to what else should be included. It also includes the flattening of the parameters making the functionality quite difficult to figure out.

There is another location this could be a problem at the end of DefaultCodegen.fromModel(String name, Schema schema) the model name s added to the CodegenModel's anyOf, oneOf and allOf lists, even though the corresponding refSchema is null. It probably wont make any difference, but it could also be an error waiting to happen.

openapi-generator version

The current build

OpenAPI declaration file content or url

N/A

Command line used for generation

N/A

Steps to reproduce

N/A

Related issues/PRs

???

Suggest a fix

To fix the problem is relatively simple, inside flattenComposedChildren, if the type and ref are null don't add it as a component.

Note, you have to use the test on the op object because innerModel doesn't copy the type field.

if (existing == null) {
	if (null != op.getType()) {
	    openAPI.getComponents().addSchemas(innerModelName, innerModel);
	    addGenerated(innerModelName, innerModel);
	}
	openAPI.getComponents().addSchemas(innerModelName, innerModel);
	addGenerated(innerModelName, innerModel);
	Schema schema = new Schema().$ref(innerModelName);
	schema.setRequired(op.getRequired());
	listIterator.set(schema);
}

I have not tested it against anyOf, oneOf yet, but I have never got these options to work before anyway. I am about to run some tests against our object library to see if we can get them to work this time around.

In the short term, if it turns out to be a problem you can just flag the test against the allOfoption.

@nlundquist
Copy link

I'm also experiencing issues resulting from the type field not being correctly copied between instances during flatting.

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

No branches or pull requests

2 participants