No way to omit redundant ID parameter on generated PUT methods

[brjohnstmsft]

Consider an operation like this one that creates or updates a resource via HTTP PUT (omitting some parts of the Swagger for clarity):

  "/indexes('{indexName}')": {
    "put": {
      "operationId": "Indexes_CreateOrUpdate",
      "parameters": [
        {
          "name": "indexName",
          "in": "path",
          "required": true,
          "type": "string",
          "description": "The definition of the index to create or update."
        },
        {
          "name": "index",
          "in": "body",
          "required": true,
          "schema": {
            "$ref": "#/definitions/Index"
          },
          "description": "The definition of the index to create or update."
        }
      ],
      "responses": {
        "200": {
          "description": "",
          "schema": {
            "$ref": "#/definitions/Index"
          }
        },
        "201": {
          "description": "",
          "schema": {
            "$ref": "#/definitions/Index"
          }
        }
      }
    },

Currently AutoRest will generate signatures like this (using C# as an example):

public static Index CreateOrUpdate(this IIndexesOperations operations, string indexName, Index index)

It would be much cleaner for users of the client library if the generated code could populate the resource ID in the URL path from the resource object itself (in this example, grabbing the indexName from the Index). If it could do that, the method signature would omit the redundant parameter:

public static Index CreateOrUpdate(this IIndexesOperations operations, Index index)
// Generated code internally does: string indexName = index.Name;

Unfortunately there is no way to use expressions in Swagger to bind parts of a model object to a parameter. Maybe we can add an extension for this?

[stankovski]

Following implementation of InputParameterTransformations we have all the necessary hooks to add this functionality. We just need to come up with an extension and update swagger modeler. Please feel free to contribute ideas and/or PRs.

[stankovski]

Tracking via: https://www.pivotaltracker.com/story/show/107070408

[stankovski]

This functionality is now available via #687

[brjohnstmsft]

If I understand x-ms-client-flatten correctly, it can basically inline body parameter properties as method arguments, but I don't see how it can implement the transformation I outlined above, where this:

  "/indexes('{indexName}')": {
    "put": {
      "operationId": "Indexes_CreateOrUpdate",
      "parameters": [
        {
          "name": "indexName",
          "in": "path",
          "required": true,
          "type": "string",
          "description": "The definition of the index to create or update."
        },
        {
          "name": "index",
          "in": "body",
          "required": true,
          "schema": {
            "$ref": "#/definitions/Index"
          },
          "description": "The definition of the index to create or update."
        }
      ],
      "responses": {
        "200": {
          "description": "",
          "schema": {
            "$ref": "#/definitions/Index"
          }
        },
        "201": {
          "description": "",
          "schema": {
            "$ref": "#/definitions/Index"
          }
        }
      }
    },

Becomes this:

public static Index CreateOrUpdate(this IIndexesOperations operations, Index index)
// Generated code internally does: string indexName = index.Name;

Am I missing something?

[stankovski]

You can take a look at this test swagger. x-ms-client-flatten can be combined with parameter grouping allowing you to produce a parameter that looks like a body, but has a path parameter as well.

[brjohnstmsft]

I see. That sort of works, but it's a shame that it has to duplicate the model class like that.

A while ago you mentioned this possibility:

Following implementation of InputParameterTransformations we have all the necessary hooks to add this functionality. We just need to come up with an extension and update swagger modeler.

Whatever became of that?

[stankovski]

The thing is, if we add a property to the same model that is used on the wire, we will need to have a way to remove it from both send and receive payloads. We will also need logic to generate a duplicate model in case the payload is used in GET method. This current approach seems like the most flexible way of achieving this functionality.

[brjohnstmsft]

In my example, the Name property is already there, it's just duplicated between the body and the path in the REST API (and therefore in the Swagger). This is a very common pattern for REST APIs. For the case where name is separate from the body, I see how the flattening solution works better. It just doesn't really work for our case (which I would argue is the common case).

[stankovski]

I see. Why do you have the same value in the body and in the path though? What if the values provided by the user are different? That seems like an odd REST API design.

[brjohnstmsft]

Normally you wouldn't have the value in both places, but it can potentially be in both places because the body of a PUT is modeled with the same type as a POST or a GET. I notice a lot of the Management SDK Swaggers have a separate type for PUT (e.g. -- Profile for GET, ProfileCreateOrUpdateParameters for PUT) which is an ugly hack IMO. Also, our API supports POST, not just PUT, and it would be even worse to use two different types for those two operations.

Our API takes the value from the path for PUT, but it also checks for a value in the body and returns 400 if there is one there that doesn't match.

[stankovski]

OK, I can see an argument here. Will return the issue to keep it on the radar.

[brjohnstmsft]

Thanks!

[brjohnstmsft]

@devigned @stankovski Any further thoughts on this? Other folks who might have opinions? I'm willing to take it on if we can agree on an approach.

[brjohnstmsft]

I thought of a scenario where it makes sense for CreateOrUpdate to take both the "id" parameter and the resource body with the "id" property in it: Renaming resources. Our API doesn't support this now, but it seems like a reasonable use of PUT. Therefore we are ok with having this particular overload in the generated code.

It would still be nice if AutoRest could also auto-generate a second set of CreateOrUpdate methods without the extra parameter, but that requires some thought and is not a high priority for us right now.

[fearthecowboy]

Howdy!

In our planning for driving towards a stable '1.0' release, I'm marking this issue as 'deferred' 💤 and we're going to review it during the post-1.0 planning cycle.

It's not to say that we're not going to work on it, or that this isn't not important, but at the moment, we're picking and choosing the stuff we must do before 1.0. 🏇 🏇 🏇

We'll make sure we pick this back up at that point. 🎉