Skip to content

Recognize path parameters in path segment fragments #39 #40

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

Merged
merged 1 commit into from
Nov 17, 2017

Conversation

elakito
Copy link
Contributor

@elakito elakito commented Nov 10, 2017

PR for #39

@casualjim
Copy link
Member

I am not sure if this is supported for openapi 2.0 spec

@codecov
Copy link

codecov bot commented Nov 10, 2017

Codecov Report

Merging #40 into master will increase coverage by 0.04%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   84.49%   84.53%   +0.04%     
==========================================
  Files          10       10              
  Lines        1412     1429      +17     
==========================================
+ Hits         1193     1208      +15     
- Misses        177      178       +1     
- Partials       42       43       +1
Impacted Files Coverage Δ
spec.go 85.89% <90%> (+0.08%) ⬆️

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 a52193a...6de7a6b. Read the comment docs.

@elakito
Copy link
Contributor Author

elakito commented Nov 14, 2017

Hi @casualjim, thank you for looking into this. I don't find the Open-API 2.0 spec explicitly allowing this URI template usage but neither forbidding this usage of URI templates. There is a related ticket about this URI template usage at OAP/OpenAPI-Specification where Ron mentioned that the spec itself does not forbid this usage.
OAI/OpenAPI-Specification#1248
regards, aki

@casualjim
Copy link
Member

ok, but making this work for just validation might not be enough, are you planning on also sending a PR to the runtime to support this in the router?

@elakito
Copy link
Contributor Author

elakito commented Nov 15, 2017

Yes. i'll work on the corresponding change to the server side and create a PR. It seems the denco code that does the parameters extraction needs to be modified. The path matching is actually working and the operation is fired but the parameters are not extracted correctly. By the way, why the denco code is in middleware/ and not in vendor? Has it been customized locally?
thanks.

@elakito
Copy link
Contributor Author

elakito commented Nov 15, 2017

Hi @casualjim,
Initially, I thought we had to modify the denco code so that the parameters are matched and extracted from that layer. But upon looking in it more, it would be better to not change the denco code but pass a different url template from go-openapi (i.e., just one param per segment) to let it match and extract the entire segment from the denco layer, and do the actual parameter matching using this extracted string against the real parameters. There is a limitation in this approach (i.e., there is no backtracking once matched but if that is required, the denco code will need to be modified in an incompatible way, which we can avoid for now).

By the way, without this change in go-openapi/runtime, the client side code already works fine and also the server side code if there is one parameter per segment e.g.
/foo/lis{fragment}

So maybe, you could merge this PR into validation and I can open an issue in runtime to track this more-than one param per segment at the server side issue?

regards, aki

@casualjim casualjim merged commit d509235 into go-openapi:master Nov 17, 2017
@casualjim
Copy link
Member

@elakito 🥇 thanks

@elakito elakito deleted the issue-39 branch November 20, 2017 09:28
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.

2 participants