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

Allow insert custom header value from playground form #148

Closed
strokine opened this Issue Sep 23, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@strokine

strokine commented Sep 23, 2015

If I understood this correctly, Spring JSONDoc just ignores annotation @ApiHeaders
For example, I've set such annotation for one of my controller's method:

@ApiHeaders(headers={@ApiHeader(name="Authorization", description="Authorization token")})

And I see nothing related to this anywhere.
I was expecting to see a text field in the playground form which would allow me to input the value of the HTTP Header with name Authorization.
Do I miss something? Should it work this way? If not, could you add this to the form?
Thanks for the grate tool to generate documentation for Spring MVC!

@davidkuster

This comment has been minimized.

Show comment
Hide comment
@davidkuster

davidkuster Sep 23, 2015

Good catch. From the sample at http://jsondoc.eu01.aws.af.cm/jsondoc-ui.html It looks like it's picking it up if it's on @RequestMapping as in CountryController. But standalone @ApiHeaders is being ignored by the UI, although I am seeing it in the generated JSON.

davidkuster commented Sep 23, 2015

Good catch. From the sample at http://jsondoc.eu01.aws.af.cm/jsondoc-ui.html It looks like it's picking it up if it's on @RequestMapping as in CountryController. But standalone @ApiHeaders is being ignored by the UI, although I am seeing it in the generated JSON.

@strokine

This comment has been minimized.

Show comment
Hide comment
@strokine

strokine Sep 23, 2015

Thanks for reply, it also helped my to work around until the issue is fixed. I've just added headers attribute to the @RequestMapping on the class level, and the UI shows me the field for each endpoint of the class.

@RequestMapping(value="/user", produces=MediaType.APPLICATION_JSON_VALUE, headers=HttpHeaders.AUTHORIZATION)

strokine commented Sep 23, 2015

Thanks for reply, it also helped my to work around until the issue is fixed. I've just added headers attribute to the @RequestMapping on the class level, and the UI shows me the field for each endpoint of the class.

@RequestMapping(value="/user", produces=MediaType.APPLICATION_JSON_VALUE, headers=HttpHeaders.AUTHORIZATION)
@davidkuster

This comment has been minimized.

Show comment
Hide comment
@davidkuster

davidkuster Sep 23, 2015

Happy to help. Ironically this helped me as well. I'm using this with Grails 3, which JSONDoc doesn't integrate with real well at the moment. So having to revert to standalone "old school servlet style" annotations. @ApiHeaders is working ok in standalone mode.

davidkuster commented Sep 23, 2015

Happy to help. Ironically this helped me as well. I'm using this with Grails 3, which JSONDoc doesn't integrate with real well at the moment. So having to revert to standalone "old school servlet style" annotations. @ApiHeaders is working ok in standalone mode.

@fabiomaffioletti

This comment has been minimized.

Show comment
Hide comment
@fabiomaffioletti

fabiomaffioletti Sep 24, 2015

Owner

Thank you guys for this discussion. The JSONDoc scanner for Spring MVC projects now extracts most of the documentation data from Spring's annotations. This happens for headers as well. Some of the properties derived from JSONDoc annotations, if present, are then merged to build a richer documentations, but currently headers are excluded from this operation: you can see the code handling this here: https://github.com/fabiomaffioletti/jsondoc/blob/master/jsondoc-springmvc/src/main/java/org/jsondoc/springmvc/scanner/AbstractSpringJSONDocScanner.java#L226

I am aware that this can be a problem in case you need an authentication header like @davidkuster does. I can see two options:

  1. integrate Spring's extracted documentation for headers with JSONDoc one
  2. provide a new ApiAuthType method that gets a token string

I'll think about it, but I would like to hear your opinion.

Owner

fabiomaffioletti commented Sep 24, 2015

Thank you guys for this discussion. The JSONDoc scanner for Spring MVC projects now extracts most of the documentation data from Spring's annotations. This happens for headers as well. Some of the properties derived from JSONDoc annotations, if present, are then merged to build a richer documentations, but currently headers are excluded from this operation: you can see the code handling this here: https://github.com/fabiomaffioletti/jsondoc/blob/master/jsondoc-springmvc/src/main/java/org/jsondoc/springmvc/scanner/AbstractSpringJSONDocScanner.java#L226

I am aware that this can be a problem in case you need an authentication header like @davidkuster does. I can see two options:

  1. integrate Spring's extracted documentation for headers with JSONDoc one
  2. provide a new ApiAuthType method that gets a token string

I'll think about it, but I would like to hear your opinion.

@davidkuster

This comment has been minimized.

Show comment
Hide comment
@davidkuster

davidkuster Sep 25, 2015

@fabiomaffioletti first off, thanks for the great work on this.

Looking at the line of code you called out above, I see headers in the list of what's being copied so not sure how it's excluded. Regardless, would like to get more into the JSONDoc code as time permits but for this discussion I'm less concerned about the internals.

As for your question, I am using the header for authentication, via JWT. I'd think that option 1 above sounds logical, and probably worth doing regardless of other choices. Option 2 I'm a little unclear on - ApiAuth_Type_? Looking at the doc page, presumably this would be under the Authentication heading. Maybe call it ApiAuthToken? Is that too specific? I would think adding an additional annotation to specify authentication via a token, whatever it's called, beyond the @ApiAuthNone and @ApiAuthBasic options you have now would be a great addition.

davidkuster commented Sep 25, 2015

@fabiomaffioletti first off, thanks for the great work on this.

