Add Router to runtime.Params #3458
Conversation
server/server_test.go
Outdated
| r, err := http.NewRequest(http.MethodGet, "/customEndpoint", nil) | ||
| if err != nil { | ||
| t.Fatalf("Unexpected error: %s", err) | ||
| } | ||
| if err := f.executeRequest(r, http.StatusOK, ""); err != nil { | ||
| t.Fatalf("Request to custom endpoint failed: %s", err) |
There was a problem hiding this comment.
I can make a guess about the current behaviour but it would be useful to verify that (a) the normal server routes are intact and (b) what happens if the provided router contains routes that overlap w/ the OPA server (e.g., what if the router had been called with "/v1/data"?)
There was a problem hiding this comment.
I agree with point a. You might check existing routes are good after you've initialized this new router. It should be ok according to mux docs but knowing for sure is good too.
On point b) are you not testing the behavior of mux if you do that? Mux does not allow duplicate routes afaik but I would expect any behavior of duplicate routes to behave as documented by mux.
Some note on this behavior in the docs though would likely help.
There was a problem hiding this comment.
Good points.
a) I added a call to /v1/data to ensure default routes are still available
b) Turns out Gorilla matches routes in registration order, so overriding a route as requested originally in #2777 is possible (I tested this as well). This behaviour is explicitly documented in the Gorilla README, so I think we can expect Gorilla to behave like this even without a test.
jkbschmid
changed the title
There was a problem hiding this comment.
Thanks, this is a nice contribution 👏
The code looks good to me, there are two small things, with those addressed, I'd happily get this merged:
- The "first matching route wins" behaviour is currently only mentioned in review comments on this PR. I think it could be made more apparent, by adding another sentence to the Router comments in runtime.go (or alternatively adding a test case).
- If you don't mind, please squash your commits before we merge this. :)
Thanks again! 🎉
- Allows extending the REST API by creating a custom runtime object - Allows overriding existing routes since Router matches routes in order of registration Signed-off-by: Jakob Schmid <jakob.schmid@sap.com>
Thanks for your reviews :) |
All changes to public APIs must be accompanied with
docs. Examples of public APIs include built-in functions,
config fields, and of course, exported Go types/functions/constants/etc.
Not sure how/whether to document a change of
runtime.Params