-
Notifications
You must be signed in to change notification settings - Fork 70
Add url pattern field #562
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
Conversation
|
Hi @ChuntaoLu @jacobgreenleaf Could I get this and #559 reviewed? A middleware in edge gateway needs access to these fields for logging purposes. Thank you! |
| bgateway.ActualGateway.ContextExtractor, | ||
| deps, | ||
| "foo", "foo", | ||
| "foo", "foo", "/foo?%gh&%ij", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this test is for the specific url, but it is wired to say "this endpoint handles requests with the url that has the specific query values". Usually we only specify the url path, i.e., without the ? part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a mistake on my end. In its place is supposed to be just "/foo"
| HandlerFn HandlerFn | ||
| EndpointName string | ||
| HandlerName string | ||
| RawURLPattern string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means an endpoint is always associated with a url, what would be the answers to following questions:
- what is the expectation for below?
router.Handle("GET", "/foo", http.HandlerFunc(zanzibar.NewRouterEndpoint(a, b, "foo", "foo", "/bar", c)))
it uses an endpoint that is for /bar to handle /foo, seems error-prone and shouldn't be allowed?
- previously I can use the same handler to handle different urls, now I will have to create one endpoint per url?
Overall, I don't think it is a good idea to couple the association between an url and an endpoint with the endpoint itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your points for concern.
However, I need a way to uniquely identify an endpoint in a form canonical to both edge gateway and rtapi. The two choices are the url or thrift method, since EndpointName and HandlerName are specific to edge gateway and not to rtapi. However, passing even the thrift method into NewRouterEndpoint would be coupling the association between a url and an endpoint with the endpoint itself because the thrift method is mapped 1-to-1 with the url.
I'll try to figure out a different way to uniquely identify an endpoint, but please let me know if you have any suggestions.
Again, thanks a lot for the review!
Can you describe the purpose more? Would it be sufficient to log the URL path part when you receive a request rather than the pattern that the path matched? |
URL path part is sufficient. However, I need the path part before the path parameters have been filled in. For example, In terms of purpose, I need a way to uniquely identify an endpoint in a form which is canonical to both edge gateway and rtapi, and the url seemed like a good candidate. |
Shouldn't the URL after the path parameters are filled in also be sufficient? After all when the router receives the URL it will be with the path parameters filled in. The router will only match the URL with one pattern. Would like to hear more about the use case. |
A middleware needs access to rawURLPattern for logging purposes. Included this field in ServerHTTPRequest