Adding new rule XmsParameterLocation: Check Global parameters for this extension #149
Conversation
| return ResourceManager.GetString("XMSPathBaseNotInPaths", resourceCulture); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Looks up a localized string similar to The parameter '{0}' 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. |
There was a problem hiding this comment.
'{0}&apos [](start = 67, length = 14)
eh?
| @@ -357,4 +357,7 @@ | |||
| <data name="LongRunningOperationsWithLongRunningExtension" xml:space="preserve"> | |||
| <value>The operation '{0}' returns 202 status code, which indicates a long running operation, please enable "x-ms-long-running-operation.</value> | |||
| </data> | |||
| <data name="XmsParameterLocation" xml:space="preserve"> | |||
| <value>The parameter '{0}' 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.</value> | |||
There was a problem hiding this comment.
Please [](start = 168, length = 6)
Nit, add a space before Please
| public class XmsParameterLocation : TypedRule<SwaggerParameter> | ||
| { | ||
| private static readonly IEnumerable<string> AllowedGlobalParameters = new List<string>() | ||
| { "subscriptionid", "api-version", "apiversion" }; |
There was a problem hiding this comment.
I think there's a case insensitive check below, should be okay. A long shot, but check if anyone is also adding parameter name as subscription-id
| /// <summary> | ||
| /// The severity of this message (ie, debug/info/warning/error/fatal, etc) | ||
| /// </summary> | ||
| public override Category Severity => Category.Warning; |
There was a problem hiding this comment.
Should this be an error to get the author's attention. No one looks at warnings and we would probably miss it and catch much later. If they actually need this then the error can always be suppressed. Based on my experience 99% of services for Management plane should not have any parameters apart from subscriptionId, and api-version in global parameters section without "x-ms-parameter-location": "method"
| public override IEnumerable<ValidationMessage> GetValidationMessages(SwaggerParameter parameter, RuleContext context) | ||
| { | ||
| if (!AllowedGlobalParameters.Contains(parameter.Name.ToLower()) && | ||
| parameter.Extensions?.Keys?.Contains("x-ms-parameter-location") == false) |
There was a problem hiding this comment.
Want to make sure that this will handle:
"x-ms-parameter-location": "client"for a parameter that is not in the AllowedGlobalParameters- the x-ms-parameter-location extension is not present.
I think the first scenario is not handled.
Here is the link to the extension documentation and the link to the definition of extension in the schema.
| "The pet we get": { | ||
|
|
||
| } | ||
| "The pet we get": {} |
There was a problem hiding this comment.
nit: Ideally json keys should not have spaces as such, consider renaming this to thePetWeGet
| @@ -8,11 +8,12 @@ | |||
| // </auto-generated> | |||
| //------------------------------------------------------------------------------ | |||
|
|
|||
| namespace OpenAPI.Validator.Properties { | |||
| namespace OpenAPI.Validator.Properties | |||
There was a problem hiding this comment.
Any idea why these diffs are being introduced? It'd be great to undo these
| @@ -357,4 +357,7 @@ | |||
| <data name="LongRunningOperationsWithLongRunningExtension" xml:space="preserve"> | |||
| <value>The operation '{0}' returns 202 status code, which indicates a long running operation, please enable "x-ms-long-running-operation.</value> | |||
| </data> | |||
| <data name="XmsParameterLocation" xml:space="preserve"> | |||
| <value>The parameter '{0}' 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.</value> | |||
There was a problem hiding this comment.
nit: Verify with @kirthik about exactly what the message should be here
| public class XmsParameterLocation : TypedRule<SwaggerParameter> | ||
| { | ||
| private static readonly IEnumerable<string> AllowedGlobalParameters = new List<string>() | ||
| { "subscriptionid", "api-version", "apiversion" }; |
There was a problem hiding this comment.
I think there's a case insensitive check below, should be okay. A long shot, but check if anyone is also adding parameter name as subscription-id
| /// <summary> | ||
| /// What kind of open api document type this rule should be applied to | ||
| /// </summary> | ||
| public override ServiceDefinitionDocumentType ServiceDefinitionDocumentType => ServiceDefinitionDocumentType.ARM; |
There was a problem hiding this comment.
Do we want to relax this for DataPlane specs? Better to confirm it.
| @@ -98,6 +98,13 @@ public void AnonymousParameterSchemaValidation() | |||
| Assert.Equal(messages.Count(), 1); | |||
| } | |||
|
|
|||
| [Fact] | |||
There was a problem hiding this comment.
It's a good idea to add a positive test case as well.
@mcardosos @dsgouda @amarzavery @salameer Please review and approve.