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

fix(arns): return right away on arns HEAD requests #202

Closed
wants to merge 1 commit into from
Closed

fix(arns): return right away on arns HEAD requests #202

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
2 changes: 1 addition & 1 deletion src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const headerNames = {
arnsResolvedId: 'X-ArNS-Resolved-Id',
arnsProcessId: 'X-ArNS-Process-Id',
};

export const REQUEST_METHOD_HEAD = 'HEAD';
export const DATA_PATH_REGEX =
/^\/?([a-zA-Z0-9-_]{43})\/?$|^\/?([a-zA-Z0-9-_]{43})\/(.*)$/i;
export const RAW_DATA_PATH_REGEX = /^\/raw\/([a-zA-Z0-9-_]{43})\/?$/i;
Expand Down
8 changes: 7 additions & 1 deletion src/middleware/arns.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { Handler } from 'express';
import { asyncMiddleware } from 'middleware-async';

import * as config from '../config.js';
import { headerNames } from '../constants.js';
import { headerNames, REQUEST_METHOD_HEAD } from '../constants.js';
import { sendNotFound } from '../routes/data/handlers.js';
import { DATA_PATH_REGEX } from '../constants.js';
import { NameResolution, NameResolver } from '../types.js';
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@dtfiedler dtfiedler Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@dtfiedler dtfiedler Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

after some more discussion, we will close this PR and:

  • create a ticket to pass the request type to getData() call to avoid creating GET method and then just terminating the stream on HEAD requests.
  • create a ticket to introduce resolver API endpoints to allow resolving names directly without worrying about potential data fetching/caching behavior in these middlewares

res.end();
return;
}

dataHandler(req, res, next);
});
4 changes: 1 addition & 3 deletions src/routes/data/handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { default as asyncHandler } from 'express-async-handler';
import { PassThrough, Transform } from 'node:stream';
import rangeParser from 'range-parser';
import { Logger } from 'winston';
import { headerNames } from '../../constants.js';
import { headerNames, REQUEST_METHOD_HEAD } from '../../constants.js';

import { MANIFEST_CONTENT_TYPE } from '../../lib/encoding.js';
import {
Expand All @@ -39,8 +39,6 @@ const NOT_FOUND_MAX_AGE = 60; // 1 minute

const DEFAULT_CONTENT_TYPE = 'application/octet-stream';

const REQUEST_METHOD_HEAD = 'HEAD';

const setDataHeaders = ({
res,
dataAttributes,
Expand Down