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.

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

Handle 304 (and others?) better in the node documentLoader #499

Open
marcvanandel opened this issue Oct 14, 2022 · 1 comment
Open

Handle 304 (and others?) better in the node documentLoader #499

marcvanandel opened this issue Oct 14, 2022 · 1 comment

Comments

@marcvanandel
Copy link

marcvanandel commented Oct 14, 2022

If a context that has to be resolved returns a HTTP 304 the node documentLoader does not resolve properly. It seems that all HTTP 300 - 399 codes are handled like redirects ... which they are not.

More precisely in this comments at lib/documentLoaders/node.js:127 where a 304 is handled similarly as a 302 ... but a 304 means 'not modified on the server side' (see mozilla http 304 documentation).

This can be reproduced by altering the node-document-loader-tests.js:83 and add .status(304) to the response:

_app.get('/data/:id', _store, (req, res) => {
  res.status(304).json(_data[req.params.id].content);
});

I stumbled upon this bug (feature?) with loading the json-ld context of the ed25519-signature-2020 because this returns a 304 (200 first time but 304 all other times; it also contains a redirect first)

@dlongley
Copy link
Member

dlongley commented Oct 14, 2022

We should simplify the node document loader to not use the manual redirect flag anymore. It was written a long time ago -- and now relies on fetch, and the fetch implementation (as of who knows when) now properly follows the fetch spec and returns an opaque response when manual is used. We needed to use manual flag in the past to check for cycles and to get the final redirection URL to return as documentUrl.

From what I've read, we can rely on fetch to handle cycles and too many redirects and it returns the final url in the response as the .url property. We should be able to now remove the manual flag as well as any old commented caching code and special redirect handling code. We will still need to handle the link header -- because that's JSON-LD specific, but we should not need special HTTP 3xx handling code anymore.

I'll also note that the context recursion error we have in the node loader is not a proper error (that's handled elsewhere) for a number of reasons ... and the above changes would eliminate the need for it anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants