Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Filtering resource models returned only by post #2365

Merged
merged 10 commits into from
Jun 19, 2017
Merged

Filtering resource models returned only by post #2365

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6ba6fff
Filtering resource models returned only by post
dsgouda Jun 14, 2017
42016bc
Addressed PR comment
dsgouda Jun 15, 2017
599bed2
proposing conditional execution of resource evaluation
dsgouda Jun 16, 2017
d4d5343
unused
dsgouda Jun 16, 2017
16b14d6
PR comment
dsgouda Jun 16, 2017
c8a7b2b
test fix
dsgouda Jun 17, 2017
d1dec80
regened
dsgouda Jun 17, 2017
af827e3
testing rollback
dsgouda Jun 19, 2017
4cc9ab5
Merge branch 'master' of https://github.com/Azure/autorest into i2316
dsgouda Jun 19, 2017
4cdde45
regened
dsgouda Jun 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions ....Swagger.Tests/Resource/Swagger/Validation/positive/valid-resource-model-definitions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,21 @@
}
}
}
},
"/foo/baz": {
"post": {
"operationId": "AzureRes_Fetch",
"description": "fetches an azure res",
"parameters": [],
"responses": {
"200": {
"description": "the resource",
"schema": {
"$ref": "#/definitions/PostRespAzureResource"
}
}
}
}
}
},
"definitions": {
Expand Down Expand Up @@ -218,6 +233,19 @@
"description": "Resource type"
}
}
},
"PostRespAzureResource": {
"description": "resource returned by a post operation",
"allOf": [
{
"$ref": "#/definitions/ProxyResource"
}
],
"properties": {
"randomSampleProp": {
"type": "string"
}
}
}
}
}
2 changes: 1 addition & 1 deletion src/modeler/AutoRest.Swagger.Tests/SwaggerModelerValidationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ public void ValidResourceModels()
var servDef = SwaggerParser.Parse(filePath, fileText);
Uri uriPath = null;
Uri.TryCreate(filePath, UriKind.RelativeOrAbsolute, out uriPath);
var context = new RuleContext(servDef, uriPath);
var context = new RuleContext(servDef, uriPath, new ServiceDefinitionMetadata() { MergeState = ServiceDefinitionDocumentState.Composed, ServiceDefinitionDocumentType = ServiceDefinitionDocumentType.ARM });
Assert.Equal(4, context.ResourceModels.Count());
Assert.Equal(1, context.TrackedResourceModels.Count());
Assert.Equal(3, context.ProxyResourceModels.Count());
Expand Down
14 changes: 11 additions & 3 deletions src/modeler/AutoRest.Swagger/Model/ValidationUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,18 @@ public static IEnumerable<string> GetResourceModels(ServiceDefinition serviceDef
.Where(modelName => !(IsBaseResourceModelName(modelName))
&& serviceDefinition.Definitions.ContainsKey(modelName)
&& IsAllOfOnModelNames(modelName, serviceDefinition.Definitions, xmsAzureResourceModels));

// return the union
return resourceModels.Union(modelsAllOfOnXmsAzureResources);

var resourceCandidates = resourceModels.Union(modelsAllOfOnXmsAzureResources);

// Now filter all the resource models that are returned from a POST operation only
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the "only" part reflected in the code but maybe I'm missing it.
Also, the indentation is misleading: the .SelectMany calls are perfectly aligned but are not operating on the same "level". I'd expect the second SelectMany to be on the same height as the inner Where, just break pathObj => pathObj<HERE>.Where and lines won't be that long.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d'oh (I went everywhere looking for a Homer emoji!)

var postOpResourceModels = serviceDefinition.Paths.Values.SelectMany(pathObj => pathObj.Where(opObj => opObj.Key.EqualsIgnoreCase("post"))
.SelectMany(opObj => opObj.Value.Responses?.Select(resp => resp.Value?.Schema?.Reference?.StripDefinitionPath())??Enumerable.Empty<string>()))
.Where(model => !string.IsNullOrWhiteSpace(model))
.Except(putOperationsResponseModels)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't implement the "only" by relying on other sets and excepting them.

  1. you already missed some HTTP methods, what about PATCH?
  2. it's not obvious that putOperationsResponseModels contains all models of PUT methods. Next month, someone applies a fix to something by changing that set, not realizing that it breaks this line. Locality 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about looking into the models returned by operations and selecting the ones that are only returned by posts? then removing those from the previous set? I would probably not call it "unfiltered" set, since it's already quite filtered, maybe "resourceCandidates".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I literally called it resourceCandidates before changing my mind 😆

Copy link
Contributor Author

@dsgouda dsgouda Jun 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@veronicagg that is what I am precisely trying to do
@olydis from what @ravbhatnagar indicated on the issue, I should really be looking at anything returned by a PUT/GET operation only.
Also, models being populated in the putOperationsResponseModels are basically resource model candidates and this particular filtering is applicable only to the resource models, would I still miss out on anything here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, how does the rule perform on the specs repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is not a rule per se, more like pre cursor step to running any arm related validation rules.
This is a one off operation and the results are stored in the context object passed to every single rule.
This makes me wonder if we should run this logic even if we do not have any arm specific rules to run, given how expensive these operations are its worth a look. what say?

.Except(getOperationsResponseModels);

// if any model is returned only by a POST operation, disregard it
return resourceCandidates.Except(postOpResourceModels);
}

public static bool IsODataProperty(string propName) => propName.ToLower().StartsWith("@");
Expand Down
2 changes: 1 addition & 1 deletion src/modeler/AutoRest.Swagger/Validation/Core/RecursiveObjectValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public RecursiveObjectValidator(Func<PropertyInfo, string> resolver)
/// <param name="metaData">The metadata associated with serviceDefinition to which <paramref name="metaData"/> belongs to</param>
public IEnumerable<LogMessage> GetValidationExceptions(Uri filePath, ServiceDefinition entity, ServiceDefinitionMetadata metadata)
{
return RecursiveValidate(entity, ObjectPath.Empty, new RuleContext(entity, filePath), Enumerable.Empty<Rule>(), metadata);
return RecursiveValidate(entity, ObjectPath.Empty, new RuleContext(entity, filePath, metadata), Enumerable.Empty<Rule>(), metadata);
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/modeler/AutoRest.Swagger/Validation/Core/RuleContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public class RuleContext
/// Initializes a top level context for rules
/// </summary>
/// <param name="root"></param>
public RuleContext(ServiceDefinition root, Uri file) : this(null)
public RuleContext(ServiceDefinition root, Uri file, ServiceDefinitionMetadata metadata) : this(null)
{
this.Root = root;
this.Value = root;
Expand Down