move link functions to apiLinks.js and changes to copyFolder #95
Conversation
There was a problem hiding this comment.
Here is some feedback for those changes. The most important ones are:
- If we copy an acl file, we always have to check it's contents. They could contain absolute paths which we would need to change. This would require using solid-acl-parser, n3, or something similar.
- this.fetch is not defined in the apiLinks.js file
In general, I would appreciate it if you could run npm run lint:fix before submitting your code (or npm run lint if you want to manually check its suggestions). It makes the indentation consistent which makes it much easier to read imo. (It currently doesn't check all src files, I have to take a look how to specify that)
src/SolidApi.js
Outdated
| if (typeof from !== 'string' || typeof to !== 'string') { | ||
| throw toComposedError(new Error(`The from and to parameters of copyFile must be strings. Found: ${from} and ${to}`)) | ||
| } | ||
| // need to edit the file.acl | ||
| if (options.withAcl && (getItemName(to) !== getItemName(from))) { |
There was a problem hiding this comment.
I think we would have to check the contents of the acl file for every copyFile withAcl=true. When we take a look at an example from the spec, we see that they can also use absolute paths to specify for which file they grant access. So if we change the name and/or the path we possibly have to change the acl file.
Here's the example (from here):
# Contents of https://alice.databox.me/docs/file1.acl
@prefix acl: <http://www.w3.org/ns/auth/acl#> .
<#authorization1>
a acl:Authorization;
acl:agent <https://alice.databox.me/profile/card#me>; # Alice's WebID
acl:accessTo <https://alice.databox.me/docs/file1>;
acl:mode acl:Read,
acl:Write,
acl:Control.
src/SolidApi.js
Outdated
| throw toComposedError(new Error( `Cannot copyFile with Acl for different filenames. Found : ${getItemName(from)} and ${getItemName(to)}`)) | ||
| } | ||
| let resFile = await this._copyFile(from, to, options).catch(toComposedError) | ||
| if (resFile.ok && options.withAcl) { |
There was a problem hiding this comment.
resFile.ok is always true here. When a fetch request resolves with response.ok === false, then it will be thrown.
src/SolidApi.js
Outdated
| ...options | ||
| } | ||
| const folderResponse = await this.createFolder(to, options).catch(toComposedError) | ||
| if (folderResponse.ok && options.withAcl) { |
There was a problem hiding this comment.
folderResponse.ok is always true
| * The first one will be the folder specified by "to". | ||
| * @throws {ComposedFetchError} | ||
| */ | ||
| async _copyFolder (from, to, options) { |
There was a problem hiding this comment.
We would also need to check the acl file of copyFolder for absolute or relative paths which need to be changed, afaik.
src/SolidApi.js
Outdated
| // For folders always add to fileItems : .meta file and if options.withAcl === true also add .acl linkFile | ||
| fileItems = fileItems.concat(await this._getFolderLinks(folderUrl, options.withAcl)) | ||
| // For folders always add to fileItems : .meta file | ||
| fileItems = fileItems.concat(await getLinks(folderUrl, options.withAcl)) |
There was a problem hiding this comment.
fileItems.push(await getLinks(folderUrl, options.withAcl)) would be more concise.
src/SolidApi.js
Outdated
| // For folders always add to fileItems : .meta file and if options.withAcl === true also add .acl linkFile | ||
| fileItems = fileItems.concat(await this._getFolderLinks(folderUrl, options.withAcl)) | ||
| // For folders always add to fileItems : .meta file | ||
| fileItems = fileItems.concat(await getLinks(folderUrl, options.withAcl)) | ||
| let files = await rdf.query(folderUrl, { thisDoc: '' }, { ldp: 'contains' }) | ||
| for (var f in files) { |
There was a problem hiding this comment.
Usually when coding you don't need the var declaration. It declares the variable globally which isn't desired in most cases. You could use const here (or let if you want to modify it)
src/SolidFileClient.js
Outdated
| async getItemLinks (url) { | ||
| let links = await getItemLinks(url) | ||
| return links | ||
| } | ||
|
|
||
| // TBD array of existings links | ||
| async getLinks (url) { | ||
| let links = await getLinks(url) | ||
| return links | ||
| } |
There was a problem hiding this comment.
You don't need to await the response here, you could just return getItemLinks(url) and it has the same effect. (async methods will always return a promise. Hence if you await something and then directly return it, you could also just return it)
src/apiLinks.js
Outdated
| const { _parseLinkHeader, _urlJoin } = folderUtils | ||
| const { getParentUrl, getItemName } = apiUtils | ||
|
|
||
| class apiLinks { |
src/apiLinks.js
Outdated
| async function getItemLinks (itemUrl) { | ||
| // don't getLinks for .acl files | ||
| if (itemUrl.endsWith('.acl')) return [] | ||
| let res = await this.fetch(itemUrl, { method: 'HEAD' }) |
There was a problem hiding this comment.
this.fetch is not defined. You'd either need to pass a fetch method as parameter, or store it in a property of the class. (Due to this currently 4/7 test suits fail while setting up the test environment)
|
@Otto-AA
Restructured code :
I had a lot of difficulty to import these 2 classes and RdfQuery class. Still open : a function to check acl content for absolut/relative paths |
|
In general, I would try to keep the fetch method inside of SolidApi/SolidFileClient. This makes it easier to (1) have an overview of what is fetched and (2) handle errors appropriately. It's also useful for reducing unnecessary duplicated requests. This would mean, that the structure/workflow would be something like this: If we do it like this, it is easier to know what requests are made just from looking at SolidApi. And we need to do less requests, as we can reuse the responses sometimes (e.g. for copyFile we don't need any additional requests to find out the location of the acl files). A few other notes:
If you agree with my suggestions, I can make the changes I've outlined here and PR them to your fork (or create a new PR here), or you can take a look at it. Whatever you prefer Edit: (Just to be clear. If we remove the fetch method from linkUtils and folderUtils, then we won't need those classes anymore. Instead we could just export functions like in apiUtils) |
|
By the way, I am still not sure if we should release this feature before the change in the spec happened. From what I've seen and heard it is not unlikely that acl files will be treated differently in the future. In particular if they will still be additional files, or merged with the resource they are describing to one item. If we release it, I would set the default to (My previous comment on this with the link to the spec discussion is here) |
|
Thanks I think you can PR directly.
I shall not make any PR in the mean time.
For acl I was thinking to add one or more methods to RdfQuery and to rename
it rdfUtils because it where N3 is used.
A full acl management should be kept separate.
Le dim. 1 déc. 2019 à 23:40, A_A <notifications@github.com> a écrit :
… In general, I would try to keep the fetch method inside of
SolidApi/SolidFileClient. This makes it easier to (1) have an overview of
what is fetched and (2) handle errors appropriately. It's also useful for
reducing unnecessary duplicated requests.
This would mean, that the structure/workflow would be something like this:
SolidApi.readFolder
-> fetches turtle
-> calls folderUtils.processFolder(turtle)
-> returns processedFolder
SolidApi.copyFile
-> fetches file
-> writes file
-> if (withAcl)
---> aclFrom = linkUtils.getAclUrlFromResponse(getResponse)
---> aclTo = linkUtils.getAclUrlFromResponse(putResponse)
---> calls SolidApi.copyAclFile(aclFrom, aclTo)
SolidApi.copyFolder
-> read folder
-> create destination folder
-> if (withAcl)
---> aclFrom = linkUtils.getAclUrlFromResponse(getResponse) // probably additional req
---> aclTo = linkUtils.getAclUrlFromResponse(postResponse)
---> calls SolidApi.copyAclFile(aclFrom, aclTo)
-> copy contents...
SolidApi.copyAclFile(from, to)
-> fetch turtle
-> update paths
-> put turtle
If we do it like this, it is easier to know what requests are made just
from looking at SolidApi. And we need to do less requests, as we can reuse
the responses sometimes (e.g. for copyFile we don't need any additional
requests to find out the location of the acl files).
A few other notes:
- copyFolder and copyFile should resolve with responses. I'm not sure
if we should include acl responses there, that's something we should discuss
- the this in getItemLinks actually targets the SolidApi instance and
not the LinksUtils one, which is pretty confusing imo. It would be more
clear if the fetch is passed as parameter in the constructor and then this
one is used (ie in SolidApi we would need this.getLinks =
link.getLinks.bind(link) for that). But if we do the changes I've
suggested above it doesn't matter
- I've worked a bit on solid-acl-parser and it's quite stable now. If
we want to add more support for working with acl files we could use it or
solid-acl-utils. If we only want to make copying acl files work I can write
a method for that instead of including the whole library.
If you agree with my suggestions, I can make the changes I've outlined
here and PR them to your fork (or create a new PR here), or you can take a
look at it. Whatever you prefer
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#95?email_source=notifications&email_token=AAQ5TZUJR4GRG2E2FGC4PNLQWQ4N3A5CNFSM4JTB5MMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFRX6YA#issuecomment-560168800>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQ5TZUPW7TBB7E5KXPE7IDQWQ4N3ANCNFSM4JTB5MMA>
.
|
|
Before starting to work in it, I'd like to discuss if we even want it in this release, or if we want to wait for the spec to be updated. The related issue is in the 19th December milestone, so maybe it will be discussed soon, but I'd actually guess it will take longer until it is clear. But even if it is only clear in two or three months, I would still consider waiting with the acl support until then. |
|
@jeff-zucker @Otto-AA To my understanding the new spec whenever ready and implemented will take time mid next year (?) maybe. with ACL and your solid-acl-parser are big steps in both understanding and practising in the solid environment. So I am very much in favour to implement withAcl with some ACL management :
I wonder if something is needed to check that acl control is not lost. A kind of report |
|
Re modifying .acl files on the fly : I am quite against modifying files
that we copy or move - no other copy or move function I'm aware of changes
the contents of the files. It should be the user's responsibility to not
use absolute paths or to be aware of how the absolute paths will work after
the copy or move. We can document that.
…On Sat, Nov 30, 2019 at 1:37 AM A_A ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Here is some feedback for those changes. The most important ones are:
- If we copy an acl file, we always have to check it's contents. They
could contain absolute paths which we would need to change. This would
require using solid-acl-parser, n3, or something similar.
- this.fetch is not defined in the apiLinks.js file
In general, I would appreciate it if you could run npm run lint:fix
before submitting your code (or npm run lint if you want to manually
check its suggestions). It makes the indentation consistent which makes it
much easier to read imo. (It currently doesn't check all src files, I have
to take a look how to specify that)
------------------------------
In src/SolidApi.js
<#95 (comment)>
:
> if (typeof from !== 'string' || typeof to !== 'string') {
throw toComposedError(new Error(`The from and to parameters of copyFile must be strings. Found: ${from} and ${to}`))
}
+ // need to edit the file.acl
+ if (options.withAcl && (getItemName(to) !== getItemName(from))) {
I think we would have to check the contents of the acl file for every
copyFile withAcl=true. When we take a look at an example from the spec, we
see that they can also use absolute paths to specify for which file they
grant access. So if we change the name and/or the path we possibly have to
change the acl file.
Here's the example (from here
<http://solid.github.io/web-access-control-spec/#example-wac-document>):
# Contents of https://alice.databox.me/docs/file1.acl
@Prefix acl: <http://www.w3.org/ns/auth/acl#> .
<#authorization1>
a acl:Authorization;
acl:agent <https://alice.databox.me/profile/card#me>; # Alice's WebID
acl:accessTo <https://alice.databox.me/docs/file1>;
acl:mode acl:Read,
acl:Write,
acl:Control.
------------------------------
In src/SolidApi.js
<#95 (comment)>
:
> if (typeof from !== 'string' || typeof to !== 'string') {
throw toComposedError(new Error(`The from and to parameters of copyFile must be strings. Found: ${from} and ${to}`))
}
+ // need to edit the file.acl
+ if (options.withAcl && (getItemName(to) !== getItemName(from))) {
+ throw toComposedError(new Error( `Cannot copyFile with Acl for different filenames. Found : ${getItemName(from)} and ${getItemName(to)}`))
+ }
+ let resFile = await this._copyFile(from, to, options).catch(toComposedError)
+ if (resFile.ok && options.withAcl) {
resFile.ok is always true here. When a fetch request resolves with response.ok
=== false, then it will be thrown.
------------------------------
In src/SolidApi.js
<#95 (comment)>
:
> + * Overwrites files per default.
+ * Merges folders if already existing
+ * @param {string} from
+ * @param {string} to
+ * @param {WriteOptions} [options]
+ * @returns {Promise<Response[]>} Resolves with an array of creation responses.
+ * The first one will be the folder specified by "to".
+ * @throws {ComposedFetchError}
+ */
+ async _copyFolder (from, to, options) {
+ options = {
+ ...defaultWriteOptions,
+ ...options
+ }
+ const folderResponse = await this.createFolder(to, options).catch(toComposedError)
+ if (folderResponse.ok && options.withAcl) {
folderResponse.ok is always true
------------------------------
In src/SolidApi.js
<#95 (comment)>
:
> @@ -397,6 +424,32 @@ class SolidAPI {
return [folderResponse].concat(...creationResults) // Alternative to Array.prototype.flat
}
+ /**
+ * non recursive copy of a folder with .acl
+ * Overwrites files per default.
+ * Merges folders if already existing
+ * @param {string} from
+ * @param {string} to
+ * @param {WriteOptions} [options]
+ * @returns {Promise<Response[]>} Resolves with an array of creation responses.
+ * The first one will be the folder specified by "to".
+ * @throws {ComposedFetchError}
+ */
+ async _copyFolder (from, to, options) {
We would also need to check the acl file of copyFolder for absolute or
relative paths which need to be changed, afaik.
------------------------------
In src/SolidApi.js
<#95 (comment)>
:
> let [rdf, folder, folderItems, fileItems] = [this.rdf, [], [], []] // eslint-disable-line no-unused-vars
- // For folders always add to fileItems : .meta file and if options.withAcl === true also add .acl linkFile
- fileItems = fileItems.concat(await this._getFolderLinks(folderUrl, options.withAcl))
+ // For folders always add to fileItems : .meta file
+ fileItems = fileItems.concat(await getLinks(folderUrl, options.withAcl))
fileItems.push(await getLinks(folderUrl, options.withAcl)) would be more
concise.
------------------------------
In src/SolidApi.js
<#95 (comment)>
:
> let [rdf, folder, folderItems, fileItems] = [this.rdf, [], [], []] // eslint-disable-line no-unused-vars
- // For folders always add to fileItems : .meta file and if options.withAcl === true also add .acl linkFile
- fileItems = fileItems.concat(await this._getFolderLinks(folderUrl, options.withAcl))
+ // For folders always add to fileItems : .meta file
+ fileItems = fileItems.concat(await getLinks(folderUrl, options.withAcl))
let files = await rdf.query(folderUrl, { thisDoc: '' }, { ldp: 'contains' })
for (var f in files) {
Usually when coding you don't need the var declaration. It declares the
variable globally which isn't desired in most cases. You could use const
here (or let if you want to modify it)
------------------------------
In src/SolidFileClient.js
<#95 (comment)>
:
> + async getItemLinks (url) {
+ let links = await getItemLinks(url)
+ return links
+ }
+
+ // TBD array of existings links
+ async getLinks (url) {
+ let links = await getLinks(url)
+ return links
+ }
You don't need to await the response here, you could just return
getItemLinks(url) and it has the same effect. (async methods will always
return a promise. Hence if you await something and then directly return
it, you could also just return it)
------------------------------
In src/apiLinks.js
<#95 (comment)>
:
> @@ -0,0 +1,137 @@
+import folderUtils from './utils/folderUtils'
+import apiUtils from './utils/apiUtils'
+
+const { _parseLinkHeader, _urlJoin } = folderUtils
+const { getParentUrl, getItemName } = apiUtils
+
+class apiLinks {
This class is unused?
------------------------------
In src/apiLinks.js
<#95 (comment)>
:
> + }
+ return itemLinks
+ }
+
+ /**
+ * @Private
+ * getItemLinks (TBD)
+ * return allways an object of linkUrls
+ * - object.acl for folder and files
+ * - object.meta for folder
+ *
+ */
+ async function getItemLinks (itemUrl) {
+ // don't getLinks for .acl files
+ if (itemUrl.endsWith('.acl')) return []
+ let res = await this.fetch(itemUrl, { method: 'HEAD' })
this.fetch is not defined. You'd either need to pass a fetch method as
parameter, or store it in a property of the class. (Due to this currently
4/7 test suits fail while setting up the test environment)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#95?email_source=notifications&email_token=AKVJCJHE6LTM5KD3BVIKIQLQWIX4XA5CNFSM4JTB5MMKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNOVGCA#pullrequestreview-324883208>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKVJCJAWO3ZOTW5VXWYKAQLQWIX4XANCNFSM4JTB5MMA>
.
|
|
I'm afraid I am lost as to what "this issue" refers to. Are you speaking
of .acl handling in general? .acl re-writing ? or something else? Even
your pointer to a previous discussion refers to "this issue". Which one?
:-)
…On Sun, Dec 1, 2019 at 11:12 PM A_A ***@***.***> wrote:
By the way, I am still not sure if we should release this feature before
the change in the spec happened. From what I've seen and heard it is not
unlikely that acl files will be treated differently in the future. In
particular if they will still be additional files, or merged with the
resource they are describing to one item.
If we release it, I would set the default to withLinks: false and add a
note that this has a rather high probability of changing. The spec update
could require breaking changes to our library again, so then a 2.0.0
version would be plausible. If we leave it for now, we could add the
feature later without breaking changes, so it would be a 1.1.0 release.
(My previous comment on this with the link to the spec discussion is here
<#49 (comment)>
)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#95?email_source=notifications&email_token=AKVJCJBJMNROROFJISMC5CLQWSYOLA5CNFSM4JTB5MMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFSPGEA#issuecomment-560263952>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKVJCJH7WMYQIJABWXWQEH3QWSYOLANCNFSM4JTB5MMA>
.
|
|
That's the issue I'm talking about: solid/specification#58 |
The problem I see with not changing the content is following: If the acl file contains absolute paths, copying it to another location can make all rules inside it invalid/unused/arbitrary (the spec is a bit unclear which of these is the case imo). We could end up with a file without any (valid) control permissions defined, or a folder where all defaults are gone. While we copied the exact files in this scenario, we probably didn't copy the meaning of the files. To sum my perspective up: Not changing the content can lead to a different behavior. Changing the content can lead to the same behavior. Both are not intuitive to the developer using our library.
I wouldn't expect our users to be aware of this or even able to use it properly. So instead of not modifying the acl and documenting that the user should be aware of relative paths, I would advocate for modifying the acl and documenting that absolute paths will be changed. We could still provide a flag for those who are certain about making exact copies. Also I'd suggest to use the flag name as a documentation itself (e.g. |
If the spec needs that long to be updated, I'd agree to include some kind of acl copying. But I would set the default to false and add a warning to the documentation that this probably changes soon. |
|
Apologies if I'm rehashing things you've already discussed, here are my current thoughts:
If we always return all files including *.meta and *.acl, we need to open each and every file to find their location. Could be a lot of overhead for large folders. Should we do that or should the user just invoke getLinks() themselves if they want to know those?
It looks like the spec is heading towards treating the linked resources (including shape as well as metadata and access) as equivalent to the original resource in terms of deleting - if we delete a file, its .meta, .acl and other links are also deleted. And copy should work the same way. So I would see this as the default behavior. I am comfortable (but could be argued out of) leaving all else to the user - they can either edit out their own absolute links using Otto's libraries or opt-out of copying/moving .acl resources. If you both think editing the .acl is the only way, I'll go along. |
|
So, by default, copyFile(x.y,destination) would copy x,y, x.y.meta, and
x.y.acl, modifying the .acl if necessary. And if the user supplied a flag,
they could get the same thing but without the modification? That works for
me. I'd suggest a flag something like unModifiedACL : true, with default
false.
…On Wed, Dec 4, 2019 at 1:37 PM A_A ***@***.***> wrote:
Re modifying .acl files on the fly : I am quite against modifying files
that we copy or move - no other copy or move function I'm aware of changes
the contents of the files. It should be the user's responsibility to not
use absolute paths or to be aware of how the absolute paths will work after
the copy or move. We can document that.
The problem I see with not changing the content is following: If the acl
file contains absolute paths, copying it to another location can make all
rules inside it invalid/unused/arbitrary (the spec is a bit unclear which
of these is the case imo). We could end up with a file without any (valid)
control permissions defined, or a folder where all defaults are gone. While
we copied the exact files in this scenario, we probably didn't copy the
meaning of the files.
To sum my perspective up: Not changing the content can lead to a different
behavior. Changing the content can lead to the same behavior. Both are not
intuitive to the developer using our library.
It should be the user's responsibility to not
use absolute paths or to be aware of how the absolute paths will work after
the copy or move. We can document that.
I wouldn't expect our users to be aware of this or even able to use it
properly.
Firstly, understanding how acl files work seems more low-level than using
a high-level method like copyFolder(a, b). I think we should try to
target people who barely even know about acl.
Secondly, even if they know about this, assuring that the acl files of all
files in a folder don't include absolute paths is too big to do in
practice. One would need to iterate over all files, fetch the acl files for
them and then check them for absolute paths. So not changing the contents
limits the use cases of copyFolder with acl files very much (I guess it
would be okay to use for folders which were just created by oneself knowing
that the acl-creating library uses relative paths. Apart from that it would
be just hoping that it works).
So instead of not modifying the acl and documenting that the user should
be aware of relative paths, I would advocate for modifying the acl and
documenting that absolute paths will be changed. We could still provide a
flag for those who are certain about making exact copies. Also I'd suggest
to use the flag name as a documentation itself (e.g. copyFolder(a, b, {
addSamePermissions: true }) instead of "withAcl"). Making the default
false would make it likelier that those who use it know what they doing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#95?email_source=notifications&email_token=AKVJCJCSOOGXW4RIHRV336DQXAPJJA5CNFSM4JTB5MMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF6SPWA#issuecomment-561850328>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKVJCJARGO3OL5T4WWRPHV3QXAPJJANCNFSM4JTB5MMA>
.
|
If we want to support acl files, I think this is the way we should go. As argued above, I don't think users can be capable of making the check for absolute paths in practice. And while it modifies the contents it keeps the meaning (in contrast to keeping the contents and modifying the meaning).
Yes, as far as I've seen they go to a atomic view on this (one atom consists of several pieces which strictly belong together). If we want copy to work the same way, we need to make sure that after copying they still are one unit, ie the acl points to the copied resource. So either it has relative paths from the start, or we modify it to point to the same resource. |
In principle yes. But I would suggest to only copy the file as a default, and copy acl only with the flag: copyFile(from, to) // Copies only the file
copyFile(from, to, { addSamePermissions: true }) // Copies file + modified acl file
copyFile(from, to, { withUnmodifiedAcl: true }) // Copies file + unmodified acl fileI currently don't have an opinion about .meta as I don't really know how it is used. |
|
Why default to not copying the permissions? A naive user would expect them
to be copied.
And the same for all other linked files including .meta and any others
(shapes too). We should parse all of the rel= links and treat any files
listed there as atomically connected to the original resource. This
supports future modifications of the spec in which other kinds of links may
be introduced. Metadata files for containers are fully supported by NSS
and we can not ignore them.
…On Wed, Dec 4, 2019 at 2:41 PM A_A ***@***.***> wrote:
So, by default, copyFile(x.y,destination) would copy x,y, x.y.meta, and
x.y.acl, modifying the .acl if necessary. And if the user supplied a flag,
they could get the same thing but without the modification? That works for
me. I'd suggest a flag something like unModifiedACL : true, with default
false.
In principle yes. But I would suggest to only copy the file as a default,
and copy acl only with the flag:
copyFile(from, to) // Copies only the filecopyFile(from, to, { addSamePermissions: true }) // Copies file + modified acl filecopyFile(from, to, { withUnmodifiedAcl: true }) // Copies file + unmodified acl file
I currently don't have an opinion about .meta as I don't really know how
it is used.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#95?email_source=notifications&email_token=AKVJCJCWWQO2ECVY6LXCYMDQXAW2NA5CNFSM4JTB5MMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF6YCQY#issuecomment-561873219>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKVJCJDHYXUYSJIUUANEEKDQXAW2NANCNFSM4JTB5MMA>
.
|
I think this depends on the use case. If I copy stuff from the private folder to the public folder I probably expect it to be public afterwards. I think that in many use cases the expectation is to copy the contents only, not the acl permissions. But there are also other cases where the expectation is to copy both. And I honestly don't know which of these will happen more often in practice. I personally wouldn't think that it copies permissions judging just from the name, that's why I suggested the default to not copying permissions. If we want to reflect the atomic model which will probably be adopted by the Solid spec, this would tend to setting the default to copy permissions too. This would support your view on the defaults. As far as I see the damage for mistakenly choosing the wrong flag isn't that big in both scenarios (if we don't make an exact copy of the acl file). So we can ignore this for this decision. |
I don't think this is a good idea, because (1) the server may add link headers which are not in the spec (I don't think the spec forbids this, I haven't check this though) and (2) future spec changes could also add link relations to external files which shouldn't be copied (from MDN: "The HTML External Resource Link element () specifies relationships between the current document and an external resource."). For example I could imagine a header like "preferred-app=solid-ide" to be set (not the best example, but I think you get the point). |
|
Okay, for now I think we should include .meta and .acl as atomically
linked. If/when a server implements shapes or some other Solid spec
approved relationship, we can add it.
…On Wed, Dec 4, 2019 at 3:22 PM A_A ***@***.***> wrote:
We should parse all of the rel= links and treat any files
listed there as atomically connected to the original resource. This
supports future modifications of the spec in which other kinds of links may
be introduced. Metadata files for containers are fully supported by NSS
and we can not ignore them.
I don't think this is a good idea, because (1) the server may add link
headers which are not in the spec (I don't think the spec forbids this, I
haven't check this though) and (2) future spec changes could also add link
relations to external files which shouldn't be copied (from MDN
<https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link>: *"The
HTML External Resource Link element () specifies relationships between the
current document and an external resource."*). For example I could
imagine a header like "preferred-app=solid-ide" to be set (not the best
example, but I think you get the point).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#95?email_source=notifications&email_token=AKVJCJECTJ7PXKQ3FFLH5IDQXA3SVA5CNFSM4JTB5MMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF63QMI#issuecomment-561887281>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKVJCJHYIGNRST7WWNRHVG3QXA3SVANCNFSM4JTB5MMA>
.
|
|
added when needed a simple |
|
@jeff-zucker checked with travis, lint on scr/* and on solid-ide for copyFile and copyFolder and ..... comments taken from @Otto-AA PR #99
|
@jeff-zucker @Otto-AA
To be able to copy from on pod to a different on having maybe a different policy for links, I made a few changes and added some cleanup :
to clean structure moved link functions from
SolidApitoapiLinksand withAcl: true
getItemLinksto findfile.aclfortoincopyFileandcopyFoldergetItemLinksandgetLinksas ne methods forSolidFileClientlimited to same from to fileName
copyFolderreplacecreateFolderby a new function _copyFolder to create a folder and copy the .aclthe logic for copyFolder is now :
readFolder(from, to, { withAcl: false }:.metais allways added tofilesand treated as a file (copied with or without acl)options.withAcl:the logic for readFolder as not been changed :
Thoroughly tested copy file and folders on solid-ide.