[megoth]

This is a fix for #918

I have not written tests for this, but plan to do that ASAP. But would be nice to have some eyes on the code in case I've missed something.

[megoth]

A problem with this implementation is that it returns quite a lot of 404-statuses, to which there are two problems:

1. Sometimes it is actually referring to a missing value, e.g. serverSide.ttl doesn't have triple for solid:storageQuota, and
2. It may look a bit weird to get a 404 when serverSide.ttl doesn't exist; it might be by design, since POD providers don't necessarily enforce a quota on their PODs

Any thoughts on this? My thinking was that it's ok to return 404, since the result for the consuming client code is that it's just ignored. But it might be annoying for developers checking console for various reasons to see a 404 being returned (especially annoying if they're trying to debug and is side-railed).

[kjetilk]

It may look a bit weird to get a 404 when serverSide.ttl doesn't exist; it might be by design, since POD providers don't necessarily enforce a quota on their PODs

Oh, yeah, that is by design and documented as the way to disable the quota system. So, basically, parsing the file takes around ~200 ms, and then computing the size of the file system takes ~100 ms, so the idea was that you can avoid this cost by not having the serverSide.ttl file, then the cost is just that of a stat on the file.

But then, I might not quite understand what will return a 404, is it just the get-account-info-request resource?

[megoth]

But then, I might not quite understand what will return a 404, is it just the get-account-info-request resource?

As the code is written now, there are various things that could go wrong:

1. Line 46: User does not have a userId (i.e. an unauthenticated visitor) --> Return 401
2. Line 57: User does not have access to the account info for the POD (i.e. an authenticated visitor) --> Return whatever acl-check returns
3. Line 66: There was an error when dereferencing the graph from user's WebID --> Return the error
4. Line 74: There wasn't a triple denoting where storage is located --> Return 404
5. Line 84: There was an error when dereferencing the graph for serverSide.ttl --> Return the error
6. Line 92: There wasn't a triple denoting the quota -> Return 404
7. Line 107: There was an error when serializing the graph that represents the content of /account --> Return the error (probably 500)

The missing triples might be a stretch to return 404s, but my thinking was that there is some data missing, hence we don't have an account resource to return. But thinking more of it, we could also return 200 and a graph that doesn't have those triples (i.e. an empty graph for now).

We could apply the same strategy when we cannot dereference the various graphs (WebID, serverSide.ttl).

[kjetilk]

But then, I might not quite understand what will return a 404, is it just the get-account-info-request resource?

As the code is written now, there are various things that could go wrong:

OK, I just needed to clear out a little misunderstanding on my part, I thought you were talking about the resource the user was requesting when they got an over-quota error, not the get-account-info-request resource. Then things start to make more sense.

1. [Line 46](https://github.com/solid/node-solid-server/pull/1060/files#diff-0c7366a5df447f793ba324d58a9a7731R46): User does not have a userId (i.e. an unauthenticated visitor) --> Return 401

👍

2. [Line 57](https://github.com/solid/node-solid-server/pull/1060/files#diff-0c7366a5df447f793ba324d58a9a7731R57): User does not have access to the account info for the POD (i.e. an authenticated visitor) --> Return whatever acl-check returns

👍

3. [Line 66](https://github.com/solid/node-solid-server/pull/1060/files#diff-0c7366a5df447f793ba324d58a9a7731R66): There was an error when dereferencing the graph from user's WebID --> Return the error

👍

4. [Line 74](https://github.com/solid/node-solid-server/pull/1060/files#diff-0c7366a5df447f793ba324d58a9a7731R74): There wasn't a triple denoting where storage is located --> Return 404

Right. Indeed an error. Actually, I wonder if that should result in a 500, you could argue that a WebID without an associated storage is a server misconfig... OTOH, it is a SHOULD for that link. Still, I think it makes sense to make that a server side error since it is no fault of the user. We still need to tell the user why, that no storage has been configured.

5. [Line 84](https://github.com/solid/node-solid-server/pull/1060/files#diff-0c7366a5df447f793ba324d58a9a7731R84): There was an error when dereferencing the graph for serverSide.ttl --> Return the error

👍 It does indeed make sense for a 404 for that resource.

6. [Line 92](https://github.com/solid/node-solid-server/pull/1060/files#diff-0c7366a5df447f793ba324d58a9a7731R92): There wasn't a triple denoting the quota -> Return 404

👍

7. [Line 107](https://github.com/solid/node-solid-server/pull/1060/files#diff-0c7366a5df447f793ba324d58a9a7731R107): There was an error when serializing the graph that represents the content of `/account` --> Return the error (probably 500)

👍

The missing triples might be a stretch to return 404s, but my thinking was that there is some data missing, hence we don't have an account resource to return. But thinking more of it, we could also return 200 and a graph that doesn't have those triples (i.e. an empty graph for now).

Yeah, I think it makes sense, except for the server side error for pim:storage, but it is interesting to get more opinions on that.

We could apply the same strategy when we cannot dereference the various graphs (WebID, serverSide.ttl).

[rubensworks]

I'm just wondering what happens if the quota system is disabled, will the user also get a 404 on /account?
I assume that we want all quota-related things to be hidden in that case?

[megoth]

I'm just wondering what happens if the quota system is disabled, will the user also get a 404 on /account?
I assume that we want all quota-related things to be hidden in that case?

If the quota system is disabled, i.e. serverSide.ttl does not have a triple with solid:storageQuota, or serverSide.ttl does not exist at all, it will return a 404.

Are there other indicators that a quota-system is not in use?

[megoth]

I'm unsure of the caching-thing myself, as @rubensworks mentions. It could be an annoyance for people uploading a lot of files in a short timespan, and not see the updated info.

Right now I've cached the data for one hour, but we could make it less, like a minute. But then there might not be the big need for this cache anyway.

Another way of doing this could be to have the cache as a service that size-altering commands (ie POST, PUT, PATCH, DELETE) could call whenever something is changed on the server; it could simply remove the cached value for the userId, which would regenerate the info on next call. I'm worried that this is a bit overcomplicating it though =/

[rubensworks]

premature optimization is the root of all evil

If we're not sure about the performance impact, I would just remove the cache, and look for solutions if we experience a performance issue.

[megoth]

premature optimization is the root of all evil

If we're not sure about the performance impact, I would just remove the cache, and look for solutions if we experience a performance issue.

I agree wholeheartedly to this; I'll remove the caching mechanism for now.

[megoth]

4. [Line 74](https://github.com/solid/node-solid-server/pull/1060/files#diff-0c7366a5df447f793ba324d58a9a7731R74): There wasn't a triple denoting where storage is located --> Return 404

Right. Indeed an error. Actually, I wonder if that should result in a 500, you could argue that a WebID without an associated storage is a server misconfig... OTOH, it is a SHOULD for that link. Still, I think it makes sense to make that a server side error since it is no fault of the user. We still need to tell the user why, that no storage has been configured.

I agree it is very strange, but as the wording is SHOULD, we have to expect cases where people have removed pim:storage from their WebID (maybe it's they don't want anyone to know where their storage is).

One of the problems is also whether or not we should return a 200 or 404 (or maybe 500 in some cases). The problem is that we have no way of knowing whether the server enforces a quota-system, expect to infer whether serverSide.ttl and solid:storageQuota is present or not. If that was intentional, return 200 with an empty graph, if it was not, return 404.

Maybe we should have it in config.json whether or not POD providers want to enforce a quota-system?

[megoth]

This could be moved as a method to ldp.js instead, something like createACL(resource, req) if that's a better abstraction? @kjetilk @rubensworks

[rubensworks]

I think it's fine to keep it here, but not strong opinion.

[kjetilk]

4. [Line 74](https://github.com/solid/node-solid-server/pull/1060/files#diff-0c7366a5df447f793ba324d58a9a7731R74): There wasn't a triple denoting where storage is located --> Return 404

Right. Indeed an error. Actually, I wonder if that should result in a 500, you could argue that a WebID without an associated storage is a server misconfig... OTOH, it is a SHOULD for that link. Still, I think it makes sense to make that a server side error since it is no fault of the user. We still need to tell the user why, that no storage has been configured.

I agree it is very strange, but as the wording is SHOULD, we have to expect cases where people have removed pim:storage from their WebID (maybe it's they don't want anyone to know where their storage is).

Right, but in that case, we either have to require pim:storage to be present if quota system is in use, or not rely on it. We have to make sure that people can't evade the quota system just by removing their pim:storage relation. Perhaps we should set it in serverSide.ttl?

One of the problems is also whether or not we should return a 200 or 404 (or maybe 500 in some cases). The problem is that we have no way of knowing whether the server enforces a quota-system, expect to infer whether serverSide.ttl and solid:storageQuota is present or not. If that was intentional, return 200 with an empty graph, if it was not, return 404.

Maybe we should have it in config.json whether or not POD providers want to enforce a quota-system?

Yeah, we could... I just didn't want to spend so much time on this stop-gap measure, but I suppose we might have to.

Regarding caching, Tim was (rightly) worried about the write performance, and caching in RAM should help, but indeed, just a max-age wouldn't do the trick, the cache would need to be invalidated on write. More work, and it isn't strictly necessary for small Pods, so I can live with not having it in.

[megoth]

One of the problems is also whether or not we should return a 200 or 404 (or maybe 500 in some cases). The problem is that we have no way of knowing whether the server enforces a quota-system, expect to infer whether serverSide.ttl and solid:storageQuota is present or not. If that was intentional, return 200 with an empty graph, if it was not, return 404.
Maybe we should have it in config.json whether or not POD providers want to enforce a quota-system?

Yeah, we could... I just didn't want to spend so much time on this stop-gap measure, but I suppose we might have to.

Adding an option to the init-script is an easy and quick thing to do. Not necessarily smart though, so am open to being convinced to not do it. I'll add it as another branch though, so that we can discuss it separately from this issue.

(Just to add what I'm thinking of doing: I'll add an option called "enforceQuota" or something, that is only prompted if the user is setting up a multiuser-server, and set it to false by default. I guess we should expand upon it as well, i.e. copy serverSide.ttl as serverSide.ttl.inactive or serverSide.ttl. I should have something in place by the end of day latest.)

[megoth]

I'll close this until we have enforceQuota in place. I'll reopen the PR then.