fix(arns): return right away on arns HEAD requests #202
Conversation
Avoid fetching data when handling HEAD requests for arns names
WalkthroughWalkthroughThe changes introduce a new constant, Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review due to trivial changes (1)
Additional comments not posted (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #202 +/- ##
========================================
Coverage 70.16% 70.16%
========================================
Files 32 32
Lines 7829 7829
Branches 438 438
========================================
Hits 5493 5493
Misses 2336 2336 ☔ View full report in Codecov by Sentry. |
| @@ -105,5 +105,11 @@ export const createArnsMiddleware = ({ | |||
| res.header(headerNames.arnsProcessId, processId); | |||
| // TODO: add a header for arns cache status | |||
| res.header('Cache-Control', `public, max-age=${ttl}`); | |||
|
|
|||
| if (req.method === REQUEST_METHOD_HEAD) { | |||
There was a problem hiding this comment.
I'm unclear what we're fixing by doing this. We already terminate early and tear down the stream in the data handler if we get a HEAD request. Is that not working as intended? If return hear our responses won't include the correct Content-Length and Content-Type.
There was a problem hiding this comment.
AH, forgot about those headers. Yes, we still end up fetching the data item stream which are then used to set the headers (as implemented)- here
We do call getDataAttributes earlier in the handler. We could check the request method at that point, set the data headers based on those attributes, and return the requests before calling dataSource.getData(). We do stop the stream right away on HEAD requests, so ultimately, doing this would prevent fetching of the data item stream entirely, with the concern being the data attributes we've fetched don't contain or match the other headers available in the data stream.
There was a problem hiding this comment.
after some more discussion, we will close this PR and:
- create a ticket to pass the request type to
getData()call to avoid creatingGETmethod and then just terminating the stream onHEADrequests. - create a ticket to introduce resolver API endpoints to allow resolving names directly without worrying about potential data fetching/caching behavior in these middlewares
Avoid fetching data when handling HEAD requests for arns names