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

setThing not creating blank nodes included within a Thing #1544

Open
Maximvdw opened this issue Mar 26, 2022 · 6 comments · May be fixed by #1545
Open

setThing not creating blank nodes included within a Thing #1544

Maximvdw opened this issue Mar 26, 2022 · 6 comments · May be fixed by #1545
Labels
bug Something isn't working

Comments

@Maximvdw
Copy link
Contributor

Maximvdw commented Mar 26, 2022

I am experiencing issues with setThing ignoring blankNodes entirely. The internal dataset changelog will have no quads for the blank node. This seems to be expected behavior according to this line;
https://github.com/inrupt/solid-client-js/blob/main/src/thing/thing.internal.ts#L121
which removes them from the "additions". Removing this filter fixes the issue without any failed tests, so I am not sure why it is being used?

I've created a unit test in this fork:
Maximvdw@d2227cf

The unit test creates a mock object:

const mockThing3: ThingPersisted = {
    type: "Subject",
    url: mockThing3Iri,
    predicates: {
      ["https://arbitrary.vocab/predicate"]: {
        namedNodes: ["https://arbitrary.vocab/object"],
        blankNodes: [
          {
            ["https://arbitrary.vocab/blanknode/predicate"]: {
              namedNodes: ["https://arbitrary.vocab/blanknode/object"],
            },
          },
        ],
      },
    },
  };

When using setThing only one quad will be added (the named node) which is verified
with the test line at Maximvdw@d2227cf#diff-b64c1976153ad9380bdd23fab11663989148669f7f2e99cbd3faf7342a687b34R600

Is this intended behavior for setThing?

@Maximvdw Maximvdw added the bug Something isn't working label Mar 26, 2022
@Vinnl
Copy link
Contributor

Vinnl commented Mar 27, 2022

solid-client doesn't currently support creating blank nodes - you're not using a documented API to add them to a Thing, I think?

@Maximvdw
Copy link
Contributor Author

Maximvdw commented Mar 27, 2022

I use addTerm which makes the blank node quads compatible with the Thing interface.

https://github.com/inrupt/solid-client-js/blob/main/src/thing/add.ts#L404

However, as noted in the OP - removing that one line which filters out blank nodes fixes the issue without any issues to the existing unit tests, so I am mainly interested in why it has been added.

01133a8#diff-66655e826d3c9a3644992d4184386acb8eab85f2f9f829d4dde6a1ac869ef9d7R207

You added the filter in this commit of May 2021 with a description on the "why". But currently I can not see why the filter is needed in the additions (or more specifically, why there is no unit test prohibiting its removal).

For my use cases I am using a fork where it is removed without any issues (blank nodes is important for my use case)

@Vinnl
Copy link
Contributor

Vinnl commented Mar 27, 2022

Ah! Yeah, that API is available but not documented because it has caveats like this. (Btw, I should note that I no longer work at Inrupt nor on this library, so my knowledge may be out of date.) You are probably right that it would have been good if I'd have added a unit test that highlights the problems that the filter is trying to avoid. If memory serves me right, the reason it was added is because if you do something like this:

const blankNode = new BlankNode();
let updatedThing = addTerm(oldThing, "https://example.predicate", blankNode);
updatedThing = removeTerm(updatedThing, "https://example.predicate, blankNode);

...solid-client would have no way of knowing that the two instances of the blank node are the same.

I'm sure it would be useful to Inrupt if you could specify your use case and why e.g. named nodes won't suffice, to inform their product roadmap :)

@Maximvdw
Copy link
Contributor Author

Maximvdw commented Mar 27, 2022

Hey, thanks for the example. Personally, I feel this would more be an issue for deletion/updating things and not really a breaking issue in the additions itself (justifying the filter to limit functionalities) . On a side note, the issue could potentially be solved with getThingsAll which was updated in Oct 2021 to return blank nodes (#1279), which could be used to remove the thing+related blank nodes (by the developer using the API)

Reconciliation will always remain a difficult aspect for blank nodes, so I see it's use within the delete changelog but not the additions

@Maximvdw
Copy link
Contributor Author

Maximvdw commented Mar 27, 2022

FYI: I will create a PR with some additional unit tests where I would remove the filter from the additions but not the deletions. Down the road these should also help in prepping the api for official blank node support, but currently it's a limitation for a problem that does not exist in the api (limiting those who wish to experiment for the future =D) . Thanks for the help Vincent.

@Maximvdw
Copy link
Contributor Author

Unfortunately no update yet on the PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants