civisanalytics / swagger-diff

Utility for comparing two Swagger specifications.
BSD 3-Clause "New" or "Revised" License
265 stars 32 forks source link

swagger-diff confused with two operations with same number of parameters #19

Closed davidvc closed 8 years ago

davidvc commented 8 years ago

I am getting this error after running swagger-diff against an unchanged resource

API specification for configurator service has changed in an incompatible way: - incompatible request     params
  - get /employers/{}
  - new required request param: key
  - missing request param: id (in: path, type: integer) 

I believe it's because this resource has two GET methods each with a single parameter:

@GET
@Path("{id: [1-9]\\d*}")
@Produces(MediaType.APPLICATION_JSON)
@ApiOperation(value = "If successful, returns an employee with the given id.",
              response = Employer.class)
public Employer getEmployerById(@PathParam("id") long id ){
    return employerRepository.findById(id);
}

@GET
@Path("{key: \\D[\\w]+}")
@Produces(MediaType.APPLICATION_JSON)
@ApiOperation(value = "If successful, returns an employee with the given key.",
              response = Employer.class)
public Employer getEmployerByKey(@PathParam("key") String key ){
    return employerRepository.findByKey(key);
}

The swagger JSON looks like this:

"/employers/{key}": {
  "get": {
    "responses": {
      "200": {
        "schema": {
          "$ref": "#/definitions/Employer"
        },
        "description": "successful operation"
      }
    },
    "parameters": [
      {
        "pattern": "\\D[\\w]+",
        "type": "string",
        "required": true,
        "in": "path",
        "name": "key"
      }
    ],
    "produces": [
      "application/json"
    ],
    "operationId": "getEmployerByKey",
    "description": "",
    "summary": "If successful, returns an employee with the given key.",
    "tags": [
      "employers"
    ]
  }
},
"/employers/{id}": {
  "get": {
    "responses": {
      "200": {
        "schema": {
          "$ref": "#/definitions/Employer"
        },
        "description": "successful operation"
      }
    },
    "parameters": [
      {
        "format": "int64",
        "pattern": "[1-9]\\d*",
        "type": "integer",
        "required": true,
        "in": "path",
        "name": "id"
      }
    ],
    "produces": [
      "application/json"
    ],
    "operationId": "getEmployerById",
    "description": "",
    "summary": "If successful, returns an employee with the given id.",
    "tags": [
      "employers"
    ]
  }
},
jeffreyc commented 8 years ago

While this validates using Swagger's Validator Badge, I don't think this is actually a valid specification. Check out the discussion on https://github.com/apigee-127/sway/issues/32. The people commenting that the paths are the same/invalid are all affiliated with Swagger, and the discussion led to an issue to document that this is unsupported.

davidvc commented 8 years ago

Ooh, I didn't know about Validator Badge.

Thanks for the thread, it does make sense. Looks like the workaround is to merge the two methods into one with two optional parameters, and we'll use the id if it's present otherwise we'll use the key.

On Tue, Dec 1, 2015 at 12:19 PM jeffreyc notifications@github.com wrote:

While this validates using Swagger's Validator Badge https://github.com/swagger-api/validator-badge, I don't think this is actually a valid specification. Check out the discussion on apigee-127/sway#32 https://github.com/apigee-127/sway/issues/32. The people commenting that the paths are the same/invalid are all affiliated with Swagger, and the discussion led to an issue to document that this is unsupported.

— Reply to this email directly or view it on GitHub https://github.com/civisanalytics/swagger-diff/issues/19#issuecomment-161068015 .

davidvc commented 8 years ago

Closing the issue, it appears to be invalid from Swagger's perspective, even though swagger-core is emitting it without error.