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

fix: router now support colon sign #552

Merged
merged 1 commit into from Nov 9, 2019

Conversation

Nephylhim
Copy link
Contributor

Useful for custom methods using custom verbs

Describe the PR
Add colon sign support for router parse method.

Relation issue
#551

Additional context
Custom methods as described in the Google API design documentation (https://cloud.google.com/apis/design/custom_methods) does not work ATM. ex:
/objects:move [post]

Unit test are passing. Fix works well on my side.
Do you want me to change the commit type from "fix" to "feat"?
Is the unit test enough?

Have a good day!

@codecov-io
Copy link

codecov-io commented Nov 6, 2019

Codecov Report

Merging #552 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #552   +/-   ##
=======================================
  Coverage   87.24%   87.24%           
=======================================
  Files           7        7           
  Lines        1560     1560           
=======================================
  Hits         1361     1361           
  Misses        117      117           
  Partials       82       82
Impacted Files Coverage Δ
operation.go 92.6% <ø> (ø) ⬆️

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 c05c273...70f6318. Read the comment docs.

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.

Regexp should not allow invalid paths.
Please fix regexp and add additional tests to cover my example.

operation.go Outdated
@@ -437,7 +437,7 @@ func parseMimeTypeList(mimeTypeList string, typeList *[]string, format string) e
return nil
}

var routerPattern = regexp.MustCompile(`([\w\.\/\-{}\+]+)[^\[]+\[([^\]]+)`)
var routerPattern = regexp.MustCompile(`([\w\.\/\-{}\+:]+)[^\[]+\[([^\]]+)`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the regexp accordigly
The following routes should not be valid
"/@router :customer/get-wishlist/{wishlist_id}:move [post]"
path format

In addition, a URI reference (Section 4.1) may be a relative-path reference, in which case the first path segment cannot contain a colon (":") character.

@Nephylhim
Copy link
Contributor Author

Original regex: ([\w\.\/\-{}\+]+)[^\[]+\[([^\]]+)

Well, I've come up with 4 different solutions, all of them pass the tests (explanation are just after):

  1. ^(/[\w\.\/\-{}\+:]+)[^\[]+\[([^\]]+)
  2. ^(/[\w\.\/\-{}\+:]+)[[:blank:]]+\[(\w+)]
  3. ^([\w\.\/\-{}\+]+:?[\w\.\/\-{}\+:]*)[^\[]+\[([^\]]+)
  4. ^([\w\.\/\-{}\+]+(:[\w\.\/\-{}\+:]+)?)[^\[]+\[([^\]]+)

Now the explanations:

  1. It's the solution that change the less characters to the original regex
    • ^ at the start of the regex to ensure the first character is correct
    • (/[\w\.\/\-{}\+:]+) instead of ([\w\.\/\-{}\+]+) will ensure that the / will always be the first character. After that, : is added to the range of accepted characters
    • The rest of the regex haven't been changed
  2. It's the solution that change the most the regex. But I do think it helps it be clearer and respects more the documentation.
    • ^(/[\w\.\/\-{}\+:]+): same explanations as just above
    • [[:blank:]]+ instead of [^\[]+: use whitespaces or tabs as separation instead of any non [ characters. Your documentation specify to use spaces as separators (https://github.com/swaggo/swag#api-operation).
    • \[(\w+)] instead of \[([^\]]+: capture the word between the [] characters instead of taking every characters possible but a ]. Actually, this comment works: /@Router /api/{id}|,*[get. A HTTP method should be a word, so \w is supposed to do the job.
  3. It's the solution that modify the less the original behavior. However, it weighs down the regex a lot.
    • ^ at the start of the regex to ensure the first character is correct
    • ([\w\.\/\-{}\+]+:?[\w\.\/\-{}\+:]*) instead of ([\w\.\/\-{}\+]+). :? allows a : to exists after a path and [\w\.\/\-{}\+:]* allows to continue the URI
    • The rest of the regex haven't been changed
  4. An alternative to the third solution. A little bit more explicit but, as go's regex doesn't support non-capturing groups, it forces to change (the code) httpMethod to matches[3] and ensure that len(matches) != 4 instead of 3

I'll update my MR with the second solution as I do think it is the best one.
I'll also add a bonus test case.

I'll update the MR if you think another solution is better.
Bests

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

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

3 participants