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.

Sign up for GitHub

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

Jump to bottom

feat: add URLParam function to router #259

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add URLParam function to router #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions router/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ type Router interface {
Mount(pattern string, h http.Handler)

ServeHTTP(http.ResponseWriter, *http.Request)

// Returns the value of a URL parameter
URLParam(*http.Request, string) string
}

// New creates a router with sensible defaults (xff, request id, cors)
Expand Down Expand Up @@ -151,6 +154,11 @@ func (r *chiWrapper) Mount(pattern string, h http.Handler) {
r.chi.Mount(pattern, h)
}

// Returns the value of a URL parameter
func (r *chiWrapper) URLParam(req *http.Request, name string) string {
return chi.URLParam(req, name)
}
Comment on lines +158 to +160
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't get why this needs to be a method on the chiWrapper?
why not make this a freestanding function?

Copy link
Member Author

@eduardoboucas eduardoboucas Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't have to be, as it doesn't access the chi instance, but from a functional perspective it felt like the appropriate place. I don't have a strong preference, though. What location do you suggest? router/helpers.go?

Copy link
Contributor

Choose a reason for hiding this comment

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

i feel like it could just be some code like this to reexport it?

const URLParam = chi.URLParam

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally prefer having a method that we explicitly implement and which we can customise the behaviour of in the future, if needed.

Is this a blocker for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

you can replace a reexport with a freestanding function at any point, it will not break anyone except if the signature differs which is the same if it is a function now.


// =======================================
// HTTP handler with custom error payload
// =======================================
Expand Down
15 changes: 15 additions & 0 deletions router/router_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,21 @@ func TestVersionHeader(t *testing.T) {
}
}

func TestURLParam(t *testing.T) {
r := New(logrus.WithField("test", t.Name()), OptEnableTracing("some-service"))

r.Get("/objects/{objectId}", func(w http.ResponseWriter, req *http.Request) error {
w.Write([]byte(r.URLParam(req, "objectId")))
w.WriteHeader(http.StatusOK)
return nil
})

rec := httptest.NewRecorder()
r.ServeHTTP(rec, httptest.NewRequest("GET", "/objects/some-object", nil))
assert.Equal(t, []byte("some-object"), rec.Body.Bytes())
assert.Equal(t, http.StatusOK, rec.Code)
}

func do(t *testing.T, opts []Option, svcName, path string, handler APIHandler, req *http.Request) *httptest.ResponseRecorder {
if opts == nil {
opts = []Option{}
Expand Down