-
Notifications
You must be signed in to change notification settings - Fork 737
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
Adding validation rules for subscriptionId/api-version params #1513
Conversation
If you're changing JSON files, make sure you pull all the latest code and do a |
@@ -252,4 +252,10 @@ | |||
<data name="InvalidConstraint" xml:space="preserve"> | |||
<value>Constraint is not supported for this type and will be ignored.</value> | |||
</data> | |||
<data name="OperationParametersNotAllowedMessage" xml:space="preserve"> | |||
<value>Parameters "subscriptionId" and "api-version" are not allowed in the operations section of azure swagger specs</value> |
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.
Parameters "subscriptionId" and "api-version" are not allowed in the operations section
? #Resolved
/// <returns></returns> | ||
public override bool IsValid(Dictionary<string, SwaggerParameter> ParametersMap) | ||
{ | ||
if (ParametersMap.Count==0 || ParametersMap == null) |
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.
Braces please. 😀
if (ParametersMap.Count==0 || ParametersMap == null)
{
return false;
}
return false; | ||
|
||
var Names = ParametersMap.Values.Select(Param => Param.Name).ToList(); | ||
return Names.Contains(SubscriptionId) && Names.Contains(ApiVersion); |
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.
The code
var Names = ParametersMap.Values.Select(Param => Param.Name).ToList();
return Names.Contains(SubscriptionId) && Names.Contains(ApiVersion);
is iterating the ParametersMap.Values
enumerable and allocating a List
for no benefit.
.ToList()
should be reserved for when you actually need to manipulate the result set of an enumeration.
The following will iterate the set only once, and will return at the first true
case. If there are a 1000 items in the list, and the first is matched, the iterating stops there. The happy path is the fast path ⏩
return ParametersMap.Values.Any( parameter => parameter.Name == SubscriptionId || parameter.Name == ApiVersion );
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.
This helps my limited knowledge of Linq 😄
/// <returns></returns> | ||
public override bool IsValid(Dictionary<string, SwaggerParameter> ParametersMap) | ||
{ | ||
if (ParametersMap.Count==0 || ParametersMap == null) |
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.
Actually, you don't want to check the count
; that presumes either that the dictionary implementation knows the actual item count (which not all do), or you're forcing the dictionary to count the items. The iteration below will be nearly 0 time if the collection is empty anyway.
So replace this with
if( ParametersMap == null )
{
return false;
}
|
||
var Names = ParametersMap.Values.Select(Param => Param.Name).ToList(); | ||
return Names.Contains(SubscriptionId) && Names.Contains(ApiVersion); | ||
} |
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.
Now, I get to the bottom and see that you could simplify it a bit more, and use a single expression that leverages null-propagation operator (which turns the expression result into a nullable bool result bool?
which can be trivially coaxed back into a bool by adding true ==
at the front of the expression.
public override bool IsValid(Dictionary<string, SwaggerParameter> ParametersMap) =>
true == ParametersMap?.Values.Any( parameter => parameter.Name == SubscriptionId || parameter.Name == ApiVersion );
/// <param name="paths"></param> | ||
/// <returns></returns> | ||
public override bool IsValid(SwaggerParameter Parameter) | ||
{ |
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.
rather than performing the separate checks on the Parameter
and Parameter.Name
, you can use the null-propagation operator to fold it into a single check and let the compiler to the heavy lifting.
public override bool IsValid(SwaggerParameter Parameter) =>
SubscriptionId != Parameter?.Name &&
ApiVersion != Parameter?.Name;
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.
New concepts, taking 🗒
LGTM 👍 |
@azuresdkci retest this please |
Adding validation rules to enforce:
Modified all swagger validation test jsons to adhere to these validation rules
Fixing issue #1515
@tbombach