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

Removed blank node filter in internal additions changelog to support setting a Thing with blank node(s) #1545

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Maximvdw
Copy link
Contributor

This PR fixes #1544.

Description of the problem
In May 2021, a commit was made that adds a blank node filter to the internal additions/deletions changelog of a dataset:
01133a8#diff-66655e826d3c9a3644992d4184386acb8eab85f2f9f829d4dde6a1ac869ef9d7R207
The reason for this was the following:

This is needed because we cannot reconcile Blank Nodes in additions and
deletions. Down the road, we should do a diff before saving a SolidDataset
against a saved copy of the originally-fetched one, based on our own data
structures, which should make it easier to reconcile.

This reason is valid, in the sense that there is no knowledge between adding/removing blank nodes to know
"what blank node" you want to remove (in the current implementation). However, this is mainly an issue with deletions and not additions. Having this filter prevents any possibility for developers to add a Thing with a blank node.

I have kept the filter in the deletions changelog (where the issue outlined in the original problem statement). This should enable setThing to store blank nodes when developers use the (undocumented) addTerm method or when they bypass the API entirely (i.e. when serializing N3 Quads to a Thing).

Why I feel this change is needed
Blank nodes is something that is implicitly supported with addTerm (unit test example for blank node https://github.com/inrupt/solid-client-js/blob/main/src/thing/add.test.ts#L2048-L2060) and the interface itself that implies its support. A general explanation on why I would need blank nodes seems a bit trivial as this applies to the general use of blank nodes on the semantic web.

getThing itself supports blank nodes, enforcing the 'assumption' that setThing would support it as well.

For example a simple observation from the SOSA ontology: https://www.w3.org/TR/vocab-ssn/#apartment-134 that uses nested blank nodes (lets assume you want to share your electricity consumption with your energy provider for this use case). The information contained in these blank nodes are very specific to the named node Observation and should not be named in this particular case.

<Observation/235714> rdf:type sosa:Observation ;
  sosa:observedProperty  <apartment/134/electricConsumption> ;
  sosa:madeBySensor <sensor/926> ; 
  sosa:hasResult [
     rdf:type qudt-1-1:QuantityValue ;
     qudt-1-1:numericValue "22.4"^^xsd:double ;
     qudt-1-1:unit qudt-unit-1-1:Kilowatthour ] ;
  sosa:phenomenonTime [
    rdf:type time:Interval ;
    time:hasBeginning [ 
      rdf:type time:Instant ;
      time:inXSDDateTimeStamp "2017-04-15T00:00:00+00:00"^^xsd:dateTimeStamp ] ;
    time:hasEnd [ 
      rdf:type time:Instant ;
      time:inXSDDateTimeStamp "2017-04-16T00:00:00+00:00"^^xsd:dateTimeStamp ] ] ;
  sosa:resultTime "2017-04-16T00:00:12+00:00"^^xsd:dateTimeStamp .

Compatibility
The change does not break any unit tests. By keeping the filter in the deletions, it will not break documented API behaviour without limiting functionalities for developers using addTerm or bypassing the Thing builder.

A unit test is created to demonstrate the change.

@Maximvdw Maximvdw requested a review from a team as a code owner March 27, 2022 18:39
@vercel vercel bot temporarily deployed to Preview March 27, 2022 18:39 Inactive
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 27, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 580fd39:

Sandbox Source
solid-client-sandbox Configuration

@vercel vercel bot temporarily deployed to Preview March 28, 2022 07:31 Inactive
@vercel
Copy link

vercel bot commented Apr 12, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/inrupt/solid-client-js/9PQMquMZC8Vuy2MYJmq6NuPiwLEZ
✅ Preview: https://solid-client-js-git-fork-maximvdw-main-inrupt.vercel.app

@vercel vercel bot temporarily deployed to Preview April 12, 2022 00:55 Inactive
@Maximvdw
Copy link
Contributor Author

Any particular info or changes requested to this PR?

@Maximvdw
Copy link
Contributor Author

Can this PR get some attention, blank node support is a very important feature in our opinion - and after testing this PR for a few months I have not found any breaking issues yet?

@ThisIsMissEm
Copy link
Contributor

@Maximvdw could you rebase this branch? I've flagged it with the team to make a decision and try to get this PR resolved.

@ThisIsMissEm ThisIsMissEm added the Triaged This means that we've a ticket to look at this in the future label Nov 25, 2022
@Maximvdw
Copy link
Contributor Author

Sorry, I lost track on this PR for a while - I will see if I can rebase it

@NSeydoux
Copy link
Contributor

I just introduced a couple of changes that are somewhat related to this PR, as they add more support for Blank Nodes. I'll see if I can rebase this and align it with the latest changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Triaged This means that we've a ticket to look at this in the future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setThing not creating blank nodes included within a Thing
3 participants