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 a customizable function for parsing the request URL #47

Merged
merged 4 commits into from
Nov 21, 2019

Conversation

kristinapathak
Copy link
Contributor

@kristinapathak kristinapathak commented Nov 5, 2019

This will help us get rid of prefixes on request urls (like /api/v1) before adding them to the context for easier parsing against capabilities for XMiDT/Codex services.

@kristinapathak kristinapathak added the enhancement New feature or request label Nov 5, 2019
@codecov-io
Copy link

codecov-io commented Nov 5, 2019

Codecov Report

Merging #47 into master will decrease coverage by 0.84%.
The diff coverage is 61.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
- Coverage   77.72%   76.88%   -0.85%     
==========================================
  Files          25       25              
  Lines         696      731      +35     
==========================================
+ Hits          541      562      +21     
- Misses        137      151      +14     
  Partials       18       18
Impacted Files Coverage Δ
basculehttp/errorResponseReason.go 100% <ø> (ø) ⬆️
context.go 100% <ø> (ø) ⬆️
basculehttp/notfoundbehavior_string.go 25% <0%> (-25%) ⬇️
basculehttp/errorresponsereason_string.go 0% <0%> (ø) ⬆️
basculehttp/constructor.go 100% <100%> (ø) ⬆️

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 59d5c05...06b0783. Read the comment docs.

@kristinapathak
Copy link
Contributor Author

will help with fixing this issue: xmidt-org/webpa-common#62

return u.EscapedPath(), nil
}

func CreateRemovePrefixURLFunc(prefix string) func(*url.URL) (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.

Could the signature of the returned closure not be func(*url.URL) (*url.URL, error)? In other words, return the original URL modified appropriately rather than the string representation. This approach has two (2) benefits:

(1) You or your client can compose things easily, without having to reparse the string repeatedly. For example:

type GetURL func(*url.URL) (*url.URL, error)
func removePrefix(prefix) GetURL { /* remove the prefix */ }
func anotherThing(next GetURL) GetURL {
    return func(u *url.URL) (*url.URL, error) {
        /* do some other stuff */
        return next(u)
    }
}

(2) A *url.URL can be useful to a client who wants to do further processing outside bascule. This is related to (1), but has more to do with a client using the results of a GetURL strategy rather than your code.

Since *url.URL is mutable, it's generally good practice to make a copy of the HTTP request's URL member prior to invoking GetURL. You can do this very simply with u := *request.URL, in which case u is of type url.URL and you'll pass &u around.

@johnabass johnabass merged commit 0b3d3fb into master Nov 21, 2019
@kristinapathak kristinapathak deleted the url-parsing branch July 27, 2020 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants