nodeSolidServer / node-solid-server

Solid server on top of the file-system in NodeJS
https://solidproject.org/for-developers/pod-server
Other
1.77k stars 297 forks source link

Change translation error (500) to not acceptable (406) #1752

Closed csarven closed 5 months ago

csarven commented 5 months ago

The problem is that the server returns a 500 in a situation where it tries to parse a document that it can't parse as RDF, and says "Error translating between RDF formats". This happens when the request (Accept header) includes a media type of a concrete RDF syntax, and the server attempts to parse the document using one of its RDF parsers.

This is not a 500, "unexpected condition" per se. It is an expected condition.

The server should be responding with a 406, not acceptable, because client is asking for a particular representation of the resource (using an RDF media type) and the server is unable to provide one.

I think the flow of code leading up to this can be better / refactored but I didn't want to touch it in this PR.


Message to application developers: your application can get around the current issue (500) by including */* (e.g., */*; q=0.1) in Accept, which is a reasonably useful thing to do but YMMV. Your application will then get a 200 representation (whatever the native format of the file/resource is on) instead of a 500. If this PR is merged, your application may get a 406 or 200, which is arguably better to work with than a 500.

bourgeoa commented 5 months ago

What was your use case. Was it text/html to RDFa ?

RDFs are defined here https://github.com/nodeSolidServer/node-solid-server/blob/83dad063955e8e42e81eb413c88710234675aa0d/lib/ldp.js#L27-L38

csarven commented 5 months ago

No, anything, that's not in that list

Try, e.g.:

curl -iH'Accept: text/turtle' https://csarven.solidcommunity.net/public/test.rss

500 response is given with Content-Type: text/plain; charset=utf-8 and "Error translating between RDF formats" in response body.

This is just a case where server can't provide a Turtle representation of that resource. It should be 406.

See also:

curl -iH'Accept: text/turtle, */*; q=0.1' https://csarven.solidcommunity.net/public/test.rss

200 response is given with Content-Type: application/rss+xml

bourgeoa commented 5 months ago

No, anything, that's not in that list

Try, e.g.:

curl -iH'Accept: text/turtle' https://csarven.solidcommunity.net/public/test.rss

Then there is an other issue. 406 should already be used. See https://github.com/nodeSolidServer/node-solid-server/blob/83dad063955e8e42e81eb413c88710234675aa0d/lib/handlers/get.js#L117-L121

csarven commented 5 months ago

If the selected representation's media type (what's stored on disk) is not in the list of serializable RDF media types, then no need to try to translate, and so should return 406. Only in the case when one of the RDF media types acceptable by the client that's also in the list of serializable RDF media types translation should be attempted.

bourgeoa commented 5 months ago

That's what the actual code tries to do and for to be checked reason is not doing in L117

csarven commented 5 months ago

What do you think of https://github.com/nodeSolidServer/node-solid-server/pull/1752/commits/65b04bbff7153d601c8b7de625d27cfc4365d336

bourgeoa commented 5 months ago

You can add tests in formats-test.js

  describe('TXT', function () {
    it('Accept text/plain', function (done) {
      server.get('/put-input.txt')
        .set('accept', 'text/plain')
        .expect('Content-type', 'text/plain; charset=utf-8')
        .expect(200, done)
    })
    it('Accept text/turtle', function (done) {
      server.get('/put-input.txt')
        .set('accept', 'text/turtle')
        .expect('Content-type', 'text/plain; charset=utf-8')
        .expect(406, done)
    })
  })
bourgeoa commented 5 months ago

There is the need to change 500 --> 406 in the following test. https://github.com/nodeSolidServer/node-solid-server/blob/83dad063955e8e42e81eb413c88710234675aa0d/test/integration/acl-oidc-test.js#L745-L753

bourgeoa commented 5 months ago

@csarven I created a branch translation can you rebase your PR. Thanks

bourgeoa commented 5 months ago

moved to https://github.com/nodeSolidServer/node-solid-server/pull/1755