Looking at the line of code you called out above, I see headers in the list of what's being copied so not sure how it's excluded. Regardless, would like to get more into the JSONDoc code as time permits but for this discussion I'm less concerned about the internals.

As for your question, I am using the header for authentication, via JWT. I'd think that option 1 above sounds logical, and probably worth doing regardless of other choices. Option 2 I'm a little unclear on - ApiAuth_Type_? Looking at the doc page, presumably this would be under the Authentication heading. Maybe call it ApiAuthToken? Is that too specific? I would think adding an additional annotation to specify authentication via a token, whatever it's called, beyond the @ApiAuthNone and @ApiAuthBasic options you have now would be a great addition.

@strokine

This comment has been minimized.

Show comment
Hide comment
@strokine

strokine Sep 25, 2015

Regarding Spring integration, I wish I wouldn't add headers attribute to the @RequestMapping annotation.

@RequestMapping(value="/user", produces=MediaType.APPLICATION_JSON_VALUE, headers=HttpHeaders.AUTHORIZATION)

Before I did, my endpoints were getting the requests without Authorization header, and send back 401 Unauthorized code. But after I did add the attribute, the requests without Authorization header wouldn't even get there, and Spring MVC returns 404 Not Found code.
So, I'd prefer to skip headers=HttpHeaders.AUTHORIZATION attribute, but define the header for JSONDoc anyway anyhow.

Choosing between option 1 or 2, both of them looks good to me. I'd say, for now, whatever is easier to implement.

strokine commented Sep 25, 2015

Regarding Spring integration, I wish I wouldn't add headers attribute to the @RequestMapping annotation.

@RequestMapping(value="/user", produces=MediaType.APPLICATION_JSON_VALUE, headers=HttpHeaders.AUTHORIZATION)

Before I did, my endpoints were getting the requests without Authorization header, and send back 401 Unauthorized code. But after I did add the attribute, the requests without Authorization header wouldn't even get there, and Spring MVC returns 404 Not Found code.
So, I'd prefer to skip headers=HttpHeaders.AUTHORIZATION attribute, but define the header for JSONDoc anyway anyhow.

Choosing between option 1 or 2, both of them looks good to me. I'd say, for now, whatever is easier to implement.

@fabiomaffioletti

This comment has been minimized.

Show comment
Hide comment
@fabiomaffioletti

fabiomaffioletti Oct 1, 2015

Owner

Hey guys, I just committed a new branch named ISSUE-148, containing an implementation of the ApiAuthToken previously discussed. Please review this: https://github.com/fabiomaffioletti/jsondoc/compare/ISSUE-148?expand=1 and if this is working for you let me know and I'll merge to master and release.

Owner

fabiomaffioletti commented Oct 1, 2015

Hey guys, I just committed a new branch named ISSUE-148, containing an implementation of the ApiAuthToken previously discussed. Please review this: https://github.com/fabiomaffioletti/jsondoc/compare/ISSUE-148?expand=1 and if this is working for you let me know and I'll merge to master and release.

@strokine

This comment has been minimized.

Show comment
Hide comment
@strokine

strokine Oct 1, 2015

Fabio, it looks good. I just have a minor comment: I know it is not orthodox to have a token without scheme specified, but many systems just take a row token.
So, instead of

Berear xyz123

they expect just

xyz123

So, if I'm one of them, and set Scheme to empty string, I'd get a space before my test token here:
https://github.com/fabiomaffioletti/jsondoc/compare/ISSUE-148?expand=1#diff-b541af55f854aade3b11faa8894a4e0bR1045
and the space could cause a problem.

So, I'd suggest to check, if scheme is not set or an empty string, don't put a space before the token.
Other than that it looks very good to me.
Thanks a lot!!!

strokine commented Oct 1, 2015

Fabio, it looks good. I just have a minor comment: I know it is not orthodox to have a token without scheme specified, but many systems just take a row token.
So, instead of

Berear xyz123

they expect just

xyz123

So, if I'm one of them, and set Scheme to empty string, I'd get a space before my test token here:
https://github.com/fabiomaffioletti/jsondoc/compare/ISSUE-148?expand=1#diff-b541af55f854aade3b11faa8894a4e0bR1045
and the space could cause a problem.

So, I'd suggest to check, if scheme is not set or an empty string, don't put a space before the token.
Other than that it looks very good to me.
Thanks a lot!!!

@fabiomaffioletti

This comment has been minimized.

Show comment
Hide comment
@fabiomaffioletti

fabiomaffioletti Oct 1, 2015

Owner

Sounds fair. I made the pull request with your suggestion. Please review it and let me know: #152

Owner

fabiomaffioletti commented Oct 1, 2015

Sounds fair. I made the pull request with your suggestion. Please review it and let me know: #152

@strokine

This comment has been minimized.

Show comment
Hide comment
@strokine

strokine Oct 1, 2015

Looks good to me.
Thank you again! You've built a very nice tool.
Eugene

strokine commented Oct 1, 2015

Looks good to me.
Thank you again! You've built a very nice tool.
Eugene

@fabiomaffioletti fabiomaffioletti modified the milestone: 1.2.7 Oct 2, 2015

@fabiomaffioletti

This comment has been minimized.

Show comment
Hide comment
@fabiomaffioletti

fabiomaffioletti Oct 2, 2015

Owner

Released. It'll take a few hours to be synched with maven central. Thank you for helping improving JSONDoc.

Owner

fabiomaffioletti commented Oct 2, 2015

Released. It'll take a few hours to be synched with maven central. Thank you for helping improving JSONDoc.

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