Filtering resource models returned only by post #2365

dsgouda · 2017-06-14T23:13:01Z

Addressing #2316

olydis · 2017-06-14T23:25:40Z

src/modeler/AutoRest.Swagger/Model/ValidationUtilities.cs

+
+            var unfilteredResourceCandidates = resourceModels.Union(modelsAllOfOnXmsAzureResources);
+
+            // Now filter all the resource models that are returned from a POST operation only 


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.

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

olydis · 2017-06-15T17:07:01Z

src/modeler/AutoRest.Swagger/Model/ValidationUtilities.cs

+            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())))
+                                                                     .Where(model => !string.IsNullOrWhiteSpace(model))
+                                                                     .Except(putOperationsResponseModels)


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

you already missed some HTTP methods, what about PATCH?

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 😉

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".

I literally called it resourceCandidates before changing my mind 😆

@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?

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

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?

veronicagg · 2017-06-15T20:36:57Z

src/modeler/AutoRest.Swagger/Model/ValidationUtilities.cs

+            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())))
+                                                                     .Where(model => !string.IsNullOrWhiteSpace(model))
+                                                                     .Except(putOperationsResponseModels)


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".

vishrutshah · 2017-06-15T21:19:14Z

I do not have any other comments apart from what's already been commented. Feel free to discuss with them and I'd be fine on the decision. I am not posting "Approve" / "Request Changes" as I'll be OOF tomorrow so if it gets resolved, go ahead :)

veronicagg · 2017-06-16T19:29:28Z

@dsgouda apart from updating the name to "resourceCandidates" which I'm not seeing, rest looks good to me. I'd be good to run some of the resource related rules on the repo and make sure we're accurate. Since the rule won't get released right away (not as "latest"), if you'd done validation already you could run on the repo afterwards with the nightly build. thanks!

veronicagg

See my prior comment, pending CI results, I'm good with the rest.

dsgouda · 2017-06-16T20:58:20Z

Thanks @veronicagg I will run the changes against the rest spec repo

dsgouda · 2017-06-17T00:17:32Z

Ran this against a few specs in the rest spec repo, and I still see the resources being picked as expected, we can investigate this further if required.

olydis · 2017-06-19T16:00:17Z

src/modeler/AutoRest.Swagger/Validation/Core/RuleContext.cs

        {
            this.Root = root;
            this.Value = root;
            this.File = file;
-            PopulateResourceTypes(root);
+            if (metadata.ServiceDefinitionDocumentType == ServiceDefinitionDocumentType.ARM)


this change breaks CI:
https://github.com/Azure/autorest/blob/master/src/modeler/AutoRest.Swagger/Validation/BodyTopLevelProperties.cs#L52 fails because resModels is null. Since that didn't happen before, I bet that it is this guard that causes the context.ResourceModels to not be populated...
Now, I see that both this guard and the rule say that they are ARM, but still, this is kind of the only potential point of failure I could make out.

further observation: this == will fail if metadata.ServiceDefinitionDocumentType are multiple types at once!
Instead of duplicating the logic for document type filtering, I'd honestly just make ResourceModels a lazily initialized property! If someone asks for it, populate it, otherwise don't. That way, you automatically inherit the correct behavior from the fact that the rules are classified correctly.

hmm... starting to think if there is any value in adding the check after all.

fair point, don't think anyone severely suffers because of this... main customer is still ARM, for data plane I also assume this goes faster since there is actually not much that looks like a resource. And even then, what's the actual overhead of this? Would be surprised if it took half a second on most Swaggers.

Agreed, even if it does warrant a performance investigation, I think we can revisit this once we move validator to a different repo,

Filtering resource models returned only by post

6ba6fff

dsgouda requested review from vishrutshah, olydis and veronicagg June 14, 2017 23:13

msftclas added the cla-not-required label Jun 14, 2017

olydis reviewed Jun 14, 2017

View reviewed changes

Addressed PR comment

42016bc

olydis reviewed Jun 15, 2017

View reviewed changes

veronicagg suggested changes Jun 15, 2017

View reviewed changes

dsgouda added 2 commits June 16, 2017 12:21

proposing conditional execution of resource evaluation

599bed2

unused

d4d5343

veronicagg approved these changes Jun 16, 2017

View reviewed changes

PR comment

16b14d6

dsgouda closed this Jun 17, 2017

dsgouda reopened this Jun 17, 2017

msftclas added the cla-not-required label Jun 17, 2017

dsgouda added 2 commits June 16, 2017 17:37

test fix

c8a7b2b

regened

d1dec80

olydis approved these changes Jun 17, 2017

View reviewed changes

olydis reviewed Jun 19, 2017

View reviewed changes

dsgouda closed this Jun 19, 2017

dsgouda reopened this Jun 19, 2017

msftclas added the cla-not-required label Jun 19, 2017

testing rollback

af827e3

dsgouda closed this Jun 19, 2017

dsgouda reopened this Jun 19, 2017

msftclas added the cla-not-required label Jun 19, 2017

dsgouda closed this Jun 19, 2017

dsgouda reopened this Jun 19, 2017

msftclas added the cla-not-required label Jun 19, 2017

vishrutshah approved these changes Jun 19, 2017

View reviewed changes

dsgouda added 2 commits June 19, 2017 14:39

Merge branch 'master' of https://github.com/Azure/autorest into i2316

4cc9ab5

regened

4cdde45

olydis merged commit 061461d into Azure:master Jun 19, 2017

dsgouda deleted the i2316 branch June 29, 2017 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filtering resource models returned only by post #2365

Filtering resource models returned only by post #2365

dsgouda commented Jun 14, 2017

olydis Jun 14, 2017

dsgouda Jun 15, 2017

olydis Jun 15, 2017

veronicagg Jun 15, 2017

dsgouda Jun 15, 2017

dsgouda Jun 15, 2017 •

edited

Loading

olydis Jun 16, 2017

dsgouda Jun 16, 2017

veronicagg Jun 15, 2017

vishrutshah commented Jun 15, 2017

veronicagg commented Jun 16, 2017

veronicagg left a comment

dsgouda commented Jun 16, 2017

dsgouda commented Jun 17, 2017

olydis Jun 19, 2017

olydis Jun 19, 2017

dsgouda Jun 19, 2017

olydis Jun 19, 2017

dsgouda Jun 19, 2017


		var unfilteredResourceCandidates = resourceModels.Union(modelsAllOfOnXmsAzureResources);

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

Filtering resource models returned only by post #2365

Filtering resource models returned only by post #2365

Conversation

dsgouda commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsgouda Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishrutshah commented Jun 15, 2017

veronicagg commented Jun 16, 2017

veronicagg left a comment

Choose a reason for hiding this comment

dsgouda commented Jun 16, 2017

dsgouda commented Jun 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsgouda Jun 15, 2017 •

edited

Loading