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.

Sign up for GitHub

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

Better support for inline schemas in parameters #12369

Merged
merged 11 commits into from
May 20, 2022
Merged

Better support for inline schemas in parameters #12369

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
eedc6f5
better support for inline schema in parameters
wing328 May 13, 2022
195f1da
fix parameter model type
wing328 May 14, 2022
95ca392
add new method for model
wing328 May 14, 2022
2b10b64
minor update
wing328 May 14, 2022
559409d
fix isModelWithProperties
wing328 May 14, 2022
33a1b6b
fix is model check
wing328 May 14, 2022
9348b15
null check for properties
wing328 May 15, 2022
ced1d53
inline parameter enhance with python-experimental fix [WIP] (#12397)
spacether May 19, 2022
5ffc8c8
Merge branch 'inline-parameter-enhance' of https://github.com/OpenAPI…
wing328 May 19, 2022
f531f37
fix test
wing328 May 19, 2022
b7aa873
bug fix for rust generator
wing328 May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4575,8 +4575,14 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
}

Schema parameterSchema;

// the parameter model name is obtained from the schema $ref
// e.g. #/components/schemas/list_pageQuery_parameter => toModelName(list_pageQuery_parameter)
String parameterModelName = null;

if (parameter.getSchema() != null) {
parameterSchema = parameter.getSchema();
parameterModelName = getParameterDataType(parameter ,parameter.getSchema());
parameterSchema = ModelUtils.getReferencedSchema(openAPI, parameter.getSchema());
Copy link
Contributor

@spacether spacether May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something odd is happening with unaliasSchema
The original code extracts the schema, unaliases it, then uses that unaliased schemas to set properties.

This code instead gets the schema from the ref automatically.
So why is unaliasSchema not handling this case automatically originally?

Copy link
Contributor

@spacether spacether May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my branch to fix the python-experimental issue:
https://github.com/OpenAPITools/openapi-generator/pull/12397/files

unaliasSchema is not retuning the actual enum schema because if an schema contains a ref to an enum schema, unaliasSchmema returns that schema with the $ref in it. That assumes that the generator will generate a model reference to the enum schema. Options are:

  1. One could make sure that the import and type reference is generator for this referenced schema rather than inlining the enum definition after dereferencing it. Deref and inlining is what typescript is doing. Option 1 is my preference because it is a more general solution.
  2. One could limit ModelUtils.getReferencedSchema to only run when getUseInlineModelResolver() is true + the ref is to an enum schema. Also how about doing this logic after the schema has already been unaliased?

One useful related question is: Do we have any flag for whether or not a generator generates models for enums?
I don't think that we do. If we had one we could use it in unaliasSchema

if referencedSchema is an enum:
    if generatorMakesEnumModel:
        return schema  # schema contains ref in it
    else:
        return referencedSchema

That would be a great general solution here.

CodegenProperty prop = fromProperty(parameter.getName(), parameterSchema);
codegenParameter.setSchema(prop);
} else if (parameter.getContent() != null) {
Expand All @@ -4586,7 +4592,8 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
}
Map.Entry<String, MediaType> entry = content.entrySet().iterator().next();
codegenParameter.contentType = entry.getKey();
parameterSchema = entry.getValue().getSchema();
parameterModelName = getParameterDataType(parameter, entry.getValue().getSchema());
parameterSchema = ModelUtils.getReferencedSchema(openAPI, entry.getValue().getSchema());
} else {
parameterSchema = null;
}
Expand Down Expand Up @@ -4717,9 +4724,8 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
//}
//codegenProperty.required = true;

