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

Report causes for compatibility breakage #105

Merged
merged 9 commits into from
Jan 8, 2020

Conversation

thiagoarrais
Copy link
Contributor

First of all, this is a work in progress. I'm trying to add details about broken compatibility and tried to start with properties removed from responses. Just let me know if I'm going in the right direction.

I'm currently getting the output below with this code (old and new specs available as a gist):

==========================================================================
==                            API CHANGE LOG                            ==
==========================================================================
                                  My API                                  
--------------------------------------------------------------------------
--                            What's Changed                            --
--------------------------------------------------------------------------
- GET    /api/v1/addons
  Return Type:
    - Changed 422 Unprocessable Entity
      Media types:
        - Changed application/json
          Schema: Broken compatibility
          Missing property: .originConstraint.leafBean
--------------------------------------------------------------------------
--                                Result                                --
--------------------------------------------------------------------------
                 API changes broke backward compatibility                 
--------------------------------------------------------------------------

I have added the Missing property line. The property path is a little rough right now, but I plan on working on this later. For this test case, my ideal output should be something along these lines:

Missing property: [Message.originConstraint.leafBean]

Do you see this getting merged if it we're able to (1) get to an acceptable path format and (2) include the detailed reported in the HTML and markdown outputs?

Related to #18

@thiagoarrais thiagoarrais force-pushed the report-missing-property branch from d3d88d1 to 0fdaa1d Compare October 24, 2019 18:09
@thiagoarrais
Copy link
Contributor Author

@quen2404: couldn't pin point where Renderer tests should go. Should I write some? Where?

@thiagoarrais
Copy link
Contributor Author

HTML reporting is done and Markdown was covered already (which was a pleasant surprise). I'm considering this funcionally done. But I'll still try some refactoring by spliting the incompatibility listing and printing responsability. I believe the listing should go inside the ChangedXXXX classes and XxxxRender should only print it. Do you see any problems with that?

@michalchmura
Copy link

Any updates on this?

@thiagoarrais
Copy link
Contributor Author

I'm still willing to work on it. It needs some refactoring in my opinion. But I would like to confirm interest in this funcionality-wise before going on. Does the current proposed output seem good to you, @michalchmura?

@michalchmura
Copy link

@thiagoarrais yes, the proposed output seems great! It'd be good to be able to list addition/deletion but also altering the property names.

I've built the project from your branch and I couldn't quite get it to output it.

@thiagoarrais
Copy link
Contributor Author

This is what I'm getting. You can try it using these input files: remove_inner_prop_1.yaml and remove_inner_prop_2.yaml:

$ java -jar target/openapi-diff-2.0.0-SNAPSHOT-jar-with-dependencies.jar src/test/resources/remove_inner_prop_1.yaml src/test/resources/remove_inner_prop_2.yaml
==========================================================================
==                            API CHANGE LOG                            ==
==========================================================================
                             Swagger Petstore                             
--------------------------------------------------------------------------
--                            What's Changed                            --
--------------------------------------------------------------------------
- GET    /pet/findByStatus
  Return Type:
    - Changed 200 OK
      Media types:
        - Changed application/json
          Schema: Broken compatibility
          Missing property: pets[n].bark
          Changed property type: pets[n].aaaa
          Changed property type: pets[n].tttt
          Missing property: pets[n].puppies[n].name
          Missing property: pets[n].puppies[n].nick
--------------------------------------------------------------------------
--                                Result                                --
--------------------------------------------------------------------------
                 API changes broke backward compatibility                 
--------------------------------------------------------------------------

@thiagoarrais
Copy link
Contributor Author

I'm currently focusing on getting removed properties working. Can you get me some before/after inputs and what you'd like too see outputed for your use case? I may be able to fit it into this PR.

Code inspired on MarkdownRender's implementation
@thiagoarrais
Copy link
Contributor Author

thiagoarrais commented Dec 19, 2019

I'm happy with the state of the code as of 856b426. This is ready for review and subsequent merging. It admittedly doesn't cover all cases for compatibility breakage (adding required parameter, for example), but it already covers the use cases I need for now and adds an extensible structure so that it can be improved in future PRs.

@thiagoarrais thiagoarrais changed the title WIP: Report causes for compatibility breakage Report causes for compatibility breakage Dec 19, 2019
@quen2404
Copy link
Member

quen2404 commented Jan 8, 2020

Hi @thiagoarrais,
thank you for your work. I'm currently looking what you've done :)

@quen2404 quen2404 merged commit 9dc158e into OpenAPITools:master Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants