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

removeMetadata #633

Merged
merged 2 commits into from
Mar 17, 2024
Merged

removeMetadata #633

merged 2 commits into from
Mar 17, 2024

Conversation

bourgeoa
Copy link
Contributor

@bourgeoa bourgeoa commented Feb 4, 2024

store.remove() fails on triple containing a Collection

const meta = this.fetcher?.appNode // this.sym('chrome://TheCurrentSession')
const linkNamespaceURI = 'http://www.w3.org/2007/ont/link#'
const kb = this
// removeMatches() --> removeMany() --> remove() fails on Collection
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to fix the remove() function to work with Collection instead of applying a workarround here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it would. I did not find howto.

Copy link
Member

Choose a reason for hiding this comment

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

What goes wrong with when one f the nodes being removed is a collection?

Copy link
Contributor Author

@bourgeoa bourgeoa Feb 10, 2024

Choose a reason for hiding this comment

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

@timbl
This is I hope is a detailed enough issue #631

for (var r = 0; r < requests.length; r++) {
const request = requests[r]
if (request !== undefined) {
this.removeMatches(request, null, null, meta)
if (request != null) { // null or undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment says "or undefined" but the code onyl checks for null. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loose inequality does it
It is used in other places in rdflib

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Angelo is correct. If you remove line 884 which checks for undefined, the !=null will throw an undefined error if request is undefined.

@@ -223,6 +223,17 @@ describe('UpdateManager', () => {
expect(updater.editable(doc1)).to.equal(undefined)
})

it('Should not detect a document is editable from metadata after removeMetadata', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these tests related to the changes? I would have expected a test that assures that Collections are now working.

Copy link
Contributor Author

@bourgeoa bourgeoa Feb 7, 2024

Choose a reason for hiding this comment

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

They test the new function removeMetadata

@bourgeoa
Copy link
Contributor Author

bourgeoa commented Mar 9, 2024 via email

@jeff-zucker
Copy link
Contributor

jeff-zucker commented Mar 9, 2024 via email

@jeff-zucker jeff-zucker self-requested a review March 16, 2024 22:19
@jeff-zucker
Copy link
Contributor

jeff-zucker commented Mar 17, 2024

I ran the attached script on solidcommunity.net and it errored with statement to be removed is not in store. I got this error even though there was no collection so I don't think the collection is the problem.

The same script on the test server did not produce an error. But it also did not remove all metadata about the document. Even after the new removeDocument(), there were three statements left over. The predicates related to the document that were not removed were acl, describedBy, and type - things from the link relations.

I am not sure if it was intentional to leave these, but if not, it should be easy to fix.

<script src="https://solidcommunity.net:8443/mashlib.js"></script>                                  
<script type="module">                                                                              
                                                                                                    
let doc = UI.rdf.sym("https://localhost:8443/public/s/test/tmpr/test.ttl");                         
                                                                                                    
async function test(){                                                                              
  await UI.store.fetcher.load(doc);                                                                 
  show('load');                                                                               
  await UI.store.removeDocument(doc);                                                                     
  show('removeDocument');;                                                               
}                                                                                                   
test();                                                                                             
                                                                                                    
function show(msg){                                                                                 
  let stmts  = UI.store.match(null,null,null,doc);                                                  
  console.log(`${stmts.length} statements with graph specified after ` + msg);                      
  stmts  = UI.store.match();                                                                        
  console.log(`${stmts.length} statements with graph not specified after ` + msg);                  
}                                                                                                   
                                                                                                    
</script>                                                                                           

The document in question looks like this:

<#A> <#isa> <#B>.                                                                                   
<#C> <#isa> <#D>.                                                                                   

The script output was this (notice the 3 in the last one) :

2 statements with graph specified after load
30 statements with graph not specified after load
0 statements with graph specified after removeDocument
3 statements with graph not specified after removeDocument

@jeff-zucker
Copy link
Contributor

Also note that removeDocument did remove everything that had the document as the graph, but the three which were not removed had the document as subject and a blank node as the graph.

@bourgeoa
Copy link
Contributor Author

bourgeoa commented Mar 17, 2024

Thanks @jeff-zucker for looking at this in detail.

I ran the attached script on solidcommunity.net and it errored with statement to be removed is not in store. I got this error even though there was no collection so I don't think the collection is the problem.

The collection is in the metadata graph. (const meta = this.fetcher?.appNode // this.sym('chrome://TheCurrentSession'))
That is were the collection is an issue, not on the document graph
The issue is with the last object Collection (full metadata content here #631 (comment))

  [
     .......
     tabont:status
              ( "[0:16:35.259] N3 parsed: 13 triples in 26 lines."
              "[0:16:35.259] Done." )
  ].

Also note that removeDocument did remove everything that had the document as the graph, but the three which were not removed had the document as subject and a blank node as the graph.

Then these 3 statements are not related to either the graph document or the document part of the metadata graph
I shall look where these are created and if they may be removed.

They make no harm for the time being. So I prefer to keep these as an other issue.

Copy link
Contributor

@jeff-zucker jeff-zucker left a comment

Choose a reason for hiding this comment

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

I'll raise a separate issue on the un-removed link relations. Other than that, this looks good to go.

for (var r = 0; r < requests.length; r++) {
const request = requests[r]
if (request !== undefined) {
this.removeMatches(request, null, null, meta)
if (request != null) { // null or undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Angelo is correct. If you remove line 884 which checks for undefined, the !=null will throw an undefined error if request is undefined.

@bourgeoa bourgeoa merged commit 7a40abf into main Mar 17, 2024
6 checks passed
@bourgeoa bourgeoa deleted the issue#631 branch March 17, 2024 14:21
@bourgeoa bourgeoa restored the issue#631 branch March 22, 2024 18:20
@bourgeoa bourgeoa mentioned this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deleteDocument() : Error : Statement to be removed is not on store
4 participants