String parameterDataType = this.getParameterDataType(parameter, parameterSchema);
if (parameterDataType != null) {
codegenParameter.dataType = parameterDataType;
if (parameterModelName != null) {
codegenParameter.dataType = parameterModelName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should codegenParameters handle dataType and complexType differently than codegenProperty?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change above simply uses a different (better) name in the code block as parameterDataType originally aims to store the model name so no change in code logic if I remember correctly.

if (ModelUtils.isObjectSchema(parameterSchema)) {
codegenProperty.complexType = codegenParameter.dataType;
}
Expand Down Expand Up @@ -4790,17 +4796,17 @@ public CodegenParameter fromParameter(Parameter parameter, Set<String> imports)
}

/**
* Returns the data type of a parameter.
* Returns the data type of parameter.
* Returns null by default to use the CodegenProperty.datatype value
*
* @param parameter Parameter
* @param schema Schema
* @return data type
*/
protected String getParameterDataType(Parameter parameter, Schema schema) {
if (parameter.get$ref() != null) {
String refName = ModelUtils.getSimpleRef(parameter.get$ref());
return toModelName(refName);
Schema unaliasSchema = ModelUtils.unaliasSchema(openAPI, schema);
if (unaliasSchema.get$ref() != null) {
return toModelName(ModelUtils.getSimpleRef(unaliasSchema.get$ref()));
}
return null;
}
Expand Down
4 changes: 3 additions & 1 deletion ...api-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,9 @@ public interface IJsonSchemaValidationProperties {
* @param p the schema which contains the type info
*/
default void setTypeProperties(Schema p) {
if (ModelUtils.isTypeObjectSchema(p)) {
if (ModelUtils.isModelWithPropertiesOnly(p)) {
spacether marked this conversation as resolved.
Show resolved Hide resolved
setIsModel(true);
} else if (ModelUtils.isTypeObjectSchema(p)) {
setIsMap(true);
} else if (ModelUtils.isArraySchema(p)) {
setIsArray(true);
Expand Down
74 changes: 16 additions & 58 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,10 +408,10 @@ private void flattenRequestBody(String modelName, Operation operation) {
/**
* Flatten inline models in parameters
*
* @param pathname target pathname
* @param modelName model name
* @param operation target operation
*/
private void flattenParameters(String pathname, Operation operation) {
private void flattenParameters(String modelName, Operation operation) {
List<Parameter> parameters = operation.getParameters();
if (parameters == null) {
return;
Expand All @@ -422,39 +422,19 @@ private void flattenParameters(String pathname, Operation operation) {
continue;
}

Schema model = parameter.getSchema();
if (model instanceof ObjectSchema) {
Schema obj = model;
if (obj.getType() == null || "object".equals(obj.getType())) {
if (obj.getProperties() != null && obj.getProperties().size() > 0) {
flattenProperties(openAPI, obj.getProperties(), pathname);
String modelName = resolveModelName(obj.getTitle(), parameter.getName());
modelName = addSchemas(modelName, model);
parameter.$ref(modelName);
}
}
} else if (model instanceof ArraySchema) {
ArraySchema am = (ArraySchema) model;
Schema inner = am.getItems();
if (inner instanceof ObjectSchema) {
ObjectSchema op = (ObjectSchema) inner;
if (op.getProperties() != null && op.getProperties().size() > 0) {
flattenProperties(openAPI, op.getProperties(), pathname);
String modelName = resolveModelName(op.getTitle(), parameter.getName());
Schema innerModel = modelFromProperty(openAPI, op, modelName);
String existing = matchGenerated(innerModel);
if (existing != null) {
Schema schema = new Schema().$ref(existing);
schema.setRequired(op.getRequired());
am.setItems(schema);
} else {
modelName = addSchemas(modelName, innerModel);
Schema schema = new Schema().$ref(modelName);
schema.setRequired(op.getRequired());
am.setItems(schema);
}
}
}
Schema parameterSchema = parameter.getSchema();

if (parameterSchema == null) {
continue;
}
String schemaName = resolveModelName(parameterSchema.getTitle(),
(operation.getOperationId() == null ? modelName : operation.getOperationId()) + "_" + parameter.getName() + "_parameter");
// Recursively gather/make inline models within this schema if any
gatherInlineModels(parameterSchema, schemaName);
if (isModelNeeded(parameterSchema)) {
// If this schema should be split into its own model, do so
Schema refSchema = this.makeSchemaInComponents(schemaName, parameterSchema);
parameter.setSchema(refSchema);
}
}
}
Expand Down Expand Up @@ -564,29 +544,7 @@ private void flattenComponents() {
flattenComposedChildren(modelName + "_oneOf", m.getOneOf());
} else if (model instanceof Schema) {
gatherInlineModels(model, modelName);
} /*else if (ModelUtils.isArraySchema(model)) {
ArraySchema m = (ArraySchema) model;
Schema inner = m.getItems();
if (inner instanceof ObjectSchema) {
ObjectSchema op = (ObjectSchema) inner;
if (op.getProperties() != null && op.getProperties().size() > 0) {
String innerModelName = resolveModelName(op.getTitle(), modelName + "_inner");
Schema innerModel = modelFromProperty(openAPI, 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());
m.setItems(schema);
} else {
Schema schema = new Schema().$ref(existing);
schema.setRequired(op.getRequired());
m.setItems(schema);
}
}
}
}*/
}
}
}

Expand Down
21 changes: 20 additions & 1 deletion modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,14 +725,33 @@ public static boolean isModel(Schema schema) {
}

// has properties
if (null != schema.getProperties()) {
if (null != schema.getProperties() && !schema.getProperties().isEmpty()) {
return true;
}

// composed schema is a model, consider very simple ObjectSchema a model
return schema instanceof ComposedSchema || schema instanceof ObjectSchema;
}

/**
* Check to see if the schema is a model with properties only (non-composed model)
*
* @param schema potentially containing a '$ref'
* @return true if it's a model with at least one properties
*/
public static boolean isModelWithPropertiesOnly(Schema schema) {
Copy link
Contributor

@spacether spacether May 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Do you want this to be true for AnyType models with properties and additionalProperties set? Right now it is true for that case.
  • Do you want this to be true for array/string/number models with properties and additionalProperties set? Right now it is true for that case.

How about limiting this to type object and AnyType models only?
Also type object with additionalProperties unset is the same as type object with additionalProperties true so why is this code returning different results for those same use cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review.

I previously didn't check getAdditionalProperties() but got a few issues (change in samples). Adding the check seems to fix it but you're right that it needs to revised further.

I simply want to set the isModel flag correctly because now isMap is set to true for a simple model. I've not touched the code in modules/openapi-generator/src/main/java/org/openapitools/codegen/IJsonSchemaValidationProperties.java for a while so I welcome suggestions on other way to fix the problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me look into what is happening with python experimental. I think it is changing because the component schema changed

Copy link
Contributor

@spacether spacether May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Root cause investigation is here: https://github.com/OpenAPITools/openapi-generator/pull/12369/files#r876380534

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be true if the schema also has anyOf or allOf definitions?

if (schema == null) {
return false;
}

if (null != schema.getProperties() && !schema.getProperties().isEmpty() && // has properties
(schema.getAdditionalProperties() == null || // no additionalProperties is set
(schema.getAdditionalProperties() instanceof Boolean && !(Boolean)schema.getAdditionalProperties()))) {
return true;
}
return false;
}

public static boolean hasValidation(Schema sc) {
return (
sc.getMaxItems() != null ||
Expand Down
14 changes: 8 additions & 6 deletions modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultCodegenTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2061,11 +2061,12 @@ public void objectQueryParamIdentifyAsObject() {
CodegenParameter parameter = codegen.fromParameter(openAPI.getPaths().get("/pony").getGet().getParameters().get(0), imports);

// TODO: This must be updated to work with flattened inline models
Assert.assertEquals(parameter.dataType, "PageQuery1");
Assert.assertEquals(parameter.dataType, "ListPageQueryParameter");
Assert.assertEquals(imports.size(), 1);
Assert.assertEquals(imports.iterator().next(), "PageQuery1");
Assert.assertEquals(imports.iterator().next(), "ListPageQueryParameter");

Assert.assertNotNull(parameter.getSchema());
Assert.assertEquals(parameter.getSchema().dataType, "Object");
Assert.assertEquals(parameter.getSchema().baseType, "object");
}

Expand Down Expand Up @@ -3167,8 +3168,8 @@ public void testVarsAndRequiredVarsPresent() {

// CodegenOperation puts the inline schema into schemas and refs it
assertTrue(co.responses.get(0).isModel);
assertEquals(co.responses.get(0).baseType, "objectData");
modelName = "objectData";
assertEquals(co.responses.get(0).baseType, "objectWithOptionalAndRequiredProps_request");
modelName = "objectWithOptionalAndRequiredProps_request";
sc = openAPI.getComponents().getSchemas().get(modelName);
cm = codegen.fromModel(modelName, sc);
assertEquals(cm.vars, vars);
Expand All @@ -3180,7 +3181,7 @@ public void testVarsAndRequiredVarsPresent() {
cm = codegen.fromModel(modelName, sc);
CodegenProperty cp = cm.getVars().get(0);
assertTrue(cp.isModel);
assertEquals(cp.complexType, "objectData");
assertEquals(cp.complexType, "objectWithOptionalAndRequiredProps_request");
}

@Test
Expand Down Expand Up @@ -4005,7 +4006,8 @@ public void testRequestParameterContent() {
CodegenMediaType mt = content.get("application/json");
assertNull(mt.getEncoding());
CodegenProperty cp = mt.getSchema();
assertTrue(cp.isMap);
assertFalse(cp.isMap);
assertTrue(cp.isModel);
assertEquals(cp.complexType, "object");
assertEquals(cp.baseName, "SchemaForRequestParameterCoordinatesInlineSchemaApplicationJson");

Expand Down
22 changes: 20 additions & 2 deletions samples/client/petstore/typescript-fetch/builds/enum/apis/DefaultApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ export interface FakeEnumRequestGetInlineRequest {
}

export interface FakeEnumRequestGetRefRequest {
stringEnum?: StringEnum;
stringEnum?: FakeEnumRequestGetRefStringEnumEnum;
nullableStringEnum?: StringEnum | null;
numberEnum?: NumberEnum;
numberEnum?: FakeEnumRequestGetRefNumberEnumEnum;
nullableNumberEnum?: NumberEnum | null;
}

Expand Down Expand Up @@ -210,3 +210,21 @@ export const FakeEnumRequestGetInlineNumberEnumEnum = {
NUMBER_3: 3
} as const;
export type FakeEnumRequestGetInlineNumberEnumEnum = typeof FakeEnumRequestGetInlineNumberEnumEnum[keyof typeof FakeEnumRequestGetInlineNumberEnumEnum];
/**
* @export
*/
export const FakeEnumRequestGetRefStringEnumEnum = {
One: 'one',
Two: 'two',
Three: 'three'
} as const;
export type FakeEnumRequestGetRefStringEnumEnum = typeof FakeEnumRequestGetRefStringEnumEnum[keyof typeof FakeEnumRequestGetRefStringEnumEnum];
/**
* @export
*/
export const FakeEnumRequestGetRefNumberEnumEnum = {
NUMBER_1: 1,
NUMBER_2: 2,
NUMBER_3: 3
} as const;
export type FakeEnumRequestGetRefNumberEnumEnum = typeof FakeEnumRequestGetRefNumberEnumEnum[keyof typeof FakeEnumRequestGetRefNumberEnumEnum];
22 changes: 20 additions & 2 deletions samples/client/petstore/typescript-fetch/builds/with-string-enums/apis/DefaultApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ export interface FakeEnumRequestGetInlineRequest {
}

export interface FakeEnumRequestGetRefRequest {
stringEnum?: StringEnum;
stringEnum?: FakeEnumRequestGetRefStringEnumEnum;
nullableStringEnum?: StringEnum | null;
numberEnum?: NumberEnum;
numberEnum?: FakeEnumRequestGetRefNumberEnumEnum;
nullableNumberEnum?: NumberEnum | null;
}

Expand Down Expand Up @@ -210,3 +210,21 @@ export enum FakeEnumRequestGetInlineNumberEnumEnum {
NUMBER_2 = 2,
NUMBER_3 = 3
}
/**
* @export
* @enum {string}
*/
export enum FakeEnumRequestGetRefStringEnumEnum {
One = 'one',
Two = 'two',
Three = 'three'
}
/**
* @export
* @enum {string}
*/
export enum FakeEnumRequestGetRefNumberEnumEnum {
NUMBER_1 = 1,
NUMBER_2 = 2,
NUMBER_3 = 3
}
9 changes: 4 additions & 5 deletions samples/openapi3/client/petstore/python-experimental/docs/FakeApi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2446,7 +2446,6 @@ To test the collection format in query parameters
```python
import petstore_api
from petstore_api.api import fake_api
from petstore_api.model.string_with_validation import StringWithValidation
from pprint import pprint
# Defining the host is optional and defaults to http://petstore.swagger.io:80/v2
# See configuration.py for a list of all supported configuration parameters.
Expand Down Expand Up @@ -2538,10 +2537,10 @@ Type | Description | Notes
**[str]** | |

#### RefParamSchema
Type | Description | Notes
------------- | ------------- | -------------
[**StringWithValidation**](StringWithValidation.md) | |

Type | Description | Notes
------------- | ------------- | -------------
**str** | |

### Return Types, Responses

Expand Down Expand Up @@ -2623,7 +2622,7 @@ mapBean | MapBeanSchema | | optional
#### MapBeanSchema
Type | Description | Notes
------------- | ------------- | -------------
[**Foo**](Foo.md) | |
[**{str: (bool, date, datetime, dict, float, int, list, str, none_type)}**](Foo.md) | |


### Return Types, Responses
Expand Down
27 changes: 1 addition & 26 deletions ...pi3/client/petstore/python-experimental/petstore_api/api/default_api_endpoints/foo_get.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,32 +68,7 @@

_path = '/foo'
_method = 'GET'


class SchemaFor0ResponseBodyApplicationJson(
DictSchema
):

@classmethod
@property
def string(cls) -> typing.Type['Foo']:
return Foo


def __new__(
cls,
*args: typing.Union[dict, frozendict, ],
string: typing.Union['Foo', Unset] = unset,
_configuration: typing.Optional[Configuration] = None,
**kwargs: typing.Type[Schema],
) -> 'SchemaFor0ResponseBodyApplicationJson':
return super().__new__(
cls,
*args,
string=string,
_configuration=_configuration,
**kwargs,
)
SchemaFor0ResponseBodyApplicationJson = Schema


@dataclass
Expand Down
Loading