search

redskap / swagger-brake

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

False negatives when removing deprecated properties from models #97

Closed pkunze closed 9 months ago

pkunze commented 1 year ago

We are facing false positives when we remove properties that are already marked as deprecated from models. I could provide you both swagger json files via private email since i am not allowed to share them publicly.

The Diff and Report looks like this (the model is reused in various endpoints): image image

pkunze commented 1 year ago

If there is anything i could do to solve this issue let me know. I would even try and solve this myself, however, i'd rather discuss the possible solution with you first.

galovics commented 1 year ago

@pkunze thanks for reporting. I think the only thing we're missing from the code is a check to see whether the attribute being removed is marked as deprecated or not. And if it is, just simply skip that attribute. The code should be here: io.redskap.swagger.brake.core.rule.request.RequestTypeAttributeRemovedRule and respectively there's one for response attributes too: io.redskap.swagger.brake.core.rule.response.ResponseTypeAttributeRemovedRule

The logic should be similar to what we have for APIs as well in: io.redskap.swagger.brake.core.rule.path.PathDeletedRule


            if (!newApi.getPath(p).isPresent()) {
                if (!p.isDeprecated() || !checkerOptions.isDeprecatedApiDeletionAllowed()) {
                    log.debug("Path {} is not included in the new API", p);
                    breakingChanges.add(new PathDeletedBreakingChange(p.getPath(), p.getMethod()));
                } else {
                    log.debug("Path {} is not included in the new API however it was marked as deprecated", p);
                }
            } else {
                log.debug("Path {} is present in the new API as well", p);
            }
pkunze commented 1 year ago

Allright, I'll ask a friend of mine to give me a Java Development Jump-Start and give it a shot. :)

galovics commented 9 months ago

Fixed in https://github.com/redskap/swagger-brake/pull/98