Autorest: NodeJS code generator / client doesn't fill result correctly on successful write requests

[aschua]

Hello!

Using the samples from https://github.com/Azure/autorest/tree/master/Samples/petstore/NodeJS to access the petstore API the responses of write (for example addPet / updatePet) requests aren't processed correct.

For example, the code

var uuid = require('uuid');
var SwaggerPetstore = require('./petstore-generated/swaggerPetstore');

var psClient = new SwaggerPetstore('http://petstore.swagger.io/v2');

var pet = {
    "name": uuid.v4(),
    "category": {
        "id": 2,
        "name": "Cats"
    },
    "photoUrls": [],
    "tags": [],
    "status": "available"
};

psClient.addPet({body: pet}, function (err, result, req, res) {
    console.log('err:  ', err);
    console.log('result:  ', result);
    console.log('req:  ', req);
    console.log('res:  ', res);
});

produces the output:

err:   { [Error: {"id":1458394567226,"category":{"id":2,"name":"Cats"},"name":"3ec431eb-fc36-4d73-851c-b266a0d4efda","photoUrls":[],"tags":[],"status":"available"}]
  statusCode: 200,
  request: 
   { rawResponse: false,
     queryString: {},
     method: 'POST',
     headers: { 'Content-Type': 'application/json; charset=utf-8' },
     url: 'http://petstore.swagger.io/v2/pet',
     body: '{"category":{"id":2,"name":"Cats"},"name":"3ec431eb-fc36-4d73-851c-b266a0d4efda","photoUrls":[],"tags":[],"status":"available"}' },
  response: 
   { body: '{"id":1458394567226,"category":{"id":2,"name":"Cats"},"name":"3ec431eb-fc36-4d73-851c-b266a0d4efda","photoUrls":[],"tags":[],"status":"available"}',
     headers: 
      { date: 'Sat, 19 Mar 2016 16:08:01 GMT',
        'access-control-allow-origin': '*',
        'access-control-allow-methods': 'GET, POST, DELETE, PUT',
        'access-control-allow-headers': 'Content-Type, api_key, Authorization',
        'content-type': 'application/json',
        connection: 'close',
        server: 'Jetty(9.2.9.v20150224)' },
     statusCode: 200 } }
result:   undefined
req:   undefined
res:   undefined

As you can see err.statusCode is 200 and in err.response.body resides the data, that I would expect in the result object.

[devigned]

Yes, that should be a success. @amarzavery thoughts?

[aschua]

The relevant part of the addPet()-Method is this

var statusCode = response.statusCode;
    if (statusCode !== 405) {
      var error = new Error(responseBody);
      error.statusCode = response.statusCode;
      error.request = msRest.stripRequest(httpRequest);
      error.response = msRest.stripResponse(response);
      if (responseBody === '') responseBody = null;
      var parsedErrorResponse;
      try {
        parsedErrorResponse = JSON.parse(responseBody);
        if (parsedErrorResponse) {
          if (parsedErrorResponse.error) parsedErrorResponse = parsedErrorResponse.error;
          if (parsedErrorResponse.code) error.code = parsedErrorResponse.code;
          if (parsedErrorResponse.message) error.message = parsedErrorResponse.message;
        }
      } catch (defaultError) {
        error.message = util.format('Error "%s" occurred in deserializing the responseBody ' + 
                         '- "%s" for the default response.', defaultError.message, responseBody);
        return callback(error);
      }
      return callback(error);
    }
    // Create Result
    var result = null;
    if (responseBody === '') responseBody = null;

    return callback(null, result, httpRequest, response);

The test for statusCode !== 405 seems wired and at the bottom the result is nulled. An similar construct is found in the updatePet() method.
The statusCode should be testet to be not in the 2xx-range and the result creation at the bottom should be result = JSON.parse(responseBody) if body is not empty.

[amarzavery]

@devigned
Yes that seems incorrect. Looks like the swagger spec has only one response 405 documented and no defaults. In that scenario, autorest considers that response as the positive response.

Which means all the generated clients will do the same thing.

• C#
• Python
• Ruby
• Java
  
  
  
  • add delegate for success
  
  
  • definition of delegate
  
  

We will have to change the logic for processing responses with single response value in our modeler

[amarzavery]

@aschua - Thanks for reporting this.
@devigned - Take a look at this old version of the petstore swagger spec that the team at Open API spec was using. They had 200 and 405 defined at that time.

Quoting the following thing from the explanation of Responses in the Open API spec

Responses Object

A container for the expected responses of an operation. The container maps a HTTP response code to the expected response. It is not expected from the documentation to necessarily cover all possible HTTP response codes, since they may not be known in advance. However, it is expected from the documentation to cover a successful operation response and any known errors.

The default can be used a default response object for all HTTP codes that are not covered individually by the specification.

The Responses Object MUST contain at least one response code, and it SHOULD be the response for a successful operation call.

So their latest version of the spec is incorrect. We are doing the right thing.

[amarzavery]

Have filed an issue at the Open API spec repo.

[devigned]

Thank you for the awesome response, @amarzavery!

[aschua]

@amarzavery / @devigned: Thanks for your fast responses and explanation!

[amarzavery]

This is a bug in the swagger spec and not with node.js codegen. Hence, removed the label node.js

[amarzavery]

May be creating a new label called Open API Spec and tagging it over here helps for visibility.

[fearthecowboy]

Howdy!

In our planning for driving towards a stable '1.0' release, I'm marking this issue as 'deferred' :zzz: 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._ :horse_racing: :horse_racing: :horse_racing:

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