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

add options to load() to resolve recursiveDelete() issue #12

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bourgeoa
Copy link
Contributor

  1. this.store.fetcher.load(doc) with no options reuse the existing document in store.
    This is not what we expect. We want load a new document.
    options.force = true reload the document but adds new triples
    we neee to remove the document from store in that case

  2. force = true is used in aclDocUrl() and getContainerMembers().
    @michielbdejong Should it be a default for load() ?

  3. solid spec implies that delete is atomic (delete ressource and auxiliary one in the same operation)
    I remove the acl delete in recursiveDelete()

Copy link

@edwardsph edwardsph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/index.ts Outdated
return this.store.fetcher.load(doc)
// force = true to reload the doc
// but first remove doc: this is to avoid just adding quads
if (options && options.force) this.store.removeDocument(doc as NamedNode)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite new to Typescript so may have misunderstood but if someone passed a NamedNode array or a string to load() will it work to pass that to removeDocument as a NamedNode?

Copy link
Contributor Author

@bourgeoa bourgeoa Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it will work if you don't use force = true. If you use force = true doc must be a Named node.
I took this solution because it was used elsewhere in the code, it was simple, it was compatible with the used cases and offer a plus possibility to load(). In an other way there is no regression

If you don't use force = true then load can receive NamedNode, NamedNode Array or String like before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edwardsph I made the changes. I don't know anything in typescript. My initial idea was wrong. Load now can pass a NamedNode, NamedNode Array or a string to load() and pass that to removeDocument() as namedNode.

Tested locally with all 3 types.

Copy link

@edwardsph edwardsph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@timbl
Copy link
Contributor

timbl commented Feb 4, 2021

"his.store.fetcher.load(doc) with no options reuse the existing document in store.
This is not what we expect. We want load a new document." -
No no no no no. The semantics of load is "loadIfNotAlreadyLoaded" That is really important. It is the porose version of a function "nowOrWhenFetched" which would call back immediately oif the thing had already been loaded into the store. The caching of documents in te quad store makes everything VERY MUCH faster. And the solid-ui code calls x.load() many times just to make sure it is loaded before following links from x. It is essentilal that x.load returns immediately if tehe document is already noted. We clearly have to make this clear in the documentation

@timbl
Copy link
Contributor

timbl commented Feb 4, 2021

If you want to reload something already loaded you have refresh https://github.com/linkeddata/rdflib.js/blob/master/src/fetcher.ts#L1732
and refershIfExpired which will check the cache against the expiry date on the resource.

There is also fetcher.unload. Do not use store.removeDocument as it will leave the cache metadata wrong.

@bourgeoa
Copy link
Contributor Author

bourgeoa commented Feb 4, 2021

@timbl Thanks.
As I understand :

  • solid-logic offers helper functions to solid-ui as the main purpose as you said in solidos chat
  • these helper functions are mainly working on the store in cache

And the solid-ui code calls x.load() many times just to make sure it is loaded before following links from x. It is essentilal that x.load returns immediately if tehe document is already noted. We clearly have to make this clear in the documentation

The proposed patch do not change the actual use of x.load() in solid-ui

Shall there be possibility to refresh a document in the store if expired ? That opens the use case of solid-logic like in test-suite.
and it improves the cache use :

  1. should refresh be at the load() level or only in the functions using load() : I prefer load() this makes it global
  2. should that be implicit or with explicit options : for performance I think default should not refresh

Do not use store.removeDocument as it will leave the cache metadata wrong.

refreshIfExpired() seems an interesting medium term. But how can I know that an rdflib function is unsafe ?

@bourgeoa
Copy link
Contributor Author

Finally modified only getContainerMembers(). Recursive delete must be sure to act on the latest situation and not from a cache that may not be up to date.

Base automatically changed from master to main February 24, 2021 17:48
@timea-solid
Copy link
Member

@bourgeoa is this still accurate? Shall we merge it?

@bourgeoa
Copy link
Contributor Author

bourgeoa commented Mar 1, 2022

For me it was good to merge.
Recursive delete it used in the solid/test-suite
I see some conflicts now

timea-solid added a commit that referenced this pull request Mar 8, 2022
@timea-solid
Copy link
Member

timea-solid commented Mar 8, 2022

Because this PR was hard to merge due to the refactoring of code, I created a new PR with just the prominent changes of this PR: https://github.com/solid/solid-logic/tree/loadImprovement

@bourgeoa please check it and close and merge these PRs if ok.
Tests are failing. Those need to be checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants