[Maps] Adding Operations for airQuality (current, forecast/daily/ and forecast/hourly) to weather swagger #15249
Conversation
|
Hi, @john35452 Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vsswagger@microsoft.com |
|
[Call for Action] To better understand Azure service dev/test scenario, and support Azure service developer better on Swagger and REST API related tests in early phase, please help to fill in with this survey https://aka.ms/SurveyForEarlyPhase. It will take 5 to 10 minutes. If you already complete survey, please neglect this comment. Thanks. |
Swagger Validation Report
|
| Rule | Message |
|---|---|
|
The new version is adding a path that was not found in the old version. New: Weather/preview/1.0/weather.json#L713:5 |
|
The new version is adding a path that was not found in the old version. New: Weather/preview/1.0/weather.json#L774:5 |
|
The new version is adding a path that was not found in the old version. New: Weather/preview/1.0/weather.json#L839:5 |
️❌LintDiff: 1 Errors, 0 Warnings failed [Detail]
- Linted configuring files (Based on source branch, openapi-validator v1.10.0 , classic-openapi-validator v1.1.9 )
- Linted configuring files (Based on target branch, openapi-validator v1.10.0 , classic-openapi-validator v1.1.9 )
| Rule | Message |
|---|---|
|
The parameter 'pollutants' is defined in global parameters section without 'x-ms-parameter-location' extension. This would add the parameter as the client property. Please ensure that this is exactly you want. If so, apply the extension 'x-ms-parameter-location': 'client'. Else, apply the extension 'x-ms-parameter-location': 'method'. Location: Weather/preview/1.0/weather.json#L79 |
The following errors/warnings exist before current PR submission:
| Rule | Message |
|---|---|
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Feedback/preview/1.0/feedback.json#L216 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Mobility/preview/1.0/mobility.json#L1464 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Mobility/preview/1.0/mobility.json#L1491 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Route/preview/1.0/route.json#L1813 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Route/preview/1.0/route.json#L1855 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Route/preview/1.0/route.json#L1878 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Route/preview/1.0/route.json#L1969 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Search/preview/1.0/search.json#L2075 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Spatial/preview/1.0/spatial.json#L1095 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Spatial/preview/1.0/spatial.json#L1118 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Spatial/preview/1.0/spatial.json#L1148 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Spatial/preview/1.0/spatial.json#L1175 |
| The operation 'Dataset_Create' returns 202 status code, which indicates a long running operation, please enable 'x-ms-long-running-operation. Location: Dataset/preview/2.0/dataset.json#L196 |
|
| The operation 'Conversion_Convert' returns 202 status code, which indicates a long running operation, please enable 'x-ms-long-running-operation. Location: DwgConversion/preview/2.0/dwgconversion.json#L203 |
|
| The operation 'Tileset_Create' returns 202 status code, which indicates a long running operation, please enable 'x-ms-long-running-operation. Location: Tileset/preview/2.0/tileset.json#L216 |
|
| The enum types should have x-ms-enum type extension set with appropriate options. Property name: type Location: FeatureState/preview/2.0/featurestate.json#L827 |
|
| The enum types should have x-ms-enum type extension set with appropriate options. Property name: type Location: Search/preview/1.0/search.json#L3448 |
|
| The enum types should have x-ms-enum type extension set with appropriate options. Property name: geometryType Location: WFS/preview/2.0/wfs.json#L887 |
|
| 200/201 Responses of long running operations must have a schema definition for return type. OperationId: 'FeatureState_PutStateset', Response code: '200' Location: FeatureState/preview/2.0/featurestate.json#L325 |
|
| 200/201 Responses of long running operations must have a schema definition for return type. OperationId: 'FeatureState_UpdateStates', Response code: '200' Location: FeatureState/preview/2.0/featurestate.json#L465 |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️Cross-Version Breaking Changes succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️[Staging] SDK Track2 Validation succeeded [Detail]
- The following tags are being changed in this PR
|:speech_balloon: AutorestCore/Exception|"readme":"maps/data-plane/readme.md",
"tag":"package-preview-2.0",
"details":"> Installing AutoRest extension '@microsoft.azure/openapi-validator' (1.8.0)"|
|:speech_balloon: AutorestCore/Exception|"readme":"maps/data-plane/readme.md",
"tag":"package-preview-2.0",
"details":"> Installed AutoRest extension '@microsoft.azure/openapi-validator' (1.8.0->1.8.0)"|
The following errors/warnings exist before current PR submission:
| Rule | Message |
|---|---|
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Duplicate Schema named ODataErrorResponse -- properties.error.$ref: undefined => "#/components/schemas/schemas:39" ; This error can be temporarily avoided by using the 'modelerfour.lenient-model-deduplication' setting. NOTE: This setting will be removed in a future version of @autorest/modelerfour; schemas should be updated to fix this issue sooner than that." |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Duplicate Schema named ODataError -- properties.details.$ref: undefined => "#/components/schemas/schemas:42" ; This error can be temporarily avoided by using the 'modelerfour.lenient-model-deduplication' setting. NOTE: This setting will be removed in a future version of @autorest/modelerfour; schemas should be updated to fix this issue sooner than that." |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Duplicate Schema named LongRunningOperationResult -- properties.error.$ref: undefined => "#/components/schemas/schemas:74", properties.warning.$ref: undefined => "#/components/schemas/schemas:74" ; This error can be temporarily avoided by using the 'modelerfour.lenient-model-deduplication' setting. NOTE: This setting will be removed in a future version of @autorest/modelerfour; schemas should be updated to fix this issue sooner than that." |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Duplicate Schema named CoordinateAbbreviated -- properties.lat.$ref: undefined => "#/components/schemas/schemas:858", properties.lat.readOnly: undefined => true, properties.lon.$ref: undefined => "#/components/schemas/schemas:859", properties.lon.readOnly: undefined => true ; This error can be temporarily avoided by using the 'modelerfour.lenient-model-deduplication' setting. NOTE: This setting will be removed in a future version of @autorest/modelerfour; schemas should be updated to fix this issue sooner than that." |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Schema 'Point' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Schema 'Polygon' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Schema 'LineString' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Schema 'MultiPoint' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Schema 'MultiPolygon' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Schema 'MultiLineString' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"Error: Plugin prechecker reported failure." |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Duplicate Schema named ODataErrorResponse -- properties.error.$ref: undefined => "#/components/schemas/schemas:36" ; This error can be temporarily avoided by using the 'modelerfour.lenient-model-deduplication' setting. NOTE: This setting will be removed in a future version of @autorest/modelerfour; schemas should be updated to fix this issue sooner than that." |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Duplicate Schema named ODataError -- properties.details.$ref: undefined => "#/components/schemas/schemas:39" ; This error can be temporarily avoided by using the 'modelerfour.lenient-model-deduplication' setting. NOTE: This setting will be removed in a future version of @autorest/modelerfour; schemas should be updated to fix this issue sooner than that." |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Duplicate Schema named LongRunningOperationResult -- properties.error.$ref: undefined => "#/components/schemas/schemas:73", properties.warning.$ref: undefined => "#/components/schemas/schemas:73" ; This error can be temporarily avoided by using the 'modelerfour.lenient-model-deduplication' setting. NOTE: This setting will be removed in a future version of @autorest/modelerfour; schemas should be updated to fix this issue sooner than that." |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Duplicate Schema named CoordinateAbbreviated -- properties.lat.$ref: undefined => "#/components/schemas/schemas:856", properties.lat.readOnly: undefined => true, properties.lon.$ref: undefined => "#/components/schemas/schemas:857", properties.lon.readOnly: undefined => true ; This error can be temporarily avoided by using the 'modelerfour.lenient-model-deduplication' setting. NOTE: This setting will be removed in a future version of @autorest/modelerfour; schemas should be updated to fix this issue sooner than that." |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Schema 'Point' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Schema 'Polygon' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Schema 'LineString' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Schema 'MultiPoint' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Schema 'MultiPolygon' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Schema 'MultiLineString' has a property 'type' that is conflicting with a property in the parent schema 'GeoJSONGeometry' differs more than just description : [x-ms-enum => '', enum => '']" |
|
"readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"Error: Plugin prechecker reported failure." |
| 💬 AutorestCore/Exception | "readme":"maps/data-plane/readme.md", "tag":"package-preview-2.0", "details":"> Loading AutoRest extension '@autorest/modelerfour' (4.15.456->4.15.456)" |
| 💬 AutorestCore/Exception | "readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"> Loading AutoRest extension '@microsoft.azure/openapi-validator' (1.8.0->1.8.0)" |
| 💬 AutorestCore/Exception | "readme":"maps/data-plane/readme.md", "tag":"package-1.0-preview", "details":"> Loading AutoRest extension '@autorest/modelerfour' (4.15.456->4.15.456)" |
️️✔️[Staging] PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️[Staging] SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️[Staging] Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
Swagger Generation Artifacts
|
|
NewApiVersionRequired reason: |
69baaa1 to
34f6c85
Compare
34f6c85 to
e9ff98e
Compare
|
@john35452: This needs to be structured accordingly to #14871 since Weather service will be under its own client. |
Thanks for the remind! Currently, this PR is used to check schema with PM for now. |
| }, | ||
| { | ||
| "name": "duration", | ||
| "description": "Specifies for how many days from now we would like to know about the air quality. Available values are 1 ~ 4.", |
There was a problem hiding this comment.
Per below AccuWeather API doc, the available time spans are 1, 2, 3, 4, 5, 6, and 7 days.
http://apidev.accuweather.com/developers/airQualityAPIParameters#queryParameters
There was a problem hiding this comment.
Thanks for the remind!
It's kind of surprising that our current implementation only allows 1 to 4 which we also need to take care of in the near future.
https://msazure.visualstudio.com/One/_git/Azure-IoT-LocationBasedServices?path=%2Fsrc%2FALSProxy%2FALS%2FModels%2FWeather%2FDailyForecastAirQualityRequest.cs&_a=contents&version=GBmaster
| }, | ||
| "/weather/airQuality/forecast/daily/{format}": { | ||
| "get": { | ||
| "description": "**Get Daily Air Quality Forecast**\n\n**Applies to**: S1 pricing tiers.\n\nThe service provides the detailed information about the concentration of pollutant and overall status for upcoming few days air quality. The service can provide forecast air quality for future one to four days.", |
There was a problem hiding this comment.
To follow existing naming convention, we could update the API title from "Get Daily Air Quality Forecast" to "Get Air Quality Daily Forecast".
| }, | ||
| "/weather/airQuality/forecast/hourly/{format}": { | ||
| "get": { | ||
| "description": "**Get Hourly Air Quality forecast**\n\n**Applies to**: S1 pricing tiers.\n\nThe service provides the detailed information about the concentration of pollutant and overall status for current air quality.", |
There was a problem hiding this comment.
To follow existing naming convention, we could update the API title from "Get Hourly Air Quality Forecast" to "Get Air Quality Hourly Forecast".
| "Pollutants": { | ||
| "name": "Pollutants", | ||
| "in": "query", | ||
| "type": "string", |
There was a problem hiding this comment.
@john35452: the description mentions this is boolean?
There was a problem hiding this comment.
it'll work either way as our web server will translate the string value to boolean when we hydrate the request query object but swagger does support a boolean type so we can use that as well. See if we have any other API that has a boolean and if we use type = string or boolean. As i said it'll work either way at the service level so atleast we can be consistent
There was a problem hiding this comment.
booleans are way better for generated clients It feels like
There was a problem hiding this comment.
@sataneja: I think we should update others to bool too in another PR for consistency.
I mean, generated code is like:
alertsResp, err := weatherClient.GetSevereWeatherAlerts(ctx, weather.ResponseFormatJSON, "48.057,-81.091", &weather.WeatherGetSevereWeatherAlertsOptions{
Details: to.StringPtr("true"),
Language: to.StringPtr("EN"),
})Having string parameters for bools is weird from clients perspective. Generated bools are better:
currentAirQualityResp, err := weatherClient.GetCurrentAirQuality(ctx, weather.ResponseFormatJSON, "47.632346,-122.138874", &weather.WeatherGetCurrentAirQualityOptions{
Language: to.StringPtr("EN"),
Pollutants: to.BoolPtr(true),
})| { | ||
| "name": "duration", | ||
| "description": "Specifies for how many days from now we would like to know about the air quality. Available values are 1, 2, 3, 4, 5, 6, and 7.", | ||
| "default": 1, |
There was a problem hiding this comment.
We should mention that the default value is 1 in the description itself. Marking "default" property = 1 may help the SDK's but it doesn't show up in the final documentation so it's not easily visible to the user.
There was a problem hiding this comment.
Thanks for the remind. I have added it on.
| @@ -76,6 +76,13 @@ | |||
| } | |||
| }, | |||
| "parameters": { | |||
| "Pollutants": { | |||
| "name": "Pollutants", | |||
| "type": "integer" | ||
| }, | ||
| { | ||
| "$ref": "#/parameters/Pollutants" |
There was a problem hiding this comment.
Just double check but i looked at the Accuweather docs and I don't think the "pollutants" query param is supported for the "daily" API. It's only supported for "hourly" and "current" API's - http://apidev.accuweather.com/developers/airQualityAPIGuide#dailyFcastv2 - It makes sense if you see the response but i wd just run the daily api in Postman to confirm and if not just remove it from the Daily API docs
There was a problem hiding this comment.
Thanks for the remind! After I try with the postman. This parameter is not supported in daily forecasat, so I remove it now.
|
Hi @john35452, Your PR has some issues. Please fix the CI sequentially by following the order of
|
| "in": "query", | ||
| "type": "boolean", | ||
| "required": false, | ||
| "description": "Boolean value that returns detailed information about each pollutant. By default is True." |
There was a problem hiding this comment.
@john35452: "x-ms-parameter-location": "method" is also needed, this is a method parameter right?
@john35452: Also keep in mind the changes introduced:
ref pr I used to test the codegen of your updates: ambientlight@f822d81#diff-434f5529f9a095a3d6a4e2520ab5b7067f710bcb75ce41788533eb6fcf0976e2 |
|
@john35452: thanks, saw the #15351 |
|
@JeffreyRichter - the service is already deployed and this pr adds three 3 endpoints to swagger to align with the service. In an offline sync map engineer mentioned - the APIs for Azure Maps have been live for a few years, hence these changes aren't really breaking - the service APIs haven't changed, and there are no SDK released. |
|
@@JeffreyRichter could you provide the breaking change approval if there are no concerns? |
|
This does seem like a bunch of breaking changes. Is this matching service behavior or does the serviced want to make this change in which case it should be done in a new api-version? |
|
I approved this because I just read your first note. But, this seems like a bunch of breaking changes pushed on to customers. Service teams shouldn't just deploy new API contract changes and then update their swagger saying that this is NOW matching service behavior in order to get approval. |
|
@anuchandy, @JeffreyRichter: this should not be merged, superseded by #15351 |
|
So, should I approve #15351? |
|
To clarify, the removal of host parametrization is intentional (https://github.com/Azure/azure-rest-api-specs/pull/15351/files#diff-434f5529f9a095a3d6a4e2520ab5b7067f710bcb75ce41788533eb6fcf0976e2R7), as it was added by mistake previously and should not appear in map clients except creator. However this has been already addressed in a seperate PR: #15386, we can remove it in order not cause 1007 - RemovedClientParameter and then there should not be any other concerns to approve #15351 ? |
|
Since this PR is targeted at Microsoft.Map directory which is in the old directory structure, and we need to add our new change to client directory now. |
This is a PR generated at OpenAPI Hub. You can view your work branch via this link.
Changelog
Please ensure to add changelog with this PR by answering the following questions.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from Breaking Change Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.