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

PUT Requests storing empty JSON object #1165

Open
boulderwebdev opened this issue Apr 4, 2019 · 12 comments

Comments

Projects
None yet
5 participants
@boulderwebdev
Copy link

commented Apr 4, 2019

I am trying to update text files on my pod using a PUT request. If I have a text file, e.g. /public/hello.txt and make a PUT request to override its value, the file is replaced by a text file with an empty JSON object.

Here's a snipped of the code I used to perform the PUT request. Here we are assuming /public/hello.txt exists already.

url = "https://boulderwebdev.solid.community/public/hello.txt"
solid.fetch(url, {
  method: "PUT",
  headers: {
    "Content-Type": "text/plain"
  },
  body: JSON.stringify({"hello": "world"})
}).then(resp => {...})

If you go to https://boulderwebdev.solid.community/public/hello.txt now you will see a page with the text

{}
@jaxoncreed

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

There was a similar issue recently at #1011. It's caused by the auth system using body-parser while the ldp expects a stream. To rectify this, we added a temporary hack that creates a new stream from the parsed body, but this hack only applies to application/json not text/json and only in a POST request.

https://github.com/solid/node-solid-server/blob/master/lib/ldp.js#L167

If you would like, you can open a pull request that extends this hack to apply to your use case.

@boulderwebdev

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

Oh, whoops, that should be text/plain. Thanks for the reference, I'll check it out soon.

@Otto-AA

This comment has been minimized.

Copy link

commented Apr 23, 2019

I've tested it a bit and from what I've seen, v4.x (solid.community) works if slug is set to "filename.json" and the Content-Type is unset, and v5.0.0 works if slug is set to "filename" and Content-Type to "application/json".
Both don't work the other way round (v4.x doesn't work with application/json and v5.0.0 requires application/json), so I haven't figured out how I should handle it to support both versions. Is there a way to do this?

And another question (I can move it to another issue if you want): Should the server accept an undefined/unset Content-Type? As far as I've seen it works perfectly to add the extension to the slug (slug: test.css) and don't set the Content-Type for v4.x and (except for json files) also for v5.0.0.
Is this behavior intended, and if yes, why doesn't this work for slug: test.json in v5.0.0?

@jaxoncreed jaxoncreed added the triage label Apr 23, 2019

@jaxoncreed

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

No, it's not intended. This behaviour is the result of a hack located here: https://github.com/solid/node-solid-server/blob/master/lib/ldp.js#L159. We will work on fixing this, but pull requests are appreciated if you'd like to take a crack at it 😃

@Otto-AA

This comment has been minimized.

Copy link

commented Apr 23, 2019

Do you mean, that using slug: filename.ext but leaving Content-Type unset is something, that should work? I myself didn't find any documentation on this, so I would be glad if you know more about this :)
(And I'll likely don't have time to get familiar with this project in the near future, sorry)

@jaxoncreed

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Maybe @michielbdejong can correct me if I'm wrong, but I believe that the file extension supersedes the content type. So if I were to submit file.txt with content-type: application/json it should still be saved in the system as file.txt and not file.json

@kjetilk

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Maybe @michielbdejong can correct me if I'm wrong, but I believe that the file extension supersedes the content type. So if I were to submit file.txt with content-type: application/json it should still be saved in the system as file.txt and not file.json

Hmmm, in that case, I'd have to hold my nose... But perhaps there is a history behind it that I'm not aware of?

@Otto-AA

This comment has been minimized.

Copy link

commented Apr 23, 2019

I've taken a look at the source code: slug and Content-Type are treated equally. The Content-Type header gets converted to a file extension and then they are concatenated to the path:

getAvailablePath (host, containerURI, { slug = uuid.v1(), extension }) {
const path = slug + extension

So for instance POST slug: test.txt Content-Type: text/css would be stored as test.txt.css.

But what I initially wanted to know is, if this behavior should be the same for all solid pod implementations, or in other words, if it is defined in some spec from solid or ldp. Because I would like to stick to the specification as much as possible to make sure it works with other server implementations too.

@kjetilk

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Well, it goes back to Web Architecture

@timbl

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Maybe @michielbdejong can correct me if I'm wrong, but I believe that the file extension supersedes the content type. So if I were to submit file.txt with content-type: application/json it should still be saved in the system as file.txt and not file.json

No absolutely not. A PUT without a content-type is an invalid request.

@kjetilk

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Right, so it indeed it seems like it is bypassing the ResourceMapper, which should follow this design issue

@jaxoncreed

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Okay thanks for the clarification!

@jaxoncreed jaxoncreed self-assigned this Apr 24, 2019

@jaxoncreed jaxoncreed removed the triage label Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.