Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Transaction Names from Swagger are not unique. #558

Closed
netmilk opened this issue Jul 11, 2016 · 3 comments · Fixed by #616
Closed

Transaction Names from Swagger are not unique. #558

netmilk opened this issue Jul 11, 2016 · 3 comments · Fixed by #616

Comments

@netmilk
Copy link
Contributor

netmilk commented Jul 11, 2016

Skipped Transaction Names generated from oneSwagger/OAI operation with different status codes and content-types are not unique.

swagger: "2.0"
info:
  version: 1.0.0
  title: Life Saving API
schemes:
  - http
consumes:
  - application/json
produces:
  - application/json
paths:
  /112:
    get:
      responses:
        200:
          description: "Successful response"
        400:
          description: "Not so successful response"

dredd swagger.yml http://localhsot --names Results in following:

...
info: /112 > GET
skip: GET /112
...

Please notice the second line. It's not even a transaction.name but transaction.id (reported here #559)

As a workaround, you can use the following hooks code. The transaction names will have appended status code and content-type to be unique within a Swagger/OAI operation.

var hooks = require('hooks');

// This is patching https://github.com/apiaryio/dredd/issues/558
hooks.beforeAll(function(transactions, callback){
  // Case insensitive object lookup
  var getValueCaseless = function(object, key){
    var objectKey = undefined;
    var objectKeys = Object.keys(object);

    objectKeys.forEach(function(currentObjectKey){
      if(currentObjectKey.toLowerCase() == key.toLowerCase()){
        objectKey = currentObjectKey;
      };
    });

    value = object[objectKey];
    return(value);
  };

  // Append statusCodes and content-types to the transaction names and ids
  transactions.forEach(function(transaction, index){
    statusCode = transaction['expected']['statusCode'];
    contentType = getValueCaseless(transaction['expected']['headers'], 'content-type')
    suffix = ' > ' + statusCode + ' ' + contentType;
    // do not patch it twice
    if(transaction['name'].indexOf(suffix) == -1){
      transaction['name'] += suffix;
      transaction['id'] = transaction['name'];
    }
  });

  callback()
});


// Unskip the transaction
hooks.before('/112 > GET > 400 application/json', function(transaction){
  transaction.skip = false
})
@honzajavorek
Copy link
Contributor

honzajavorek commented Aug 24, 2016

@netmilk Is the workaround also a proposal for a fix? In other words, if Dredd gets fixed, should it generate following transaction names?

/112 > GET > 200 application/json
/112 > GET > 400 application/json

The application/json isn't needed at the moment, since Dredd supports only application/json as of now, but it's true that it could be needed in the future. Also we need to verify the proposal in #227 counts with Swagger.

Another question: Do we want to consider this to be a bug fix or a breaking change? Because it is breaking change and can break people's existing hooks.

@honzajavorek
Copy link
Contributor

Discussed several things with @netmilk in person and the outcome is:

  • From external (user's) point of view, this is a bug.
  • From internal (dredd's codebase) point of view, this is a new feature and a bit of something what's proposed in Canonical transaction paths  #227
  • From any point of view, this will be a breaking change and thus will need to bump Dredd's major version number

The final form of the names discussed is:

/112 > GET > 200 > application/json
/112 > GET > 400 > application/json

@honzajavorek
Copy link
Contributor

Reminder for myself: Closing this will also need documentation update of all Swagger examples.

honzajavorek added a commit to apiaryio/dredd-transactions that referenced this issue Aug 25, 2016
honzajavorek added a commit that referenced this issue Sep 8, 2016
BREAKING CHANGE: Users of Dredd hooks will have to change how they address
transactions - e.g. '/112 > GET' will have to be changed to '/112 > GET > 200 > application/json'

See apiaryio/dredd-transactions#63 for details. Closes #558.
honzajavorek added a commit that referenced this issue Sep 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants