Unified functions

[CxRes]

I have now written unified versions for all the "composed" functions - copy, move and remove (delete is reserved by the low level API). Please consider this PR in lieu of #136, which I am now closing.

Again, I have kept the logic absolutely unchanged, only rearranged and optimized the code, especially getting rid of redundant network calls. If a function needed modification, I have rewritten it. Despite the duplication of logic, it ensures that the old functionality is absolutely untouched.

There is just one exception. There are now moveFile() and moveFolder() methods instead of move() using the old functionality for the sake of consistency with the old API. move() itself is uses the newer logic.

I am able to run only local tests on my machine, they all seem to pass!

@jeff-zucker Again, I do not wish to rush you in any way! Have a look when you feel up to it!

[jeff-zucker]

@Otto-AA can you review this request, I am not familiar enough with that part of the code to make a proper evaluation.

[CxRes]

Hi, just checking in to see if there is some issue keeping this PR from being reviewed...

[jeff-zucker]

I have been busy with several very serious bugs relating to submitter access (#149). I need to get those cleared up first. I also have not heard anything from @bourgeoa or @Otto-AA about your PR and I am hesitant to include it before then.

And frankly, I have some concerns about this PR. It seems to me that it makes lower-level functions slightly easier to use but does not add new functionality or offer anything for the higher level methods. The "high-level methods" in solid-file-client are the ones documented in the readme - copyFile, copyFolder, moveFile, moveFolder, etc. They were chosen as non-unified because the options for them have different implications (e.g. the difference between POST and PUT for files but not folders). Regardless of whether that was a good decision, the non-unified methods are the ones available now and the ones documented and it would be messy for all upstream apps to deprecate them. So given that the non-unified methods need to remain in place, who do the unified methods work for and what do they actually bring for them? I am not vetoing the PR, but I'd need some convincing.

[CxRes]

I am a bit befuddled by your criticisms:

1. Unified copy, move, remove functions I introduce are high level methods.
2. I am not at all for removing the older functionality, precisely to maintain backward compatibility - never break user land!
3. (Convincing part):
   1. Having to deal with fewer functions in itself makes for a simpler client.
   2. The client does not have to keep track of the nature of the resource itself; the resulting simplified state lends to simpler client design.
   3. It is also more consistent with how file system and libraries work - see fs-extra for example (which I am trying to interop with). Even on a native file system you have mv, cp and rm. (rmdir explicitly requires empty directories).
   4. Behaviour of High Level functionality should be consistent with fetch/HTTP (which I fear works inconsistently). Now, one can GET with or without a trailing slash in the url on a Container (even though the returned url always has a trailing slash according to the standard). Now, I would like my functions to mimic this. Determining if the function should run based on a trailing slash, as it currently does, is bad design! I hear your concerns about PUT and POST but GET is the first operation and its behaviour must take preference.
   5. Users may receive the url out of band and thus may not be aware if the resource is a container. In this case you are forced to make an extra call to the server first to determine what is the nature of the resource. Alternatively, you let ~File() fail before you use ~Folder(). Either way its wasteful. In my design at maximum only one HEAD call is redundant if the resource is a folder only for remove. Otherwise my functions work without any extra network calls.
   6. I am using fewer network calls with my design (in some cases significantly fewer) than the current functions do (though admittedly some of that can and should be fixed in the current design).

[jeff-zucker]

They aren't criticisms. I much appreciate you offering the PR. You have a different definition of "high level" than I do. I use it to refer to the methods documented in the README which are limited to copyFile and copyFolder and do not include copy. I understand you're not removing any functionality, I'm questioning whether you add any, and for whom.

[CxRes]

@jeff-zucker I am sorry the previous comment got truncated, I was typing during a power outage. Please read the comments above in the entirety please!

[jeff-zucker]

There already are copy, move, and delete methods which work on files or folders depending on the trailing slash. What does yours add or change in terms of functionality?

[jeff-zucker]

Nevermind my last question, you have answered it already. I will give this more thought. I am tending toward including the PR, just need to get some things straight in my mind. The other issue I mentioned has implications for this too. For example - can copy be be used by someone with Append but not Write access? If so it would need to do this with POST, not PUT for files.

[jeff-zucker]

Also, in terms of a unified remove - this is essentially "rm -rf" so quite dangerous and we were thinking that giving it a name of its own could be a protection.

[CxRes]

I was in the process of writing a long answer but seeing your later replies I would just curtail it to this thought:
If a url is correct (but without a trailing slash), and one makes a request to copy etc., why should one be stopped? Just because one does not know if the resource is a Resource or a Container?

Now, after your reply, it actually might be not be a good idea to do this for remove, the reason why Solid server will not remove but an empty container. Another strategy for protection (again for consistency with filesystem methods) could be in the form of a flag that is unset by default, equivalent of -rf. My suggestion would be err closer to posix api form.

Re Append: To copy (or writing in general) to a destination you most probably will want to check write permissions on the parent anyway. So you may use that as a basis to select a PUT or POST, at least for files! The spec is unclear on the issue.
But I don't know about folders. Imho, it makes no sense to append a folder: what is the permission on the sub-folders, given you are not supposed to read inside the folder. Should you be allowed to make sub-folders and do you append them blindly then?

[jeff-zucker]

it makes no sense to append a folder

One use case is deployment. Suppose I want to create an inbox folder specific to my app within the user's inbox.

[CxRes]

it makes no sense to append a folder

One use case is deployment. Suppose I want to create an inbox folder specific to my app within the user's inbox.

I am still not so sure... So you create this folder. But you cannot read the inbox and hence not check if it is there. So the next time you return and can't remember if you created the folder (or the user deletes it), do you create the folder again?

[jeff-zucker]

Submitters can try to put something in the folder, catch the error and if it is 404 for non-existent parent, you re-create the parent. There is nothing in the spec I can see that forbids a submitter creating a container and NSS allows it.

But what about a Poster (Append+Read but no Write) - they can append the folder and read it, but if they try to copy a file, will that use POST or PUT to copy the file in your PR? If it uses PUT, it will throw an error because PUT implies ability to overwrite a file (delete the original) and therefore requires Write access.

[jeff-zucker]

I think I am being too picky. Probably I should just say that copying is for Editors, not Submitters. I am thinking of adding some other unified methods for reading and creating. So things would look like what's below. Does it make sense?

High-Level Methods by Role

• Viewers
  • read - get the contents of a file or an object describing a folder and its contents
  • head - get the header of a file or folder
  • itemExists - return true if the item is readable
  • getItemLinks - return possible location of linked files (e.g. ACL)
• Submitters
  • create - create new file or folder, make new version if already exists (POST)
• Posters
  • all of the above
• Editors
  • all of the above
  • createOrReplace - create new file or replace it if it already exists (PUT)
  • copy - copy a file or recursively copy a folder tree
  • move - move a file or recursively move a folder tree
  • delete - delete a file or recursively delete a folder tree
• Owners
  • all of the above
  • any method on an ACL (Access Conrol) file

[CxRes]

Submitters can try to put something in the folder, catch the error and if it is 404 for non-existent parent, you re-create the parent. There is nothing in the spec I can see that forbids a submitter creating a container and NSS allows it.

Doable, but it is kinda ugly!!!!

But what about a Poster...

I have no idea how to proceed, also see below...

Methods by Role looks good for now!

I think we should take the larger issue to the Solid specification people and let them suggest what should happen. It is certainly above my pay grade!

Incidentally, what other unified (read) methods are you looking at?

Also, I think we could do better than head for a high level method (and its a good idea to keep all the fetch methods as is). Something closer to a sfc.stat++, perhaps! We can leverage the fact that we are already parsing the header in various ways for our operations. Shall I start a new thread (if you like this idea, that is)!

Also a pedantic point. We should not reuse low level fetch method names for high level functions. That's why I prefer remove to delete (though deleteFile and deleteFolder are ok).

[jeff-zucker]

what other unified (read) methods are you looking at

just read() instead of readFile() plus readFolder() and create() instead of createFile() and createFolder().

I think we could do better than head for a high level method

do you mean parse the headers and return them as an object? for sure, that has been in the back of my mind for a while. Or did you mean something else?

I prefer remove to delete

yes, probably better

[CxRes]

do you mean parse the headers and return them as an object? for sure, that has been in the back of my mind for a while. Or did you mean something else?

Just to clarify my thinking not just a data object but provide methods on it as well. So you could do, just as an example, something like isFile or isDirectory. Or make the link header query-able. etc.

[CxRes]

@jeff-zucker Hey Jeff! Long time...

Could you please look at including this PR (or some version of it with appropriate changes) again. Given the progress you have made in the last six months, I need to redesign my stuff quite a bit and was hoping I could work with official binaries instead of my fork!!!!

[jeff-zucker]

@CxRes - I am afraid I am now almost completely removed from development of solid-file-client. I am swamped with work getting solid-rest and solid-node-client a) functioning within SolidOS, which they are now part of and b) functioning with the new auth scheme of ESS which is just released. @bourgeoa is the person who has been actively maintaining and expanding solid-file-client and I leave it entirely up to him how to handle your request. @bourgeoa - maybe we should look for another collaborator to help out since I won't be able to do anything with solid-file-client anytime soon.