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

updated logic for making a range request with no end value #156

Merged
merged 6 commits into from
Mar 6, 2023
Merged

updated logic for making a range request with no end value #156

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9794779
updated logic for making a range request with a negative value
danielRicaud Mar 2, 2023
88c6951
Merge branch 'main' of github.com:opentdf/client-web into feature/fix…
danielRicaud Mar 2, 2023
d846995
Update chunkers.ts
danielRicaud Mar 6, 2023
393c22a
Update chunkers.ts
danielRicaud Mar 6, 2023
4f437d4
Update chunkers.ts
danielRicaud Mar 6, 2023
2383d79
Update chunkers.ts
danielRicaud Mar 6, 2023
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
4 changes: 2 additions & 2 deletions lib/tdf3/src/utils/chunkers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,8 @@ export const fromUrl = (location: string): Chunker => {
if (byteEnd && byteEnd < 0) {
// NOTE: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Range
throw Error('negative end unsupported');
} else {
rangeHeader += `-${(byteEnd || 0) - 1}`;
} else if (byteEnd) {
rangeHeader += `-${byteEnd - 1}`;
}
Copy link
Member

@elizabethhealy elizabethhealy Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

you might want to add an extra else for when byteEnd is undefined and throw an error there, or log the error?

Copy link
Member

Choose a reason for hiding this comment

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

Required fix: Update line 103 to end with - and remove the - from line 108. Also, 0 is always an invalid byteEnd for the current implementation, the guard on 104 should be !byteEnd || byteEnd < 0.

Actually reading the range header docs was informative. Notably, it does not implement the full slice API, so we'd need to update this to first run a request to see how big the file is, allowing us to implement the negative end output. For now we can throw an Unsupported error as above and when/if we start seeing that we can go back and add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure we should automatically add - to line 103 since negative range's shouldn't append the -. I've changed 107 to not allow byteEnd to be a zero value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also in the case of byteEnd being undefined that's ok in the case of negative range headers like -1000

Copy link
Member

Choose a reason for hiding this comment

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

So if byteEnd exists, it must:

  • not be negative
  • byteStart can't be negative either, otherwise we need to use the suffix format (last N bytes, -N)
  • must be strictly greater than byteStart

So it should be like:

  if (byteEnd) {
    if (byteStart < 0 || byteEnd <= byteStart) {
      throw Error(`invalid HTTP range [${byteStart}-${byteEnd}]`);
    }
    rangeHeader += `-${(byteEnd || 0) - 1}`;
  }

Copy link
Member

Choose a reason for hiding this comment

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

details: https://httpwg.org/specs/rfc9110.html#rfc.section.14.1.1

Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Pull this out as an exported function, just so you can add some unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmihalcik-virtru implemented the changes. I'll make a quick Jira ticket to pull this function out and add unit tests just because it needs to be released as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this logic back to the original implementation because throwing an error that early was causing a unit test to fail that's already making sure the Range is satisfiable. On line 92 if an Unsatisfiable range is encountered with an HTTP response code of 416 the test catches the issue correctly

return getRemoteChunk(location, rangeHeader);
};
Expand Down