swagger-node-runner icon indicating copy to clipboard operation
swagger-node-runner copied to clipboard

Array parameters with default values

Open gforge opened this issue 9 years ago • 2 comments

There seems to be an issue with default parameters when used in combination with arrays. I've added the following to the test-case swagger:

  /hello_w_default_array:
    x-swagger-router-controller: hello_world
    get:
      description: Returns 'Hello' to the caller
      operationId: hello_array
      parameters:
        - name: names
          in: query
          description: The names of the people to whom to say hello
          default: ["Tom", "Maria"]
          type: array
          items:
            type: string
          collectionFormat: csv

The test code in /tests/lib/common.js:

    it('should execute with default names', function(done) {
      request(this.app)
        .get('/hello_w_default_array')
        .set('Accept', 'application/json')
        .expect(200)
        .expect('Content-Type', /json/)
        .end(function(err, res) {
          should.not.exist(err);
          res.body.should.eql('Hello, Tom and Maria');
          done();
        });
    });

and the added function to the hello_world.js is:

...
  hello_array: hello_array,
...

function hello_array(req, res) {
  var hello = 'Hello';
  console.log(req.swagger)
  var names = req.swagger.params.names.value
  for (let n =0; n < names.length; n ++) {
    if (names.hasOwnProperty(n)) {
      if ((n + 1) === names.length){
        hello += " and "
      }else {
        hello += ", "
      }
      hello += names[n]
    }
  }
  res.json(hello);
}

And mocha test/lib/connect_middleware.js -g "with default names" fails with:

  1) connect_middleware standard controllers should execute with default names:
     Uncaught AssertionError: expected Error { message: 'expected 200 "OK", got 400 "Bad Request"' } to not exist
      at Test.<anonymous> (test/lib/common.js:72:22)
      at Test.assert (node_modules/supertest/lib/test.js:156:6)
      at Server.assert (node_modules/supertest/lib/test.js:127:12)
      at emitCloseNT (net.js:1521:8)

If I add a required: false the hello_array is properly identified but the names property is empty as if the default parameters are ignored. This could possibly be a upstream bug in sway as there seems to be an issue with how default arrays are passed. I think the package should have tests for default values so I've added them (see pull request) and therefore started reporting the issue here.

gforge avatar Jul 03 '16 10:07 gforge

Thanks, @gforge! I appreciate the detailed report and test case!

theganyo avatar Jul 06 '16 22:07 theganyo

I found an additional related annoyance, the swagger editor used to set parameters to hello_world?param1=&param2=value. I'm not sure if the param1= should be an acceptable query spec but it behaves different depending on the variable type and this probably not intentional. When param1 is a string it will pass an empty string to the function while if it is defined as an integer there will be a Bad Request issued. Here's the yaml section illustrating the issue:

    get:
      description: Returns 'Hello' to the caller
      operationId: hello
      parameters:
        - name: name
          in: query
          description: The name of the person to whom to say hello
          default: "no stranger"
          type: string
        - name: id
          in: query
          description: The ID of a person
          default: 0
          type: integer

The tests to cause the issue to trigger are:

    it('if a parameter is specified as param= then is should go to default for a string', function(done) {
      request(this.app)
        .get('/hello_w_default?name=&id=1')
        .set('Accept', 'application/json')
        .expect(200)
        .expect('Content-Type', /json/)
        .end(function(err, res) {
          should.not.exist(err);
          res.body.should.eql('Hello, no stranger!');
          done();
        });
    });

    it('if a parameter is specified as param= then is should go to default for an integer', function(done) {
      request(this.app)
        .get('/hello_w_default?name=Mary&id=')
        .set('Accept', 'application/json')
        .expect(200)
        .expect('Content-Type', /json/)
        .end(function(err, res) {
          should.not.exist(err);
          res.body.should.eql('Hello, Mary!');
          done();
        });
    });

And the errors that these cause are:

 1) express_middleware standard controllers if a parameter is specified as param= then is should go to default for a string:

      Uncaught AssertionError: expected 'Hello, stranger!' to equal 'Hello, no stranger!'
      + expected - actual

      -Hello, stranger!
      +Hello, no stranger!

      at Assertion.fail (node_modules/should/lib/assertion.js:180:17)
      at Assertion.prop.value (node_modules/should/lib/assertion.js:65:17)
      at Test.<anonymous> (test/lib/common.js:60:27)
      at Test.assert (node_modules/supertest/lib/test.js:156:6)
      at Server.assert (node_modules/supertest/lib/test.js:127:12)
      at emitCloseNT (net.js:1524:8)

  2) express_middleware standard controllers if a parameter is specified as param= then is should go to default for a string:
     Uncaught AssertionError: expected Error { message: 'expected 200 "OK", got 400 "Bad Request"' } to not exist
      at Test.<anonymous> (test/lib/common.js:72:22)
      at Test.assert (node_modules/supertest/lib/test.js:156:6)
      at Server.assert (node_modules/supertest/lib/test.js:127:12)
      at emitCloseNT (net.js:1524:8)

Again this could be an issue residing outside the swagger-node-runner, after some node-inspector debugging I think the problem is that when sway does its ParameterValue the default value isn't available at the validation since the value isn't found to be undefined. Not sure how to best solve this as ignoring empty query variables will inhibit the option of passing an empty string. Interfering at the params parser feels generally like a bad idea.

gforge avatar Jul 12 '16 20:07 gforge