search

RobWin / assertj-swagger

A Swagger assertj test library which compares a contract-first Swagger YAML/JSON file with a code-first Swagger JSON
Apache License 2.0
171 stars 44 forks source link

Base properties not supported? #1

Closed g00glen00b closed 8 years ago

g00glen00b commented 9 years ago

In Swagger v2 you can specify some base parameters like the path, consumes, produces, ... . For example:

host: localhost
schemes:
  - http
basePath: /api
consumes:
  - application/json
produces:
  - application/json

I'm using these in my Swagger YAML definition, however, it seems that neither of these parameters is supported, because the basePath is being ignored and since I didn't define the consumes or produces parameter in the paths themself (because they're already defined as a base parameter), my test fails because it expects null.

RobWin commented 9 years ago

Hi, indeed it is not supported yet. assertj-swagger compares Swagger objects like Paths, Parameters and Definitions. It does not compare Swagger objects like info, descriptions or summaries yet. A PR is welcome. I dont have much time because I'm on parental leave.

RobWin commented 9 years ago

If you like the idea of asserj-swagger, I would love to get your GitHub Star.

marcelstoer commented 6 years ago

Why was this closed? You can't seriously assert the validity of paths and ignore basePath.

It does not compare Swagger objects like info, descriptions or summaries

That's fine and I don't think that's expected here. Except for title and version the fields of the info object are optional anyhow. None of them have a functional impact on how the API behaves - very much in contrast to basePath.

So, if I have an OpenAPI definition like so

...
basePath: /broker-monitor/v1
paths:
  /failure-messages:
...

and a Spring controller like so

...
@RequestMapping(value = "/broker-monitor/v1", produces = MediaType.APPLICATION_JSON_VALUE)
public interface BrokerMonitorApi {
    @GetMapping("/failure-messages")
...

Then you would expect that assertj-swagger considers this to be valid. Currently, however, I get something like this

1) [Checking Paths] 
Expecting:
 <[...
    "/broker-monitor/v1/failure-messages",
    ...]>
to contain:
 <["/failure-messages"]>
but could not find:
 <["/failure-messages"]>
RobWin commented 6 years ago

Hi, I can't remember why it was closed :( I don't use assertj-swagger anymore, but I still merge PRs and release new versions. If it's still not working, please create a PR and I merge it.

g00glen00b commented 6 years ago

To me, it didn't matter if the issue was closed. It was more a question to make sure I wasn't forgetting something on my end, rather than a feature request. We got around it by changing our Swagger spec to include the basepath in every endpoint definition. It's not the most clean solution, but it works.

Nonetheless, thanks for the great work!

marcelstoer commented 6 years ago

I don't use assertj-swagger anymore, but I still merge PRs and release new versions.

Yeah yeah, no worries...I know I've got to fix this myself.

This one really hit me by surprise and at first I doubted it was a bug at all. To me it looks like assertj-swagger is an established library as I don't know any other JVM- or Node.js-based lib that offers similar functionality. Yet, the basePath feature seems so fundamental to Swagger/OpenAPI that I first thought I was doing something wrong when I found this bug.

We got around it by changing our Swagger spec to include the basepath in every endpoint definition.

True, that's the obvious workaround but I prefer fixing the tools rather than compromising on my code.

marcelstoer commented 6 years ago

I've been thinking a bit more about this and I believe it boils down to 1 question.

Is this (my design-first contract)

basePath: /broker-monitor/v1
paths:
  /failure-messages:

equal to this (reverse engineered from impl-first code)

basePath: /
paths:
  /broker-monitor/v1/failure-messages:

or not. Academically it may not be equal but for all practical purpose it is. See also https://github.com/RobWin/assertj-swagger#comparing-expected-and-actual-paths-in-schemas about the config param assertj.swagger.pathsPrependExpected.

In order to not brake backwards compatibility I'll prepare a PR that adds basePath to the expected paths only if assertj.swagger.pathsPrependExpected is not set. This will make setting assertj.swagger.pathsPrependExpected obsolete if its value would be equal to basePath.