-
Notifications
You must be signed in to change notification settings - Fork 737
[Linter] Adding rule to warn about boolean type properties #1783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b1722a8
fe98335
1361647
f238ce0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| { | ||
| "swagger": "2.0", | ||
| "info": { | ||
| "title": "Boolean properties not recommended in models", | ||
| "description": "Some documentation.", | ||
| "version": "2017-02-08" | ||
| }, | ||
| "host": "management.azure.com", | ||
| "schemes": [ | ||
| "https" | ||
| ], | ||
| "basePath": "/", | ||
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "consumes": [ | ||
| "application/json" | ||
| ], | ||
| "paths": { | ||
| "/foo": { | ||
| "post": { | ||
| "operationId": "PostFoo", | ||
| "summary": "Foo path", | ||
| "description": "Foo operation", | ||
| "responses": { | ||
| "default": { | ||
| "description": "Unexpected error" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "definitions": { | ||
| "Test1": { | ||
| "description": "Property for foo path 1", | ||
| "properties": { | ||
| "nameAvailable": { | ||
| "readOnly": true, | ||
| "type": "boolean", | ||
| "description": "Gets a boolean value that indicates whether the name is available for you to use. If true, the name is available. If false, the name has already been taken or invalid and cannot be used." | ||
| } | ||
| } | ||
| }, | ||
| "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" | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,7 +94,7 @@ public ServiceDefinition() | |
|
|
||
| /// <summary> | ||
| /// Key is the object serviceTypeName and the value is swagger definition. | ||
| /// </summary> | ||
| [Rule(typeof(BooleanPropertyNotRecommended))] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: deleted line |
||
| [Rule(typeof(ResourceModelValidation))] | ||
| [Rule(typeof(TrackedResourceValidation))] | ||
| [Rule(typeof(ResourceIsMsResourceValidation))] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
|
||
| using AutoRest.Core.Logging; | ||
| using AutoRest.Core.Properties; | ||
| using AutoRest.Core.Validation; | ||
| using System.Collections.Generic; | ||
| using AutoRest.Swagger.Model; | ||
|
|
||
| namespace AutoRest.Swagger.Validation | ||
| { | ||
| /// <summary> | ||
| /// Flags properties of boolean type as they are not recommended, unless it's the only option. | ||
| /// </summary> | ||
| public class BooleanPropertyNotRecommended : TypedRule<Dictionary<string, Schema>> | ||
| { | ||
| /// <summary> | ||
| /// The template message for this Rule. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This may contain placeholders '{0}' for parameterized messages. | ||
| /// </remarks> | ||
| public override string MessageTemplate => Resources.BooleanPropertyNotRecommended; | ||
|
|
||
| /// <summary> | ||
| /// The severity of this message (ie, debug/info/warning/error/fatal, etc) | ||
| /// </summary> | ||
| public override Category Severity => Category.Warning; | ||
|
|
||
| /// <summary> | ||
| /// Validates whether properties of type boolean exist. | ||
| /// </summary> | ||
| /// <param name="definitions">Operation Definition to validate</param> | ||
| /// <returns>true if there are propeties of type boolean, false otherwise.</returns> | ||
| public override bool IsValid(Dictionary<string, Schema> definitions, RuleContext context, out object[] formatParameters) | ||
| { | ||
| formatParameters = null; | ||
| List<string> booleanProperties = new List<string>(); | ||
| foreach (string key in definitions.Keys) | ||
| { | ||
| Schema definitionSchema = definitions.GetValueOrNull(key); | ||
| if (definitionSchema.Properties != null) | ||
| { | ||
| foreach (var property in definitionSchema.Properties) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can probably be simplified using Linq's .Select() construct, will leave it to your discretion |
||
| { | ||
| if (property.Value.Type.ToString().ToLower().Equals("boolean")) | ||
| { | ||
| booleanProperties.Add(key + "/" + property.Key); | ||
|
|
||
| } | ||
| } | ||
| } | ||
| } | ||
| formatParameters = new[] { string.Join(", ", booleanProperties.ToArray()) }; | ||
| return (booleanProperties.Count == 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly you are returning a true (or valid) if there are no boolean properties, I believe the comments don't reflect it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, thanks! I'll make another PR for the fix as this one got merged. |
||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per @amarzavery we should be adding "positive" tests too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure which positive test to add in this case, since many of the other tests already don't have boolean properties included. For example, "clean-complex-spec.json" does not have boolean properties and therefore doesn't return this warning. If you think that's not enough, just let me know and I can add something else.