Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False negatives when removing deprecated properties from models #97

Closed
pkunze opened this issue Feb 1, 2023 · 4 comments
Closed

False negatives when removing deprecated properties from models #97

pkunze opened this issue Feb 1, 2023 · 4 comments
Labels

Comments

@pkunze
Copy link
Contributor

pkunze commented Feb 1, 2023

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
Copy link
Contributor Author

pkunze commented Mar 2, 2023

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
Copy link
Member

galovics commented Mar 6, 2023

@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
Copy link
Contributor Author

pkunze commented Mar 9, 2023

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

pkunze added a commit to pkunze/swagger-brake that referenced this issue Mar 9, 2023
galovics pushed a commit to pkunze/swagger-brake that referenced this issue Sep 30, 2023
galovics added a commit to pkunze/swagger-brake that referenced this issue Sep 30, 2023
galovics pushed a commit that referenced this issue Sep 30, 2023
@galovics
Copy link
Member

Fixed in #98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants