-
Notifications
You must be signed in to change notification settings - Fork 36
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
base: main
Are you sure you want to change the base?
Conversation
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:
|
…s with blank nodes
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/inrupt/solid-client-js/9PQMquMZC8Vuy2MYJmq6NuPiwLEZ |
Any particular info or changes requested to this PR? |
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? |
@Maximvdw could you rebase this branch? I've flagged it with the team to make a decision and try to get this PR resolved. |
Sorry, I lost track on this PR for a while - I will see if I can rebase it |
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. |
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 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' thatsetThing
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.
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.