diff --git a/src/core/AutoRest.Core/Properties/Resources.Designer.cs b/src/core/AutoRest.Core/Properties/Resources.Designer.cs index ed9f9ad449..e9e3cd7c96 100644 --- a/src/core/AutoRest.Core/Properties/Resources.Designer.cs +++ b/src/core/AutoRest.Core/Properties/Resources.Designer.cs @@ -62,7 +62,7 @@ internal Resources() { } /// - /// Looks up a localized string similar to Top level properties should be one of name, type, id, location, properties, tags, plan, sku, etag, managedBy, identity. Extra properties found: "{0}".. + /// Looks up a localized string similar to Top level properties should be one of name, type, id, location, properties, tags, plan, sku, etag, managedBy, identity. Model definition '{0}' has extra properties ['{1}'].. /// public static string AllowedTopLevelProperties { get { diff --git a/src/core/AutoRest.Core/Properties/Resources.resx b/src/core/AutoRest.Core/Properties/Resources.resx index cc98d81fdc..bbf0d31967 100644 --- a/src/core/AutoRest.Core/Properties/Resources.resx +++ b/src/core/AutoRest.Core/Properties/Resources.resx @@ -308,7 +308,7 @@ Modelers: Booleans are not descriptive and make them hard to use. Instead use string enums with allowed set of values defined. - Top level properties should be one of name, type, id, location, properties, tags, plan, sku, etag, managedBy, identity. Extra properties found: "{0}". + Top level properties should be one of name, type, id, location, properties, tags, plan, sku, etag, managedBy, identity. Model definition '{0}' has extra properties ['{1}']. Tracked resource '{0}' must have a get operation. diff --git a/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/Validation/body-top-level-properties.json b/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/Validation/body-top-level-properties.json index a5cb70a7c6..0b46f6703e 100644 --- a/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/Validation/body-top-level-properties.json +++ b/src/modeler/AutoRest.Swagger.Tests/Resource/Swagger/Validation/body-top-level-properties.json @@ -28,17 +28,7 @@ "in": "body", "required": true, "schema": { - "type": "object", - "allOf": [ - { - "$ref": "#/definitions/Resource" - } - ], - "properties": { - "extraprop": { - "type": "string" - } - } + "$ref": "#/definitions/ParameterWithExtraProperty" }, "description": "The parameters to provide for the updated account." } @@ -99,6 +89,14 @@ "type": "string" }, "description": "Gets or sets a list of key value pairs that describe the resource. These tags can be used in viewing and grouping this resource (across resource groups). A maximum of 15 tags can be provided for a resource. Each tag must have a key no greater than 128 characters and value no greater than 256 characters." + }, + "properties": { + "extraProperty1": { + "description": "a property inside the props bag", + "schema": { + "type": "string" + } + } } }, "required": [ @@ -144,20 +142,20 @@ "x-ms-azure-resource": true } }, - "parameters": { - "SubscriptionIdParameter": { - "name": "subscriptionId", - "in": "path", - "required": true, - "type": "string", - "description": "test subscription id" - }, - "ApiVersion": { - "name": "api-version", - "in": "path", - "required": true, - "type": "string", - "description": "test api version" - } + "parameters": { + "SubscriptionIdParameter": { + "name": "subscriptionId", + "in": "path", + "required": true, + "type": "string", + "description": "test subscription id" + }, + "ApiVersion": { + "name": "api-version", + "in": "path", + "required": true, + "type": "string", + "description": "test api version" } - } \ No newline at end of file + } +} \ No newline at end of file diff --git a/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerValidationTests.cs b/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerValidationTests.cs index de005e9ee5..ad94ca4190 100644 --- a/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerValidationTests.cs +++ b/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerValidationTests.cs @@ -230,7 +230,7 @@ public void CollectionObjectsPropertiesNamingValidation() public void BodyTopLevelPropertiesValidation() { var messages = ValidateSwagger(Path.Combine(Core.Utilities.Extensions.CodeBaseDirectory, "Resource", "Swagger", "Validation", "body-top-level-properties.json")); - messages.AssertOnlyValidationMessage(typeof(BodyTopLevelProperties), 2); + messages.AssertOnlyValidationMessage(typeof(BodyTopLevelProperties), 1); } [Fact] diff --git a/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs b/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs index 507c61876d..57664fca0d 100644 --- a/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs +++ b/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs @@ -87,7 +87,6 @@ public ServiceDefinition() [Rule(typeof(OperationsAPIImplementationValidation))] [Rule(typeof(ProvidersPathValidation))] [Rule(typeof(PutResponseResourceValidation))] - [CollectionRule(typeof(BodyTopLevelProperties))] [CollectionRule(typeof(HttpVerbValidation))] [CollectionRule(typeof(DeleteMustNotHaveRequestBody))] [CollectionRule(typeof(BodyPropertiesNamesCamelCase))] @@ -125,6 +124,7 @@ public ServiceDefinition() [Rule(typeof(TrackedResourcePatchOperationValidation))] [Rule(typeof(DescriptionMissing))] [Rule(typeof(PatchBodyParametersSchemaValidation))] + [Rule(typeof(BodyTopLevelProperties))] [CollectionRule(typeof(RequiredReadOnlyPropertiesValidation))] public Dictionary Definitions { get; set; } diff --git a/src/modeler/AutoRest.Swagger/Validation/BodyTopLevelProperties.cs b/src/modeler/AutoRest.Swagger/Validation/BodyTopLevelProperties.cs index c4c8932818..02f36562d2 100644 --- a/src/modeler/AutoRest.Swagger/Validation/BodyTopLevelProperties.cs +++ b/src/modeler/AutoRest.Swagger/Validation/BodyTopLevelProperties.cs @@ -11,12 +11,12 @@ namespace AutoRest.Swagger.Validation { - public class BodyTopLevelProperties : TypedRule> + public class BodyTopLevelProperties : TypedRule> { - private readonly Regex resourceRefRegEx = new Regex(@".+/Resource$", RegexOptions.IgnoreCase); - private readonly string[] allowedTopLevelProperties = { "name", "type", "id", "location", "properties", "tags", "plan", "sku", "etag", - "managedBy", "identity", "kind"}; + private static readonly IEnumerable AllowedTopLevelProperties = new List() + { "name", "type", "id", "location", "properties", "tags", "plan", "sku", "etag", + "managedBy", "identity", "kind"}; /// /// Id of the Rule. @@ -28,62 +28,25 @@ public class BodyTopLevelProperties : TypedRule> /// public override ValidationCategory ValidationCategory => ValidationCategory.RPCViolation; + /// /// This rule passes if the body parameter contains top level properties only from the allowed set: name, type, /// id, location, properties, tags, plan, sku, etag, managedBy, identity /// - /// - /// - public override bool IsValid( Dictionary path, RuleContext context, out object[] formatParameters) + /// The model definitions + /// The context object + /// validation messages + public override IEnumerable GetValidationMessages(Dictionary definitions, RuleContext context) { - List notAllowedProperties = new List(); - foreach (string operation in path.Keys) + var resModels = context.ResourceModels; + var violatingModels = resModels.Where(resModel => definitions[resModel].Properties?.Keys.Except(AllowedTopLevelProperties).Any() == true); + foreach (var violatingModel in violatingModels) { - if ((operation.ToLower().Equals("get") || - operation.ToLower().Equals("put") || - operation.ToLower().Equals("patch")) && path[operation]?.Parameters != null) - { - foreach (SwaggerParameter param in path[operation].Parameters) - { - if (param.In == ParameterLocation.Body) - { - if (param?.Schema?.Reference != null) - { - string defName = Extensions.StripDefinitionPath(param.Schema.Reference); - var definition = ((ServiceDefinition)context.Root).Definitions[defName]; - if (definition?.AllOf != null && definition.Properties != null && - definition.AllOf.Select(s => s.Reference).Where(reference => resourceRefRegEx.IsMatch(reference)) != null) - { - //Model is allOf Resource - foreach (KeyValuePair prop in definition.Properties) - { - if (!allowedTopLevelProperties.Contains(prop.Key.ToLower())) - { - notAllowedProperties.Add(defName + "/" + prop.Key); - } - } - } - } - if (param?.Schema?.AllOf != null && param.Schema.Properties != null && - param.Schema.AllOf.Select(s => s.Reference).Where(reference => resourceRefRegEx.IsMatch(reference)) != null) - { - //Model is allOf Resource - foreach (KeyValuePair prop in param.Schema.Properties) - { - if (!allowedTopLevelProperties.Contains(prop.Key.ToLower())) - { - notAllowedProperties.Add(path[operation].OperationId + ":" + prop.Key); - } - } - } - } - } - } - } - formatParameters = new[] { string.Join(", ", notAllowedProperties.ToArray()) }; - return (notAllowedProperties.Count() == 0); + yield return new ValidationMessage(new FileObjectPath(context.File, context.Path.AppendProperty(violatingModel).AppendProperty("properties")), this, + violatingModel, string.Join(",", definitions[violatingModel].Properties.Keys.Except(AllowedTopLevelProperties))); + } } - + /// /// The template message for this Rule. ///