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

added security definition description #1174

Merged
merged 11 commits into from Apr 19, 2022
Merged

added security definition description #1174

merged 11 commits into from Apr 19, 2022

Conversation

pmorelli92
Copy link
Contributor

@pmorelli92 pmorelli92 commented Apr 12, 2022

Describe the PR

Added description capabilities for the security definitions types.

Additional context

I know this field is not mandatory, but another tool I am working with, relies on me adding the descriptions, otherwise it will break. I would need pretty please a tag if this gets merged 🙏

@ubogdan
Copy link
Contributor

ubogdan commented Apr 13, 2022

@pmorelli92 I perfectly understand the use case, please open a feature request https://github.com/swaggo/swag/issues examining this implementation requirement and I will follow you with a CR.

@pmorelli92
Copy link
Contributor Author

Done @ubogdan #1175

Copy link
Contributor

@ubogdan ubogdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done.
We need to have a clear readme so our users will understand how to use it.

parser.go Show resolved Hide resolved
README.md Outdated
| securitydefinitions.oauth2.application | [OAuth2 application](https://swagger.io/docs/specification/authentication/oauth2/) auth. | tokenUrl, scope, sec.def.description | // @securitydefinitions.oauth2.application OAuth2Application |
| securitydefinitions.oauth2.implicit | [OAuth2 implicit](https://swagger.io/docs/specification/authentication/oauth2/) auth. | authorizationUrl, scope, sec.def.description | // @securitydefinitions.oauth2.implicit OAuth2Implicit |
| securitydefinitions.oauth2.password | [OAuth2 password](https://swagger.io/docs/specification/authentication/oauth2/) auth. | tokenUrl, scope, sec.def.description | // @securitydefinitions.oauth2.password OAuth2Password |
| securitydefinitions.oauth2.accessCode | [OAuth2 access code](https://swagger.io/docs/specification/authentication/oauth2/) auth. | tokenUrl, authorizationUrl, scope, sec.def.description | // @securitydefinitions.oauth2.accessCode OAuth2AccessCode |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update sec.def.description with "description" for the parameter column and add also the "description" feature in the example column.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the example on the table below where the parameter annotation | example lives as I cannot do line breaks on the example cell of the mentioned table. Hope that is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ubogdan , as sec.def.description is a "line" parameter such as header, in, scopes etc; I had to name it in the mentioned way because if I named it description it will override (or take) the description at the general API info level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can of course change it to x-description as well as an alternative :)

Copy link
Contributor

@ubogdan ubogdan Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the following definition

// @securityDefinitions.basic BasicAuth
// @securityDefinitions.apikey ApiKeyAuth
// @in header
// @name Authorization
// @securitydefinitions.oauth2.application OAuth2Application
// @tokenUrl https://example.com/oauth/token
// @scope.write Grants write access
// @scope.admin Grants read and write access to administrative information
// @securitydefinitions.oauth2.implicit OAuth2Implicit
// @authorizationUrl https://example.com/oauth/authorize
// @scope.write Grants write access
// @scope.admin Grants read and write access to administrative information
// @securitydefinitions.oauth2.password OAuth2Password
// @tokenUrl https://example.com/oauth/token
// @scope.read Grants read access
// @scope.write Grants write access
// @scope.admin Grants read and write access to administrative information
// @securitydefinitions.oauth2.accessCode OAuth2AccessCode
// @tokenUrl https://example.com/oauth/token
// @authorizationUrl https://example.com/oauth/authorize
// @scope.admin Grants read and write access to administrative information

I expect to see something like

// @securitydefinitions.oauth2.application  OAuth2Application
// @tokenUrl                                https://example.com/oauth/token
// @scope.write                             Grants write access
// @scope.admin                             Grants read and write access to administrative information
// @description                             Perform OAUTH2 at application level

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You made it @security.definition.description instead of @description ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, did not see your edited comment 😅
I will try to make it work with just description but as I said before, when I tried it overlapped with the @description that you put to the general API info. I will debug and see if I can solve that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current way it works is the following, given these lines:

// @description this API is a demo
// @securitydefinitions.oauth2.application  OAuth2Application
// @description                             Perform OAUTH2 at application level

The code will:

  • Range every comment
  • For the first line it will set the API description to this API is a demo
  • For the second line it will range all comments starting from comments[i+1:] in other words skipping the current. This will effectively go through the @description and set it correctly.
  • For the third line it will set the API description (and override) to Perform OAUTH2 at application level

The issue is that the parseSecAttr function is iterating all the lines below until it finds another @securitydefinitions.. But then, the lines that parseSecAttr iterated, will be iterated back again by the main for buckle.

This is why I changed the name to something different. I can potentially see how to make the main buckle not go through the lines that the security definition already did, but that might introduce lots of changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ubogdan check it now, with the changes I have done it is working correctly and the main buckle does not re-iterate the lines that the security definitions already iterated :)

@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #1174 (d991a85) into master (050b0aa) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head d991a85 differs from pull request most recent head a117495. Consider uploading reports for the commit a117495 to get more accurate results

@@            Coverage Diff             @@
##           master    #1174      +/-   ##
==========================================
- Coverage   94.77%   94.71%   -0.06%     
==========================================
  Files          10       10              
  Lines        2449     2460      +11     
==========================================
+ Hits         2321     2330       +9     
- Misses         67       68       +1     
- Partials       61       62       +1     
Impacted Files Coverage Δ
parser.go 92.41% <100.00%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 050b0aa...a117495. Read the comment docs.

@pmorelli92 pmorelli92 requested a review from ubogdan April 19, 2022 06:57
ubogdan
ubogdan previously approved these changes Apr 19, 2022
Copy link
Contributor

@ubogdan ubogdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pmorelli92
Copy link
Contributor Author

@ubogdan I discovered a bug when you could have more than one security definition so I did the fix and added test cases for it.

Copy link
Contributor

@ubogdan ubogdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ubogdan ubogdan merged commit d209f71 into swaggo:master Apr 19, 2022
@ubogdan
Copy link
Contributor

ubogdan commented Apr 19, 2022

@pmorelli92 Thanks for your contribution.

@pmorelli92 pmorelli92 deleted the sec-def-description branch April 19, 2022 13:11
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.

None yet

2 participants