From 88cf9b733f04e1de7772cc113aea247ab390fe66 Mon Sep 17 00:00:00 2001 From: Vishrut Shah Date: Fri, 6 Jan 2017 10:11:14 -0800 Subject: [PATCH 1/4] Implement OperationNameValidation Rule for M1005, M1006, M1007 & M1009 --- .../Properties/Resources.Designer.cs | 9 +++ .../AutoRest.Core/Properties/Resources.resx | 3 + .../Model/ServiceDefinition.cs | 1 + .../Validation/OperationNameValidation.cs | 73 +++++++++++++++++++ 4 files changed, 86 insertions(+) create mode 100644 src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs diff --git a/src/core/AutoRest.Core/Properties/Resources.Designer.cs b/src/core/AutoRest.Core/Properties/Resources.Designer.cs index cbdfc1a537..90486c5d42 100644 --- a/src/core/AutoRest.Core/Properties/Resources.Designer.cs +++ b/src/core/AutoRest.Core/Properties/Resources.Designer.cs @@ -419,6 +419,15 @@ public static string OperationIdNounInVerb { } } + /// + /// Looks up a localized string similar to 'GET' operation must use method name 'Get', 'PUT' operation must use method name 'Create', 'PATCH' operation must use method name 'Update' and 'DELETE' operation must use method name 'Delete'.. + /// + public static string OperationNameNotValid { + get { + return ResourceManager.GetString("OperationNameNotValid", resourceCulture); + } + } + /// /// Looks up a localized string similar to Parameters "subscriptionId" and "api-version" are not allowed in the operations section. /// diff --git a/src/core/AutoRest.Core/Properties/Resources.resx b/src/core/AutoRest.Core/Properties/Resources.resx index c437a15757..41a9b7c49b 100644 --- a/src/core/AutoRest.Core/Properties/Resources.resx +++ b/src/core/AutoRest.Core/Properties/Resources.resx @@ -273,4 +273,7 @@ path cannot be null or an empty string or a string with white spaces while getting the parent directory + + 'GET' operation must use method name 'Get', 'PUT' operation must use method name 'Create', 'PATCH' operation must use method name 'Update' and 'DELETE' operation must use method name 'Delete'. + \ No newline at end of file diff --git a/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs b/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs index 80f183e5a0..cb0cdb074d 100644 --- a/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs +++ b/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs @@ -79,6 +79,7 @@ public ServiceDefinition() /// Key is actual path and the value is serializationProperty of http operations and operation objects. /// [Rule(typeof(UniqueResourcePaths))] + [CollectionRule(typeof(OperationNameValidation))] public Dictionary> Paths { get; set; } /// diff --git a/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs b/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs new file mode 100644 index 0000000000..a7acbfebe6 --- /dev/null +++ b/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs @@ -0,0 +1,73 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using AutoRest.Core.Logging; +using AutoRest.Core.Properties; +using AutoRest.Core.Validation; +using AutoRest.Swagger.Model; + +namespace AutoRest.Swagger.Validation +{ + public class OperationNameValidation : TypedRule> + { + /// + /// The template message for this Rule. + /// + /// + /// This may contain placeholders '{0}' for parameterized messages. + /// + public override string MessageTemplate => Resources.OperationNameNotValid; + + /// + /// The severity of this message (ie, debug/info/warning/error/fatal, etc) + /// + public override Category Severity => Category.Warning; + + /// + /// An fails this rule if it does not have the correct HTTP Verb. + /// + /// Operation Definition to validate + /// + public override bool IsValid(Dictionary operationDefinition) + { + bool areAllOperationNameValid = true; + foreach (KeyValuePair entry in operationDefinition) + { + bool isOperationNameValid = true; + string httpVerb = entry.Key; + string operationId = entry.Value.OperationId; + + if (httpVerb.Equals("GET", StringComparison.InvariantCultureIgnoreCase)) + { + isOperationNameValid = operationId.EndsWith("_Get") || operationId.Contains("_List"); + } + else if (httpVerb.Equals("PUT", StringComparison.InvariantCultureIgnoreCase)) + { + isOperationNameValid = operationId.EndsWith("_Create"); + } + else if (httpVerb.Equals("PATCH", StringComparison.InvariantCultureIgnoreCase)) + { + isOperationNameValid = operationId.EndsWith("_Update"); + } + else if (httpVerb.Equals("DELETE", StringComparison.InvariantCultureIgnoreCase)) + { + isOperationNameValid = operationId.EndsWith("_Delete"); + } + else + { + continue; + } + + // If any of the operation name under the path is invalid, then areAllOperationNameValid is false + if (!isOperationNameValid && areAllOperationNameValid) + { + areAllOperationNameValid = isOperationNameValid; + } + } + + return areAllOperationNameValid; + } + } +} From 653159acde8c32fad70603ff84b1e232560a5e9f Mon Sep 17 00:00:00 2001 From: Vishrut Shah Date: Fri, 6 Jan 2017 10:42:08 -0800 Subject: [PATCH 2/4] Update warning message --- .../Properties/Resources.Designer.cs | 2 +- .../AutoRest.Core/Properties/Resources.resx | 2 +- .../Validation/OperationNameValidation.cs | 17 ++++++++++++++--- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/core/AutoRest.Core/Properties/Resources.Designer.cs b/src/core/AutoRest.Core/Properties/Resources.Designer.cs index 90486c5d42..1914a67d9f 100644 --- a/src/core/AutoRest.Core/Properties/Resources.Designer.cs +++ b/src/core/AutoRest.Core/Properties/Resources.Designer.cs @@ -420,7 +420,7 @@ public static string OperationIdNounInVerb { } /// - /// Looks up a localized string similar to 'GET' operation must use method name 'Get', 'PUT' operation must use method name 'Create', 'PATCH' operation must use method name 'Update' and 'DELETE' operation must use method name 'Delete'.. + /// Looks up a localized string similar to 'GET' operation must use method name 'Get' or Method name start with 'List', 'PUT' operation must use method name 'Create', 'PATCH' operation must use method name 'Update' and 'DELETE' operation must use method name 'Delete'.. /// public static string OperationNameNotValid { get { diff --git a/src/core/AutoRest.Core/Properties/Resources.resx b/src/core/AutoRest.Core/Properties/Resources.resx index 41a9b7c49b..097d076661 100644 --- a/src/core/AutoRest.Core/Properties/Resources.resx +++ b/src/core/AutoRest.Core/Properties/Resources.resx @@ -274,6 +274,6 @@ path cannot be null or an empty string or a string with white spaces while getting the parent directory - 'GET' operation must use method name 'Get', 'PUT' operation must use method name 'Create', 'PATCH' operation must use method name 'Update' and 'DELETE' operation must use method name 'Delete'. + 'GET' operation must use method name 'Get' or Method name start with 'List', 'PUT' operation must use method name 'Create', 'PATCH' operation must use method name 'Update' and 'DELETE' operation must use method name 'Delete'. \ No newline at end of file diff --git a/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs b/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs index a7acbfebe6..851f3cdcac 100644 --- a/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs +++ b/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs @@ -23,13 +23,24 @@ public class OperationNameValidation : TypedRule> /// /// The severity of this message (ie, debug/info/warning/error/fatal, etc) /// + /// + /// This rule corresponds to M1005, M1006, M1007 & M1009. + /// public override Category Severity => Category.Warning; /// - /// An fails this rule if it does not have the correct HTTP Verb. + /// This rule passes if the operation id of HTTP Method confirms to M1005, M1006, M1007 & M1009. + /// e.g. For Get method User_Get or User_List + /// or For Put method User_Create + /// or For Patch method User_Update + /// or For Delete method User_Delete + /// are valid names. /// - /// Operation Definition to validate - /// + /// Dictionary of the path and respective operations. + /// true if operation name confimes to above rules, otherwise false. + /// + /// Message will be shown at the path level. + /// public override bool IsValid(Dictionary operationDefinition) { bool areAllOperationNameValid = true; From 9a7c34c6117df07a05387a1a7b3614991bb0f366 Mon Sep 17 00:00:00 2001 From: Vishrut Shah Date: Mon, 9 Jan 2017 10:37:03 -0800 Subject: [PATCH 3/4] Adding swagger modeler validation tests --- .../Validation/operation-name-not-valid.json | 85 +++++++++++++++++++ .../SwaggerModelerValidationTests.cs | 7 ++ 2 files changed, 92 insertions(+) create mode 100644 src/modeler/AutoRest.Swagger.Tests/Swagger/Validation/operation-name-not-valid.json diff --git a/src/modeler/AutoRest.Swagger.Tests/Swagger/Validation/operation-name-not-valid.json b/src/modeler/AutoRest.Swagger.Tests/Swagger/Validation/operation-name-not-valid.json new file mode 100644 index 0000000000..510094f61a --- /dev/null +++ b/src/modeler/AutoRest.Swagger.Tests/Swagger/Validation/operation-name-not-valid.json @@ -0,0 +1,85 @@ +{ + "swagger": "2.0", + "info": { + "title": "Operation name not valid", + "description": "Some documentation.", + "version": "2014-04-01-preview" + }, + "host": "management.azure.com", + "schemes": [ + "https" + ], + "basePath": "/", + "produces": [ + "application/json" + ], + "consumes": [ + "application/json" + ], + "paths": { + "/foo": { + "get": { + "operationId": "Noun_NotNamedGet", + "summary": "Foo get path", + "description": "Foo operation", + "responses": { + "default": { + "description": "Unexpected error" + } + } + } + }, + "/foo1": { + "put": { + "operationId": "Noun_NotNamedCreate", + "summary": "Foo2 create path", + "description": "Foo2 operation", + "responses": { + "default": { + "description": "Unexpected error" + } + } + } + }, + "/foo2": { + "delete": { + "operationId": "Noun_NotNamedDelete", + "summary": "Foo2 delete path", + "description": "Foo2 operation", + "responses": { + "default": { + "description": "Unexpected error" + } + } + } + }, + "/foo3": { + "get": { + "operationId": "Noun_List", + "summary": "Foo3 path", + "description": "Foo3 list operation", + "responses": { + "default": { + "description": "Unexpected error" + } + } + } + } + }, + "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" + } + } +} diff --git a/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerValidationTests.cs b/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerValidationTests.cs index 20f77f6321..c0b66f2898 100644 --- a/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerValidationTests.cs +++ b/src/modeler/AutoRest.Swagger.Tests/SwaggerModelerValidationTests.cs @@ -242,6 +242,13 @@ public void Pageable200ResponseNotModeledValidation() var messages = ValidateSwagger(Path.Combine("Swagger", "Validation", "pageable-no-200-response.json")); messages.Any(m => m.Type == typeof(PageableRequires200Response)); } + + [Fact] + public void OperationNameValidation() + { + var messages = ValidateSwagger(Path.Combine("Swagger", "Validation", "operation-name-not-valid.json")); + messages.AssertOnlyValidationMessage(typeof(OperationNameValidation), 3); + } } #region Positive tests From adf8aec42b7eeef4581de800f0298785ffc40168 Mon Sep 17 00:00:00 2001 From: Vishrut Shah Date: Mon, 9 Jan 2017 16:51:42 -0800 Subject: [PATCH 4/4] Correcting the clean swagger and improving the rules correctness --- .../positive/clean-complex-spec.json | 2 +- .../AutoRest.Swagger/Model/Operation.cs | 1 + .../Model/ServiceDefinition.cs | 1 - .../Validation/OperationNameValidation.cs | 58 ++++++++----------- 4 files changed, 25 insertions(+), 37 deletions(-) diff --git a/src/modeler/AutoRest.Swagger.Tests/Swagger/Validation/positive/clean-complex-spec.json b/src/modeler/AutoRest.Swagger.Tests/Swagger/Validation/positive/clean-complex-spec.json index 94d67ed82f..7f7d6ff68a 100644 --- a/src/modeler/AutoRest.Swagger.Tests/Swagger/Validation/positive/clean-complex-spec.json +++ b/src/modeler/AutoRest.Swagger.Tests/Swagger/Validation/positive/clean-complex-spec.json @@ -81,7 +81,7 @@ "/pets/{petId}": { "get": { "summary": "Info for a specific pet", - "operationId": "showPetById", + "operationId": "getPetById", "description": "foo", "tags": [ "pets" diff --git a/src/modeler/AutoRest.Swagger/Model/Operation.cs b/src/modeler/AutoRest.Swagger/Model/Operation.cs index 059573cf0a..43fc366af7 100644 --- a/src/modeler/AutoRest.Swagger/Model/Operation.cs +++ b/src/modeler/AutoRest.Swagger/Model/Operation.cs @@ -36,6 +36,7 @@ public Operation() /// [Rule(typeof(OneUnderscoreInOperationId))] [Rule(typeof(OperationIdNounInVerb))] + [Rule(typeof(OperationNameValidation))] public string OperationId { get; set; } public string Summary diff --git a/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs b/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs index cb0cdb074d..80f183e5a0 100644 --- a/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs +++ b/src/modeler/AutoRest.Swagger/Model/ServiceDefinition.cs @@ -79,7 +79,6 @@ public ServiceDefinition() /// Key is actual path and the value is serializationProperty of http operations and operation objects. /// [Rule(typeof(UniqueResourcePaths))] - [CollectionRule(typeof(OperationNameValidation))] public Dictionary> Paths { get; set; } /// diff --git a/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs b/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs index 851f3cdcac..d398c1993d 100644 --- a/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs +++ b/src/modeler/AutoRest.Swagger/Validation/OperationNameValidation.cs @@ -10,8 +10,10 @@ namespace AutoRest.Swagger.Validation { - public class OperationNameValidation : TypedRule> + public class OperationNameValidation : TypedRule { + private const string NOUN_VERB_PATTERN = "^(\\w+)?_(\\w+)$"; + /// /// The template message for this Rule. /// @@ -41,44 +43,30 @@ public class OperationNameValidation : TypedRule> /// /// Message will be shown at the path level. /// - public override bool IsValid(Dictionary operationDefinition) + public override bool IsValid(string entity, RuleContext context) { - bool areAllOperationNameValid = true; - foreach (KeyValuePair entry in operationDefinition) - { - bool isOperationNameValid = true; - string httpVerb = entry.Key; - string operationId = entry.Value.OperationId; - - if (httpVerb.Equals("GET", StringComparison.InvariantCultureIgnoreCase)) - { - isOperationNameValid = operationId.EndsWith("_Get") || operationId.Contains("_List"); - } - else if (httpVerb.Equals("PUT", StringComparison.InvariantCultureIgnoreCase)) - { - isOperationNameValid = operationId.EndsWith("_Create"); - } - else if (httpVerb.Equals("PATCH", StringComparison.InvariantCultureIgnoreCase)) - { - isOperationNameValid = operationId.EndsWith("_Update"); - } - else if (httpVerb.Equals("DELETE", StringComparison.InvariantCultureIgnoreCase)) - { - isOperationNameValid = operationId.EndsWith("_Delete"); - } - else - { - continue; - } + bool isOperationNameValid = true; + string httpVerb = context?.Parent?.Key; - // If any of the operation name under the path is invalid, then areAllOperationNameValid is false - if (!isOperationNameValid && areAllOperationNameValid) - { - areAllOperationNameValid = isOperationNameValid; - } + if (httpVerb.Equals("GET", StringComparison.InvariantCultureIgnoreCase)) + { + isOperationNameValid = (entity.Contains("_") && (entity.EndsWith("_Get") || entity.Contains("_List"))) || + (entity.StartsWith("get") || entity.StartsWith("list")); + } + else if (httpVerb.Equals("PUT", StringComparison.InvariantCultureIgnoreCase)) + { + isOperationNameValid = (entity.Contains("_") && entity.EndsWith("_Create")) || entity.StartsWith("create"); + } + else if (httpVerb.Equals("PATCH", StringComparison.InvariantCultureIgnoreCase)) + { + isOperationNameValid = (entity.Contains("_") && entity.EndsWith("_Update")) || entity.StartsWith("update"); + } + else if (httpVerb.Equals("DELETE", StringComparison.InvariantCultureIgnoreCase)) + { + isOperationNameValid = (entity.Contains("_") && entity.EndsWith("_Delete")) || entity.StartsWith("update"); } - return areAllOperationNameValid; + return isOperationNameValid; } } }