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

gorillamux.NewRouter fails if any servers url contain variable for port number of the url #367

Closed
karupanerura opened this issue Jun 9, 2021 · 6 comments · Fixed by #524

Comments

@karupanerura
Copy link

e.g.

openapi: "3.0.0"
servers:
  - url: http://127.0.0.1:{port}/
    variables:
      port:
        default: '8000'

It throws error at https://github.com/getkin/kin-openapi/blob/v0.63.0/routers/gorillamux/router.go#L41

parse "http://127.0.0.1:--------------------------------------------------port__________________________________________________": invalid port ":--------------------------------------------------port__________________________________________________" after host
@fenollp
Copy link
Collaborator

fenollp commented Jun 10, 2021

Ah yes this is related to #337 and the issue it attempts to solve.
In my mind there isn't a perfect solution to this as it relies on url.Parse's leniency. A good enough solution might be to use the default value of variables in the host but then your port will only ever match 8000. Which is fine as long as it is documented I guess.

@karupanerura
Copy link
Author

Thank you for the kind reply. My understanding is almost exactly the same as yours.

In my opinion, url.Parse is only intended to validate the URL format. So, I think that creating a URL string using the default value of variables in the host and validating it will serve the purpose.
Next, (*mux.Route).Host method can accept the OpenAPI 3.0 servers URL template syntax(by accident). So, I think it can pass the host section of the template directly to this method.
I think it can solve this issue.

If this idea is good for you, I can send you a patch. What do you think?

@slessard
Copy link
Contributor

Now I've hit this problem. @karupanerura I'd be interested in trying out a patch if you have one

slessard pushed a commit to slessard/kin-openapi that referenced this issue Mar 31, 2022
@slessard
Copy link
Contributor

I submitted a patch for this issue but it is failing one of the unit tests. The specific test case is this:

	doc.Servers = []*openapi3.Server{
		{URL: "https://www.example.com/api/v1"},
		{URL: "{scheme}://{d0}.{d1}.com/api/v1/", Variables: map[string]*openapi3.ServerVariable{
			"d0":     {Default: "www"},
			"d1":     {Default: "example", Enum: []string{"example"}},
			"scheme": {Default: "https", Enum: []string{"https", "http"}},
		}},
	}

and

	expect(r, http.MethodGet, "https://domain0.domain1.com/api/v1/hello", helloGET, map[string]string{
		"d0": "domain0",
		"d1": "domain1",
		// "scheme": "https", TODO: https://github.com/gorilla/mux/issues/624
	})

Do you have any guidance on how to make this test happy while still resolving #367?

@fenollp
Copy link
Collaborator

fenollp commented Apr 1, 2022

I've opened #524 that handles this specific issue.

@slessard Your PR (at this time) may solve this issue but it uses defaults for all variables not just this one specific case of port vars and so is not backwards compatible.

@karupanerura @slessard please give feedback
If you guys are happy with it I'll merge this and close #523

@slessard
Copy link
Contributor

slessard commented Apr 5, 2022

Sorry for the delay. I am looking at this now

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 a pull request may close this issue.

3 participants