Ignore query params when using customRoutes with SocketModeReceiver #1207
Conversation
|
|
|
Thanks again for taking the initiative to address the bug you've identified, @moustacheful! 🙌🏼 The same (flawed) logic is also present in the |
misscoded
added
the
bug
Hey! no problem @misscoded. I believe the HTTPReceiver might not run into the same issue because of this: URL.pathname does not include the query params section of the URL. that is, unless I'm looking at the wrong file 🕵️ |
|
I've updated the PR to have the same implementation as HTTPReceiver, using URL instead. I'm sure it can save us some headache on some random edge case I might not have been considering with the |
moustacheful
changed the title
There was a problem hiding this comment.
You're absolutely right! I'd entirely forgotten that the two receivers handled the req.url differently.
I left a note about the hardcoded localhost reference and what I anticipate will be an associated failing test, but other than that, everything looks to be in order. 👍🏼
| const match = this.routes[req.url] && this.routes[req.url][method] !== undefined; | ||
| // NOTE: the domain and scheme are irrelevant here. | ||
| // The URL object is only used to safely obtain the path to match | ||
| const { pathname: path } = new URL(req.url as string, 'http://localhost'); |
There was a problem hiding this comment.
We probably want to switch this hard-coded string out for the same http://${req.headers.host} that's featured in the HTTPReceiver implementation.
I believe that as a result, one of the tests will fail and the fakeReq will need to be augmented with the headers object (similar to what's seen in the test you've introduced).
There was a problem hiding this comment.
I actually changed it from that because it made it look like it had a purpose. As the comment on the other side explained
// NOTE: the domain and scheme of the following URL object are not necessarily accurate. The URL object is only
// meant to be used to parse the path and query
it doesn't actually serve any purpose beyond extracting the pathname, it's just there because URL needs a URL and not a URI.
I don't feel too strongly though, let me know if you still want it that way, I can fix it tomorrow.
There was a problem hiding this comment.
@moustacheful Thanks for elaborating here!
You're right that, for the second argument in the URL constructor, choosing either localhost or req.headers.host does not matter in this scenario. That being said, we don't have a strong reason to use localhost (I know that's a common code example, though). Plus, we prefer consistency across the code base in general.
Could you do the same with the code in HTTPReceiver tomorrow? Thanks again for taking the time to report and fix this issue!
There was a problem hiding this comment.
Hey yeah, no problem, I've updated HTTPReceiver to be consistent with SocketReceiver, and added a test case for this scenario on the former.
|
My suggestion in my last comment was to use That being said, how we construct the URL objects is not important here. Either way, we don't extract the hostname value from URL objects. If other maintainers are fine with this change, please go ahead 👍 |
Codecov Report
@@ Coverage Diff @@
## main #1207 +/- ##
==========================================
+ Coverage 72.22% 72.26% +0.03%
==========================================
Files 17 17
Lines 1433 1435 +2
Branches 430 430
==========================================
+ Hits 1035 1037 +2
Misses 322 322
Partials 76 76
Continue to review full report at Codecov.
|
misscoded
changed the title
misscoded
changed the title
Summary
#1206
Allows for using query params in custom routes. Currently it will attempt to match the whole url including the query params.
With this change, it will skip the query params when matching.
Requirements (place an
xin each[ ])