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

tools/importer: skip build model if input schema is only for property #3041

Merged
merged 5 commits into from
Sep 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 17 additions & 10 deletions tools/importer-rest-api-specs/components/parser/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,8 @@ func (d *SwaggerDefinition) detailsForField(modelName string, propertyName strin
}

// first get the object definition
objectDefinition, nestedResult, err := d.parseObjectDefinition(modelName, propertyName, &value, result)
parsingModel := false
objectDefinition, nestedResult, err := d.parseObjectDefinition(modelName, propertyName, &value, result, parsingModel)
if err != nil {
return nil, nil, fmt.Errorf("parsing object definition: %+v", err)
}
Expand Down Expand Up @@ -451,7 +452,13 @@ func (d *SwaggerDefinition) findAncestorType(input spec.Schema) (*string, *strin
return nil, nil, nil
}

func (d SwaggerDefinition) parseObjectDefinition(modelName, propertyName string, input *spec.Schema, known internal.ParseResult) (*models.ObjectDefinition, *internal.ParseResult, error) {
// if `inputForModel` is false, it means the `input` schema cannot be used to parse the model of `modelName`
func (d SwaggerDefinition) parseObjectDefinition(
modelName, propertyName string,
input *spec.Schema,
known internal.ParseResult,
parsingModel bool,
) (*models.ObjectDefinition, *internal.ParseResult, error) {
// find the object and any models and constants etc we can find
// however _don't_ look for discriminator implementations - since that should be done when we're completely done
result := internal.ParseResult{
Expand Down Expand Up @@ -509,7 +516,7 @@ func (d SwaggerDefinition) parseObjectDefinition(modelName, propertyName string,
}

// then call ourselves to work out what to do with it
objectDefinition, nestedResult, err := d.parseObjectDefinition(*objectName, propertyName, topLevelObject, knownIncludingPlaceholder)
objectDefinition, nestedResult, err := d.parseObjectDefinition(*objectName, propertyName, topLevelObject, knownIncludingPlaceholder, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

presumably this should be passing parsingModel in when looping around? else we'll potentially be passing true when the false thing:

Suggested change
objectDefinition, nestedResult, err := d.parseObjectDefinition(*objectName, propertyName, topLevelObject, knownIncludingPlaceholder, true)
objectDefinition, nestedResult, err := d.parseObjectDefinition(*objectName, propertyName, topLevelObject, knownIncludingPlaceholder, parsingModel)

Copy link
Collaborator Author

@wuxu92 wuxu92 Sep 18, 2023

Choose a reason for hiding this comment

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

I think we should always pass a true here, because the Schame represented by topLevelObject is what the *objectName want. that means we can use topLevelObject to parse model of *objectName. also as other two calls below, the inner/inheritedModel is corresponding to the modelName, and they should always pass a true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Importing the full set of Services there's no difference with this logic, but threading this through causes:

2023-09-21T13:09:57.569+0200 [INFO]  Importer for Service "Advisor".Importer for API Version "2022-10-01": :x: Service "Advisor" - Api Version "2022-10-01"
2023-09-21T13:09:57.569+0200 [ERROR] Importer for Service "Advisor".Importer for API Version "2022-10-01":      :boom: Error: parsing Swagger files: parsing files in "../../submodules/rest-api-specs/specification/advisor/resource-manager/Microsoft.Advisor/stable/2022-10-01": parsing definition: finding resources for tag "AdvisorScore": finding nested items yet to be parsed: finding top level object named "TimeSeriesEntityTimeSeriesInlined": the top level object "TimeSeriesEntityTimeSeriesInlined" was not found
2023/09/21 13:09:57 Error: parsing data for Service "Advisor": parsing data for Service "Advisor" / Version "2022-10-01": parsing Swagger files: parsing files in "../../submodules/rest-api-specs/specification/advisor/resource-manager/Microsoft.Advisor/stable/2022-10-01": parsing definition: finding resources for tag "AdvisorScore": finding nested items yet to be parsed: finding top level object named "TimeSeriesEntityTimeSeriesInlined": the top level object "TimeSeriesEntityTimeSeriesInlined" was not found

So this looks fine 👍

if err != nil {
return nil, nil, err
}
Expand All @@ -519,18 +526,18 @@ func (d SwaggerDefinition) parseObjectDefinition(modelName, propertyName string,
return objectDefinition, nestedResult, nil
}

// if it's an inlined model, pull it out and return that
// note: some models can just be references to other models
// however we should only do this when we're parsing a model (`parsingModel`) directly rather than when parsing a model from a field - and only if we haven't already parsed this model
if len(input.Properties) > 0 || len(input.AllOf) > 0 {
// special-case: if the model has no properties and inherits from one model
// then just return that object instead, there's no point creating the wrapper type
if len(input.Properties) == 0 && len(input.AllOf) == 1 {
inheritedModel := input.AllOf[0]
return d.parseObjectDefinition(inheritedModel.Title, propertyName, &inheritedModel, result)
return d.parseObjectDefinition(inheritedModel.Title, propertyName, &inheritedModel, result, true)
}

// check for / avoid circular references
if _, ok := result.Models[modelName]; !ok {
// check for / avoid circular references,
// however, we should only do this when we're parsing a model (`parsingModel`) directly rather than when parsing a model from a field - and only if we haven't already parsed this model
if _, ok := result.Models[modelName]; !ok && parsingModel {
nestedResult, err := d.parseModel(modelName, *input)
if err != nil {
return nil, nil, fmt.Errorf("parsing object from inlined model %q: %+v", modelName, err)
Expand Down Expand Up @@ -568,7 +575,7 @@ func (d SwaggerDefinition) parseObjectDefinition(modelName, propertyName string,
innerModelName = input.AdditionalProperties.Schema.Title
}

nestedItem, nestedResult, err := d.parseObjectDefinition(innerModelName, propertyName, input.AdditionalProperties.Schema, result)
nestedItem, nestedResult, err := d.parseObjectDefinition(innerModelName, propertyName, input.AdditionalProperties.Schema, result, true)
if err != nil {
return nil, nil, fmt.Errorf("parsing nested item for dictionary: %+v", err)
}
Expand All @@ -591,7 +598,7 @@ func (d SwaggerDefinition) parseObjectDefinition(modelName, propertyName string,
inlinedName = fmt.Sprintf("%s%sInlined", cleanup.NormalizeName(modelName), cleanup.NormalizeName(propertyName))
}

nestedItem, nestedResult, err := d.parseObjectDefinition(inlinedName, propertyName, input.Items.Schema, result)
nestedItem, nestedResult, err := d.parseObjectDefinition(inlinedName, propertyName, input.Items.Schema, result, true)
if err != nil {
return nil, nil, fmt.Errorf("parsing nested item for array: %+v", err)
}
Expand Down
27 changes: 27 additions & 0 deletions tools/importer-rest-api-specs/components/parser/models_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1861,3 +1861,30 @@ func TestParseModelWithLocation(t *testing.T) {
t.Fatalf("expected example.Fields['Location'] to be a Location but got %q", string(*field.CustomFieldType))
}
}

func TestParseModelBug2675DuplicateModel(t *testing.T) {
result, err := ParseSwaggerFileForTesting(t, "models_bug_2675_duplicate_model.json")
if err != nil {
t.Fatalf("parsing: %+v", err)
}
if result == nil {
t.Fatal("result was nil")
}

example, ok := result.Resources["Example"]
if !ok {
t.Fatal("the Resource 'Example' was not found")
}

if len(example.Operations) != 3 {
t.Fatalf("expected the resource `Example` to have 3 operation but got %d", len(example.Operations))
}

if len(example.Models) != 7 {
t.Fatalf("expected the resource `Example` to have 7 models but got %d", len(example.Models))
}

if _, ok := example.Models["ExampleEnvironmentUpdatePropertiesCreatorRoleAssignment"]; !ok {
t.Fatalf("expected the resource `Example` to have a model named `ExampleEnvironmentUpdatePropertiesCreatorRoleAssignment` but didn't get one")
}
}
18 changes: 10 additions & 8 deletions tools/importer-rest-api-specs/components/parser/operations.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,11 +346,11 @@ func (p operationsParser) optionsForOperation(input parsedOperation, logger hclo

// looks like these can be dates etc too
// ./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json- "name": "reportedEndTime",
//./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json- "in": "query",
//./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json- "required": true,
//./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json- "type": "string",
//./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json: "format": "date-time",
//./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json- "description": "The end of the time range to retrieve data for."
// ./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json- "in": "query",
// ./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json- "required": true,
// ./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json- "type": "string",
// ./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json: "format": "date-time",
// ./commerce/resource-manager/Microsoft.Commerce/preview/2015-06-01-preview/commerce.json- "description": "The end of the time range to retrieve data for."
objectDefinition, err := p.determineObjectDefinitionForOption(param)
if err != nil {
return nil, nil, fmt.Errorf("determining field type for operation: %+v", err)
Expand Down Expand Up @@ -417,7 +417,8 @@ func (p operationsParser) requestObjectForOperation(input parsedOperation, known

for _, param := range unexpandedOperation.Parameters {
if strings.EqualFold(param.In, "body") {
objectDefinition, result, err := p.swaggerDefinition.parseObjectDefinition(param.Schema.Title, param.Schema.Title, param.Schema, known)
parsingModel := true
objectDefinition, result, err := p.swaggerDefinition.parseObjectDefinition(param.Schema.Title, param.Schema.Title, param.Schema, known, parsingModel)
if err != nil {
return nil, nil, fmt.Errorf("parsing request object for parameter %q: %+v", param.Name, err)
}
Expand Down Expand Up @@ -473,7 +474,8 @@ func (p operationsParser) responseObjectForOperation(input parsedOperation, know
continue
}

objectDefinition, nestedResult, err := p.swaggerDefinition.parseObjectDefinition(details.ResponseProps.Schema.Title, details.ResponseProps.Schema.Title, details.ResponseProps.Schema, result)
parsingModel := true
objectDefinition, nestedResult, err := p.swaggerDefinition.parseObjectDefinition(details.ResponseProps.Schema.Title, details.ResponseProps.Schema.Title, details.ResponseProps.Schema, result, parsingModel)
if err != nil {
return nil, nil, fmt.Errorf("parsing response object from status code %d: %+v", statusCode, err)
}
Expand Down Expand Up @@ -520,7 +522,7 @@ type parsedOperation struct {
func (d *SwaggerDefinition) findOperationsMatchingTag(tag *string) *[]parsedOperation {
result := make([]parsedOperation, 0)
for httpMethod, operation := range d.swaggerSpecExpanded.Operations() {
//operation = inferMissingTags(operation, tag)
// operation = inferMissingTags(operation, tag)
for uri, operationDetails := range operation {
if !operationMatchesTag(operationDetails, tag) {
continue
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
{
"swagger": "2.0",
"info": {
"title": "Example",
"description": "Example",
"version": "2020-01-01"
},
"host": "management.mysite.com",
"schemes": [
"https"
],
"consumes": [
"application/json"
],
"produces": [
"application/json"
],
"security": [],
"securityDefinitions": {},
"paths": {
"/environments/{environmentName}": {
"get": {
"tags": [
"Example"
],
"description": "Gets an example environment.",
"parameters": [
{
"name": "environmentName",
"in": "path",
"required": true,
"type": "string",
"x-ms-parameter-location": "method"
}
],
"operationId": "Example_Get",
"responses": {
"200": {
"description": "OK. The request has succeeded.",
"schema": {
"$ref": "#/definitions/ExampleEnvironment"
}
}
}
},
"put": {
"tags": [
"Example"
],
"description": "Creates or updates an example environment.",
"parameters": [
{
"name": "environmentName",
"in": "path",
"required": true,
"type": "string",
"x-ms-parameter-location": "method"
},
{
"name": "body",
"in": "body",
"required": true,
"schema": {
"$ref": "#/definitions/ExampleEnvironment"
}
}
],
"operationId": "Example_CreateOrUpdate",
"responses": {
"200": {
"description": "Succeeded",
"schema": {
"$ref": "#/definitions/ExampleEnvironment"
}
}
}
},
"patch": {
"tags": [
"Example"
],
"description": "Patches an example environment.",
"parameters": [
{
"name": "environmentName",
"in": "path",
"required": true,
"type": "string",
"x-ms-parameter-location": "method"
},
{
"name": "body",
"in": "body",
"description": "Updatable project environment type properties.",
"required": true,
"schema": {
"$ref": "#/definitions/ExampleEnvironmentUpdate"
}
}
],
"operationId": "Example_Update",
"responses": {
"200": {
"description": "The resource was updated.",
"schema": {
"$ref": "#/definitions/ExampleEnvironment"
}
}
}
}
}
},
"definitions": {
"ExampleEnvironment": {
"type": "object",
"properties": {
"properties": {
"x-ms-client-flatten": true,
"$ref": "#/definitions/ExampleEnvironmentProperties"
},
"location": {
"type": "string"
}
}
},
"ExampleEnvironmentUpdateProperties": {
"type": "object",
"properties": {
"deploymentTargetId": {
"type": "string"
},
"creatorRoleAssignment": {
"type": "object",
"properties": {
"roles": {
"type": "object",
"additionalProperties": {
"$ref": "#/definitions/EnvironmentRole"
}
}
}
},
"userRoleAssignments": {
"type": "object",
"additionalProperties": {
"$ref": "#/definitions/UserRoleAssignment"
}
}
}
},
"ExampleEnvironmentProperties": {
"type": "object",
"allOf": [
{
"$ref": "#/definitions/ExampleEnvironmentUpdateProperties"
}
],
"properties": {
"provisioningState": {
"type": "string",
"readOnly": true
}
}
},
"ExampleEnvironmentUpdate": {
"type": "object",
"properties": {
"properties": {
"x-ms-client-flatten": true,
"$ref": "#/definitions/ExampleEnvironmentUpdateProperties"
},
"example": {
"type": "string"
}
}
},
"UserRoleAssignment": {
"type": "object",
"x-ms-client-name": "userRoleAssignmentValue",
"properties": {
"roles": {
"type": "object",
"additionalProperties": {
"$ref": "#/definitions/EnvironmentRole"
}
}
}
},
"EnvironmentRole": {
"type": "object",
"properties": {
"roleName": {
"type": "string",
"readOnly": true
},
"description": {
"type": "string",
"readOnly": true
}
}
}
},
"parameters": {
}
}