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

Spring Annotation Support #51

Closed
benkiefer opened this Issue Jan 4, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@benkiefer
Contributor

benkiefer commented Jan 4, 2015

I know there is an outstanding pull request (#16) out there for spring annotation support, but it's missing tests, and doesn't merge anymore. I'd like to know what you think about a do-over on this.

My current thinking is that it should be part of the jsondoc-springmvc module, and basically just map the spring annotations over to the core classes. That way, non-spring folks can keep using the core library without pulling spring along for the ride.

The Api annotations would supplement things that are missing from the spring annotations (descriptions, groupings, etc.). Here are some specific things that I know need to be addressed in order to call this done. Other folks can add to this as needed.

  • Support for ResponseEntity
  • Opt-in for documentation (My current theory is that we would reuse the @ApiMethod and @Api annotations, with some tweaks (like not requiring the verb on @ApiMethod).
  • Handling Spring defaults for the annotations (ex: verb defaults, content type defaults, produces defaults)

Thoughts/concerns?

@benkiefer

This comment has been minimized.

Show comment
Hide comment
@benkiefer

benkiefer Jan 4, 2015

Contributor

After poking at this a bit, I'm almost wondering if it might be easier to just have a separate set of method annotations for the spring stuff. That will allow the two method annotations to live independently of each other. They would still map to the same base pojo. Just throwing some thoughts out there for discussion.

Contributor

benkiefer commented Jan 4, 2015

After poking at this a bit, I'm almost wondering if it might be easier to just have a separate set of method annotations for the spring stuff. That will allow the two method annotations to live independently of each other. They would still map to the same base pojo. Just throwing some thoughts out there for discussion.

@benkiefer

This comment has been minimized.

Show comment
Hide comment
@benkiefer

benkiefer Jan 4, 2015

Contributor

I think I've got a good first pass at this ready to go. There are no changes to the underlying library (except what my IDE tried to do with imports, and some method scoping cleanup). If you have jsondoc-springmvc on your classpath, and the appropriate annotations, it just works. I'm going to be working with some more complicated apis on this tomorrow, but I'll go ahead and give you a pull request to look over.

Contributor

benkiefer commented Jan 4, 2015

I think I've got a good first pass at this ready to go. There are no changes to the underlying library (except what my IDE tried to do with imports, and some method scoping cleanup). If you have jsondoc-springmvc on your classpath, and the appropriate annotations, it just works. I'm going to be working with some more complicated apis on this tomorrow, but I'll go ahead and give you a pull request to look over.

@benkiefer

This comment has been minimized.

Show comment
Hide comment
@benkiefer

benkiefer Jan 4, 2015

Contributor

One thing to note, I didn't do anything with auth support. In part, because I don't need it. We're using oauth, and the existing implementation doesn't quite work for us. I've got markers for where it should go in the code.

Contributor

benkiefer commented Jan 4, 2015

One thing to note, I didn't do anything with auth support. In part, because I don't need it. We're using oauth, and the existing implementation doesn't quite work for us. I've got markers for where it should go in the code.

@benkiefer

This comment has been minimized.

Show comment
Hide comment
@benkiefer

benkiefer Jan 5, 2015

Contributor

Sorry, one last thing on this. I'd really like to see something like what I did with the ApiDocScanner work its way into the project. That way, folks fan add their own mappers to the documentation classes that are the core of this project. Thanks again for all your work.

Contributor

benkiefer commented Jan 5, 2015

Sorry, one last thing on this. I'd really like to see something like what I did with the ApiDocScanner work its way into the project. That way, folks fan add their own mappers to the documentation classes that are the core of this project. Thanks again for all your work.

@fabiomaffioletti

This comment has been minimized.

Show comment
Hide comment
@fabiomaffioletti

fabiomaffioletti Jan 5, 2015

Owner

Hey Ben,

thank you for your proposal. In these days I will have very few time to work on the project, so I'll be very slow in responding to issues and pull requests. The next things I wanted to work on in the project are (and this would be the roadmap I was thinking about):

  • consolidate the spring boot integration, using the jsondoc-springmvc module for both spring boot and non spring boot applications (still need to understand if it's feasible, suggestions are welcome)
  • use a single jsondoc-ui module for every kind of webapplications (spring boot, non spring boot, any other frameworks): I don't like having jsondoc-ui and jsondoc-ui-webjar, so also in this case proposals on how to do that are welcome
  • give support for spring mvc annotaions as you and other guys proposed
  • develop a new feature which will provide a richer documentation for api clients (I prefer not to disclose it, it'll be a surprise)
  • do improvements and bugfixes as usual

Anyway I had a quick look at the code you pushed and I have a few comments:

  • The ApiDocScanner interface is definitely the correct approach, I also was thinking about this approach to support spring mvc and other kind of implementations
  • Instead, I don't agree on having new annotations (SpringApiMethod, ...), because it will mean that every new implementation could have it's own annotations. I think the right thing to do is to use the core annotations instead. What about the mandatory annotation's attributes? When I developed them I thougth about the minimum set of information needed to have a meaningful documentation which could also be tested in the playground, that's why there are mandatory attributes. It is known that this is a problem when putting jsondoc annotations on controller methods, because you end up writing the same info twice, for example on the ApiMethod path and on the RequestMapping value. So we need a way to keep some info as "mandatory because meaningful and needed for the playground", while relaxing the checks when putting the jsondoc annotation on a method already annotated with the mandatory info (like the RequestMapping spring's one). So, the idea is to relax the checks at compile time, providing some defaults like you said (ApiVerb, produces, etc), but add new "errors" and "warnings" sections in the single PojoDoc class or globally in the JSONDoc class, filled at jsondoc documentation generation (runtime), clearly communicating that something is mandatory (and in this case the documentation is not meaningful - "path" attribute for example - in the "errors") and that something should be specified in the jsondoc annotation to provide better documentation ("description" attribute for example, in the "warnings"). This needs to be shown very clearly in the UI.
  • Speaking about the UI, it expects some of the json data to be always filled with some data (the mandatory annotations' attributes), used to populate the handlebars template in the right way. If the checks will be relaxed then also the UI must take into account that, and add additional checks on the presence of the not-anymore-mandatory data.

At the end, here are the steps I have in mind to make spring mvc (or other frameworks) and jsondoc cohexist:

  • do as you did with the interface approach
  • relax checks on jsondoc annotations which have a spring's pair, providing errors and warnings sections in the jsondoc pojo or object
  • when using only jsondoc-core, scanning classes for documentation will be done in two steps for each method annotated with ApiMethod: first step will use the default jsondoc scanner methods, which could produce an incomplete documentation. Second step is a consistency check step: the generated pojo is analyzed and eventually errors and warnings lists are filled
  • when using jsondoc-springmvc scanning classes for documentation will be done in three steps for each method annotated with ApiMethod: first step will use the default jsondoc scanner method which will an incomplete documentation (missing "path" for example). Second step will take the result of the first step and integrate the pojos with the data coming from the Spring annotations. The third step is a consistency check step: the generated pojo is analyzed and eventually errors and warnings lists are filled
  • In case the same info is specified both on jsondoc and spring annotations, then the spring's one wins
  • In the UI, show errors and warnings very clearly (I'll think a way to do that) and manage cases in which some of the "semantic mandatory" data is missing in handlebars templates

Some additional notes:

  • generally speaking, jsondoc must work on servlet 2.4+ environments, so everything should be compatible also with 2.4 servlets. Is spring-mvc 4.1.3.RELEASE compatible with it?
  • Travis CI build is failing on the pull request

Thank you

Owner

fabiomaffioletti commented Jan 5, 2015

Hey Ben,

thank you for your proposal. In these days I will have very few time to work on the project, so I'll be very slow in responding to issues and pull requests. The next things I wanted to work on in the project are (and this would be the roadmap I was thinking about):

  • consolidate the spring boot integration, using the jsondoc-springmvc module for both spring boot and non spring boot applications (still need to understand if it's feasible, suggestions are welcome)
  • use a single jsondoc-ui module for every kind of webapplications (spring boot, non spring boot, any other frameworks): I don't like having jsondoc-ui and jsondoc-ui-webjar, so also in this case proposals on how to do that are welcome
  • give support for spring mvc annotaions as you and other guys proposed
  • develop a new feature which will provide a richer documentation for api clients (I prefer not to disclose it, it'll be a surprise)
  • do improvements and bugfixes as usual

Anyway I had a quick look at the code you pushed and I have a few comments:

  • The ApiDocScanner interface is definitely the correct approach, I also was thinking about this approach to support spring mvc and other kind of implementations
  • Instead, I don't agree on having new annotations (SpringApiMethod, ...), because it will mean that every new implementation could have it's own annotations. I think the right thing to do is to use the core annotations instead. What about the mandatory annotation's attributes? When I developed them I thougth about the minimum set of information needed to have a meaningful documentation which could also be tested in the playground, that's why there are mandatory attributes. It is known that this is a problem when putting jsondoc annotations on controller methods, because you end up writing the same info twice, for example on the ApiMethod path and on the RequestMapping value. So we need a way to keep some info as "mandatory because meaningful and needed for the playground", while relaxing the checks when putting the jsondoc annotation on a method already annotated with the mandatory info (like the RequestMapping spring's one). So, the idea is to relax the checks at compile time, providing some defaults like you said (ApiVerb, produces, etc), but add new "errors" and "warnings" sections in the single PojoDoc class or globally in the JSONDoc class, filled at jsondoc documentation generation (runtime), clearly communicating that something is mandatory (and in this case the documentation is not meaningful - "path" attribute for example - in the "errors") and that something should be specified in the jsondoc annotation to provide better documentation ("description" attribute for example, in the "warnings"). This needs to be shown very clearly in the UI.
  • Speaking about the UI, it expects some of the json data to be always filled with some data (the mandatory annotations' attributes), used to populate the handlebars template in the right way. If the checks will be relaxed then also the UI must take into account that, and add additional checks on the presence of the not-anymore-mandatory data.

At the end, here are the steps I have in mind to make spring mvc (or other frameworks) and jsondoc cohexist:

  • do as you did with the interface approach
  • relax checks on jsondoc annotations which have a spring's pair, providing errors and warnings sections in the jsondoc pojo or object
  • when using only jsondoc-core, scanning classes for documentation will be done in two steps for each method annotated with ApiMethod: first step will use the default jsondoc scanner methods, which could produce an incomplete documentation. Second step is a consistency check step: the generated pojo is analyzed and eventually errors and warnings lists are filled
  • when using jsondoc-springmvc scanning classes for documentation will be done in three steps for each method annotated with ApiMethod: first step will use the default jsondoc scanner method which will an incomplete documentation (missing "path" for example). Second step will take the result of the first step and integrate the pojos with the data coming from the Spring annotations. The third step is a consistency check step: the generated pojo is analyzed and eventually errors and warnings lists are filled
  • In case the same info is specified both on jsondoc and spring annotations, then the spring's one wins
  • In the UI, show errors and warnings very clearly (I'll think a way to do that) and manage cases in which some of the "semantic mandatory" data is missing in handlebars templates

Some additional notes:

  • generally speaking, jsondoc must work on servlet 2.4+ environments, so everything should be compatible also with 2.4 servlets. Is spring-mvc 4.1.3.RELEASE compatible with it?
  • Travis CI build is failing on the pull request

Thank you

@fabiomaffioletti fabiomaffioletti added this to the 1.1.0 milestone Jan 10, 2015

@fabiomaffioletti fabiomaffioletti self-assigned this Jan 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment