Add Parameter Grouping to AutoRest for Azure.CSharp, Azure.NodeJS #381

matthchr · 2015-10-08T17:17:40Z

A "Parameter group" is just a nicer way to bundle parameters so that methods don't get horribly long. It is controlled by an extension to the Swagger specification.

Left out Ruby/Java for now, as Java doesn't have an "Azure.Java" yet and cut Ruby in the name of time (if/when somebody wants the feature in Ruby they should be able to follow the implementation from NodeJS/C#/Java since the metadata is in the client model now)

azuresdkci · 2015-10-08T17:17:41Z

Can one of the admins verify this patch?

azuresdkci · 2015-10-08T17:17:41Z

Can one of the admins verify this patch?

azurecla · 2015-10-08T17:17:44Z

Hi @matthchr, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (matthchr). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, or work for Microsoft Open Technologies, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

amarzavery · 2015-10-08T18:59:04Z

@matthchr - It is a nice concept. As per the current implementation, the user can group parameters in 1 bucket. What if the user wants to group them into more than 1 bucket? Would it be a better idea if the extension takes a key value pair instead of a Boolean, something like this:

"x-ms-parameter-grouping" : {"groupName" : "group1" }

In this way the user can put parameters into different buckets if required. It also provides a flexibility to name the parameter group as per their requirement.

What do you think?

matthchr · 2015-10-08T19:09:35Z

@amarzavery Yes, allowing more than one parameter group was something I thought about (if you see in the implementation the "groups" is actually a collection and everywhere when I generate the code I treat it as such), so the only thing currently stopping us from having multiple groups is the extension piece...

With that said are some additional complications with allowing multiple parameter groups which is why I leaned against it initially:

You must allow swagger to specify "type" names then (which only apply to some languages?)
You must allow swagger to specify "parameter" names (instead of just hardcoding the string "parameter") - maybe this could be sourced off of the same string as the "type" names issue mentioned above
You have to handle type-collisions better then, which I wasn't really sure how to do. Since right now the type of the parameter group class is based on the OperationId (which I believe is unique) we're protected from that naturally.

I agree it would be good to have, but maybe leave it for a V2 implementation?
I know that my team at least is happy with even just 1 parameter group (i.e. 1 is better than 0 for us, but N is not much better than 1)

edit: Okay I tried this and it turns out it was pretty easy to do (I thought it would be harder), so I added support for this right out of the box (and added some tests to make sure it works). Let me know what you think.

brjohnstmsft · 2015-10-08T20:44:27Z

Have you considered a more general approach? Rather than indirectly define a model type by grouping parameters, it would be really handy if we could bind some parameter values to others. For example, for PUT operations, it would be really nice to be able to bind the URL parameter for the resource ID to the corresponding property on the resource itself. See this issue for a concrete example: #382

matthchr · 2015-10-08T21:01:36Z

@brjohnstmsft David Justice had discussed something similar to what you're asking for - he colloquially called it "parameter factories" or something. My team has a similar request, but instead of a "duplicate" parameter like in your example (where there is Model.Id and id), we want to set certain parameters to be automatically generated instead of including them in the signature.

For example: parameter "string requestId" (required: true) could instead be optional or omitted entirely and automatically generated as a Guid (or in your case automatically generated from an existing model field).

I do see the two things being different features though: this is about how to structure the parameters we do want, what you're asking for is a way to omit parameters which are "required" by the REST API but which you do not want.

yugangw-msft · 2015-10-13T20:19:07Z

add to whitelist

yugangw-msft · 2015-10-13T21:02:10Z

AutoRest/Generators/Azure.Common/Azure.Common/AzureCodeGenerator.cs

+
+            foreach (Method method in serviceClient.Methods)
+            {
+                //TODO: Does this group name need to be normalized later, or does that already happen?


it will be normalized by each code generator, so you are good

@yugangw-msft Updated comment to reflect that fact

matthchr · 2015-10-15T21:30:10Z

Okay, I moved the Parameters objects into the models folder and namespace.

matthchr · 2015-10-15T22:04:29Z

@yugangw-msft I am not sure why the "default" check failed in jenkins, can somebody help me take a look at that? Travis pass seems okay...

yugangw-msft · 2015-10-15T23:22:45Z

@matthchr please merge with dev. The CI failure was caused by the java change made to the dev branch this afternoon

matthchr · 2015-10-15T23:47:10Z

@yugangw-msft Merge with dev done

matthchr · 2015-10-17T01:03:22Z

@devigned @stankovski Should we use these comments to discuss proposed changes to this implementation?

One thing that @xingwu1 pointed out to me is that this PR is pretty critical to our adoption, so I don't really want to have to re-design/re-do everything (especially if it's significantly more complicated to do an alternate way) because it will delay our swagger adoption (after this PR, we can actually generate a real client).

If we can align on:

The Swagger representation (tentatively, I think what I have now is fine).
The generated code structure (I think what I have now is fine).
A model structure which is generic enough for now.

If we change the internals of AutoRest (the model structure) in the future to be more generic or cater to more scenarios, that change is fine because it's hidden from clients generated code and their swagger specifications.

I'll start with my suggestion:

Swagger extension format stays as it is in this PR (a piece of metadata on the Parameter object in the swagger spec).
Client model is updated to have some sort of "mapped" parameter notion. Basically, there can be parameters in the ParameterList, and there will be a dictionary mapping ParameterList parameters to "real" parameters, and there will be some generic methods which will transform the parameters in the list to parameters in the internals of the method. At the end of the day the generated method code will still look almost exactly like I have it in the PR today (top of the method will extract stuff out of the object into a set of named local variables, other pieces of the method will consume the local variables).

I think that the part which might be reusable is the stuff for the client model, but even that is maybe not totally code-reuse but rather just concept re-use... what I mean is this:

For input parameters, there is this notion of a ParameterList, and a dictionary or something which describes the mapping of Parameters -> UnwrappedParameters.
For output parameters, there is the reverse notion - you have a set of parameters and a mapping collapsing them into a new type, so it's UnwrappedParameters -> OutputParameters.

Let me know what you think.

stankovski · 2015-10-19T15:13:16Z

AutoRest/AutoRest.Core/ClientModel/Method.cs

@@ -59,6 +59,27 @@ public Method()
        public List<Parameter> Parameters { get; private set; }

        /// <summary>
+        /// Gets the parameter groups.
+        /// </summary>
+        public IEnumerable<string> ParameterGroups


This will be replaced with two "Mapping" properties - one for input and one for output.

stankovski · 2015-10-19T15:27:58Z

@matthchr I agree with the approach. Let's leave swagger extension as-is. I also think that the generated code will stay almost the same. An output block of similar structure will be needed and you may need to convert terniary expression to an if block for mapping from primitives to a class.

As far as mapping, I would leave current ParameterList as is and add a new data structure (either just a dictionanry or a class to work with output mapping) that matches an existing parameter or parameter property to a new parameter or parameter property.

matthchr · 2015-10-19T17:01:41Z

@stankovski I've created an issue #395 to discuss the structure of the returned object for the return header -> object path. Can we delay the implementation of that until we've decided on what the output object should look like (is it a union of the returned headers and the body, a <T, K> double-generic (in C#) or something else?).

Until then, I think I won't be able to write any test cases or samples for the "re-grouping" case (since there will be no place to put the returned header seeing as the body is already populated by the JSON deserialization), so I will hold off on writing it. When we get there, we can follow the exact same pattern as the un-grouping case in this PR.

Does that sound okay?

matthchr · 2015-10-20T17:23:17Z

@stankovski This has the changes to ParameterGrouping to move to the "Mapping" idea -- the CI is failing due to an issue in dev branch -- once that is fixed I'll merge and update.

stankovski · 2015-10-21T15:54:55Z

AutoRest/AutoRest.Core/ClientModel/Method.cs

@@ -59,6 +60,22 @@ public Method()
        public List<Parameter> Parameters { get; private set; }

        /// <summary>
+        /// Gets the list of Parameter Expansions
+        /// </summary>
+        public Dictionary<string, Dictionary<Property, Parameter>> ParameterExpansions { get; private set; }


To make it more generic and support parameter groups as well primitive to object mapping can we do the following:

Rename this to ParameterMappings

Add new type to ClientModel called ParameterMapping with the following properties

Parameter InputParameter {get; set;}

string InputParameterProperty {get; set;} (either null, which will map the parameter itself, or dot separated string to match properties)

Parameter OutputParameter {get; set;}

string OutputParameterProperty {get; set;} (either null, which will map the parameter itself, or dot separated string to match properties)

We should also add a public Parameter Body {get;} to get easier access to the body parameter

- Support in C#, NodeJS, and partially in Java.

Add Parameter Grouping to AutoRest for Azure.CSharp, Azure.NodeJS

* Add examples in body-string * Update examples * Update examples * Update package lock version * Update format

azurecla added the cla-not-required label Oct 8, 2015

matthchr force-pushed the feature/parameter-grouping branch from 18230d7 to 105ea6a Compare October 8, 2015 18:08

matthchr force-pushed the feature/parameter-grouping branch from af20e76 to 73e51d2 Compare October 12, 2015 16:36

yugangw-msft reviewed Oct 13, 2015
View reviewed changes

matthchr force-pushed the feature/parameter-grouping branch from 2acb4ee to 6a03ddf Compare October 15, 2015 21:29

matthchr force-pushed the feature/parameter-grouping branch from 6a03ddf to d09a149 Compare October 15, 2015 23:46

matthchr force-pushed the feature/parameter-grouping branch from d09a149 to d21bea0 Compare October 16, 2015 00:18

stankovski reviewed Oct 19, 2015
View reviewed changes

matthchr force-pushed the feature/parameter-grouping branch from d21bea0 to e9d74c3 Compare October 20, 2015 17:07

stankovski reviewed Oct 21, 2015
View reviewed changes

matthchr force-pushed the feature/parameter-grouping branch 2 times, most recently from dde26e6 to 357d061 Compare October 22, 2015 22:06

Add parameter grouping to AutoRest

3791d93

- Support in C#, NodeJS, and partially in Java.

matthchr force-pushed the feature/parameter-grouping branch from 357d061 to 3791d93 Compare October 22, 2015 23:54

stankovski added a commit that referenced this pull request Oct 23, 2015

Merge pull request #381 from matthchr/feature/parameter-grouping

21dfdca

Add Parameter Grouping to AutoRest for Azure.CSharp, Azure.NodeJS

stankovski merged commit 21dfdca into Azure:dev Oct 23, 2015

matthchr deleted the feature/parameter-grouping branch November 3, 2015 19:05

timayabi2020 pushed a commit to timayabi2020/autorest that referenced this pull request Jan 22, 2025

Adding examples in body-string swagger file (Azure#381)

53da532

* Add examples in body-string * Update examples * Update examples * Update package lock version * Update format

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Parameter Grouping to AutoRest for Azure.CSharp, Azure.NodeJS #381

Add Parameter Grouping to AutoRest for Azure.CSharp, Azure.NodeJS #381

matthchr commented Oct 8, 2015

azuresdkci commented Oct 8, 2015

azuresdkci commented Oct 8, 2015

azurecla commented Oct 8, 2015

amarzavery commented Oct 8, 2015

matthchr commented Oct 8, 2015

brjohnstmsft commented Oct 8, 2015

matthchr commented Oct 8, 2015

yugangw-msft commented Oct 13, 2015

yugangw-msft Oct 13, 2015

matthchr Oct 14, 2015

matthchr commented Oct 15, 2015

matthchr commented Oct 15, 2015

yugangw-msft commented Oct 15, 2015

matthchr commented Oct 15, 2015

matthchr commented Oct 17, 2015

stankovski Oct 19, 2015

stankovski commented Oct 19, 2015

matthchr commented Oct 19, 2015

matthchr commented Oct 20, 2015

stankovski Oct 21, 2015

stankovski Oct 21, 2015

Add Parameter Grouping to AutoRest for Azure.CSharp, Azure.NodeJS #381

Add Parameter Grouping to AutoRest for Azure.CSharp, Azure.NodeJS #381

Conversation

matthchr commented Oct 8, 2015

azuresdkci commented Oct 8, 2015

azuresdkci commented Oct 8, 2015

azurecla commented Oct 8, 2015

amarzavery commented Oct 8, 2015

matthchr commented Oct 8, 2015

brjohnstmsft commented Oct 8, 2015

matthchr commented Oct 8, 2015

yugangw-msft commented Oct 13, 2015

yugangw-msft Oct 13, 2015

Choose a reason for hiding this comment

matthchr Oct 14, 2015

Choose a reason for hiding this comment

matthchr commented Oct 15, 2015

matthchr commented Oct 15, 2015

yugangw-msft commented Oct 15, 2015

matthchr commented Oct 15, 2015

matthchr commented Oct 17, 2015

stankovski Oct 19, 2015

Choose a reason for hiding this comment

stankovski commented Oct 19, 2015

matthchr commented Oct 19, 2015

matthchr commented Oct 20, 2015

stankovski Oct 21, 2015

Choose a reason for hiding this comment

stankovski Oct 21, 2015

Choose a reason for hiding this comment