tools/importer: skip build model if input schema is only for property
#3041
Conversation
There was a problem hiding this comment.
hey @wuxu92
Thanks for this PR - I've taken a look and left some comments inline, but this is looking pretty good - if we can fix those up then this should otherwise be good to go 👍
Thanks!
| modelName, propertyName string, | ||
| input *spec.Schema, | ||
| known internal.ParseResult, | ||
| isInputForModel bool, |
There was a problem hiding this comment.
since this is being used to determine if we're passing a model, or pulling inlined information from a field - this is probably better named as:
| isInputForModel bool, | |
| parsingModel bool, |
| @@ -509,7 +515,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) | |||
There was a problem hiding this comment.
presumably this should be passing parsingModel in when looping around? else we'll potentially be passing true when the false thing:
| objectDefinition, nestedResult, err := d.parseObjectDefinition(*objectName, propertyName, topLevelObject, knownIncludingPlaceholder, true) | |
| objectDefinition, nestedResult, err := d.parseObjectDefinition(*objectName, propertyName, topLevelObject, knownIncludingPlaceholder, parsingModel) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
| // only parse model when modelName equals PropertyName, otherwise the `input` Schema may not for the modelName | ||
| // if parsing a top-level or an inlined model, should pass the propertyName just the same as the modelName |
There was a problem hiding this comment.
I think this'd be clearer as:
| // only parse model when modelName equals PropertyName, otherwise the `input` Schema may not for the modelName | |
| // if parsing a top-level or an inlined model, should pass the propertyName just the same as the modelName | |
| // 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 |
tombuildsstuff
added
tool/importer-rest-api-specs
There was a problem hiding this comment.
hi @wuxu92
If we can fix up the remaining comment (about the comment) then this should otherwise be good to go 👍
Thanks!
| @@ -509,7 +515,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) | |||
There was a problem hiding this comment.
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 👍
|
@tombuildsstuff Thanks a lot for reminding me of missing one comment ❤️ . updated now. |
should fix the importer issue of #2675. also cherry-picked @tombuildsstuff 's commit of adding Unit Test for it: d2ea0bb
I have regenerated all services and it caused no different from main branch's data, (except the DevCenter service):