redskap / swagger-brake

Swagger contract checker for breaking API changes
Apache License 2.0
59 stars 16 forks source link

Prevent removal of deprecated request/response attribute from creating a breaking change #98

Closed pkunze closed 1 year ago

pkunze commented 1 year ago

I tried to implement it according to the discussion in #97 Please have a critical look at it as this (Java) is foreign territory to me. 🔥

pkunze commented 1 year ago

@galovics if you already had time to have a look at it let me know if there are things left to do. ;)

galovics commented 1 year ago

Plus I just approved the run of the integration tests and it seems there are failures, please take a look.

pkunze commented 1 year ago

Alright. I too noticed failing tests, however, those seemed to be broken even without my changes. However, I will take a look no matter what. :) Thank you for takin the time!

galovics commented 1 year ago

@pkunze strange. I'll check it soon myself but let me know if you're stuck somewhere.

pkunze commented 1 year ago

@galovics i managed to fix those in the meantime. now im left with some remainig (correct) errors that im currently working on. Those are actually errors i caused 🤭 No need for you to step in (so far).

pkunze commented 1 year ago

So...i am currently trying to make it work with cases where deep attributes are removed. Ex. category.name.

Now here it gets a bit tricky...

Image a response/request attribute contains an attribute of type Category, which is removed in a newer api-version. Category its self has 2 more attributes (id / name). Currently, this results in 3 breaking changes beeing reported, one for the whole category attribute and 2 more for its properties (name & id) which is a flaw IMO. Because the schema of Category didn't change, is is just not referenced anymore.

The cases where breaking changes are reported are compatible, its just the amount of (IMO false positives) that would be reduced in any of these cases, because of the reasons stated above.

So the question is, would it be ok for you, that instead i would just report the removal of the category attribute from the request/response object, but not the nestes/deep attributes? Because those are still there, they just can not be found since the containing object is removed.

galovics commented 1 year ago

@pkunze I think the behavior is correct i.e. to report 3 attributes being removed. Indeed the schema for the Category object did not change but remember, swagger-brake doesn't report breaking/non-breaking changes on the schemas but on the APIs.

Imagine you had this API:

POST /category
{
    "category": {
        "id": 1,
        "name": "something"
    }
}

And then you change your API definition to accept only this:

POST /category
{
    "id": 1,
    "name": "something"
}

In this case, your API has been changed, not the schema object you used to represent the category. The category attribute has been removed along with category.id and category.name. But this relates to the API, not the schema as I said.

galovics commented 1 year ago

@pkunze please rebase your branch from master. I've made a commit to fix the build errors.