setThing not creating blank nodes included within a Thing

[Maximvdw]

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?

[Vinnl]

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]

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]

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]

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]

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]

Unfortunately no update yet on the PR...