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

Allow different names for swagger specs #885

Closed
wants to merge 3 commits into from

Conversation

joshk0
Copy link

@joshk0 joshk0 commented Feb 15, 2021

Describe the PR

In the case where we need to include multiple docs packages in the same
process, swag.Register will fail. Despite looking like having support
for swagger specs with different names it actually does not work.

Define RegisterName and ReadDocName functions which allow you to specify
the name of the swag. Default method will still use this path but with
the 'classic' swag name of 'swagger'.

Relation issue

None

Additional context

None

@joshk0 joshk0 marked this pull request as ready for review February 15, 2021 21:42
joshk0 added a commit to joshk0/echo-swagger that referenced this pull request Feb 15, 2021
For backwards compatibility, preserve the functionality of
EchoWrapHandler but create new EchoWrapHandlerName which accepts a name
parameter.

See: swaggo/swag#885
@joshk0 joshk0 force-pushed the named-swags branch 3 times, most recently from 3cf89d5 to afa7d2c Compare February 15, 2021 22:18
In the case where we need to include multiple docs packages in the same
process, swag.Register will fail. Despite looking like having support
for swagger specs with different names it actually does not work.

Define RegisterName and ReadDocName functions which allow you to specify
the name of the swag. Default method will still use this path but with
the 'classic' swag name of 'swagger'.
@joshk0
Copy link
Author

joshk0 commented Feb 16, 2021

The CI fails for the same reason as the latest master commit.

@ubogdan
Copy link
Contributor

ubogdan commented Feb 17, 2021

We need to investigate and rollback asap. Please be patient.

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #885 (9882772) into master (17c1766) will decrease coverage by 0.24%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
- Coverage   84.83%   84.59%   -0.25%     
==========================================
  Files           8        8              
  Lines        1649     1655       +6     
==========================================
+ Hits         1399     1400       +1     
- Misses        145      147       +2     
- Partials      105      108       +3     
Impacted Files Coverage Δ
gen/gen.go 95.27% <ø> (ø)
swagger.go 68.75% <50.00%> (-31.25%) ⬇️

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 17c1766...9882772. Read the comment docs.

@joshk0
Copy link
Author

joshk0 commented Apr 5, 2021

Hi, is this waiting for me to add tests to restore the code coverage?

@sp3c73r2038
Copy link

+1 to this. I want to serve multiple swagger instances from one gin router. Currently swag.Register could only called once, no matter what name parameter is.

This will require some changes in gin-swagger CustomHandler too.

@ubogdan
Copy link
Contributor

ubogdan commented Jul 27, 2021

Hi, is this waiting for me to add tests to restore the code coverage?

Yes. You introduced new code branches that are not covered by testing.

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.

We can't introduce breaking changes.

}

// ReadDocName reads swagger document with specific name.
func ReadDocName(name string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the function signature is not an acceptable solution. This is a breaking change and breaks all other projects.
func ReadDoc() (string, error) {
to
func ReadDocName(name string) (string, error) {

func ReadDocName(name string) (string, error) {
if swags == nil {
return "", errors.New("no swag has yet been registered")
} else if swag, ok := swags[name]; !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Race condition: searching in the swags map without a mutex !

return "", errors.New("no swag has yet been registered")
} else if swag, ok := swags[name]; !ok {
return "", fmt.Errorf("no swag named \"%s\" was registered", name)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

please use return instead if nested else.

@@ -309,6 +309,6 @@ func (s *s) ReadDoc() string {
}

func init() {
swag.Register(swag.Name, &s{})
swag.Register({{ printf "%q + %q" .Host .BasePath}}, &s{})
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected behavior In the event of many API applications don't have a Host and Basepath defined?

@ubogdan
Copy link
Contributor

ubogdan commented Oct 11, 2021

Closing: fixed in #1022!

@ubogdan ubogdan closed this Oct 11, 2021
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