A null value can be added to a graph, but then throws an exception when removing

[lordtatty]

Currently tripod can add null values, but cannot remove them, resulting in an exception.

$emptyGraph = new \Tripod\ExtendedGraph()'
$testGraph = new \Tripod\ExtendedGraph();
$testGraph->add_literal_triple('http://testme.fake', 'http://purl.org/ontology/bibo/Bill', null);

// Works - saves the new triple to the graph with a null value
$tripod->saveChanges($emptyGraph, $testGraph);

// Fails - cannot remove the triple, throws exception "Removal value http://testme.fake http://purl.org/ontology/bibo/Bill does not appear in target document to be updated"
$tripod->saveChanges($testGraph, $emptyGraph);

The error is thrown here: https://github.com/talis/tripod-php/blob/master/src/mongo/delegates/Updates.class.php#L515

$valueIndex = array_search($removal, $valueObject); is returning null, because of line https://github.com/talis/tripod-php/blob/master/src/mongo/delegates/Updates.class.php#L490. The isset will return false against a null value, meaning tripod assumes the value to be an array of values already, so does not wrap the single null value in an array, meaning the array structure will be wrong when trying to match against the array_search. Resulting false, and throwing the exception.

1. Should tripod allow null values to be saved to a graph? I feel like it should be throwing an exception if trying to add anything other than a string.
2. We need to modify tripod to be able to cope with any graphs which already have null values inserted.

[rsinger]

Related to #47

[rsinger]

FWIW, this is a bug with ExtendedGraph - It's what should be preventing null values.

[scaleupcto]

Yup, agree @rsinger . ExtendedGraph should guard against adding null triples in the first place (should probably just silently continue if you try rather than save a null to the internal array).

@lordtatty if that was the case I am guessing your code above would work because $testGraph wouldn't actually contain anything, i.e. it would be functionally equivalent to $emptyGraph

[lordtatty]

Agree that this is the way tripod should be working, but what about handling existing data which has already been set as null? If we prevent extended graph from accepting a null value then any null values will be orphaned, as there will be no way to delete them (because there will be no way to have a 'before' graph with a null value in it). This could also have knock-on effects for anything which is relying on those triples existing.

Agree that anything which is affected by this is effectively a bug, but ExtendedGraph still needs to be able to load null values from the graph, tripod should prevent them from being saved.

Alternatively If we did want to lock down ExtendedGraph then we would could prevent null values being added by typical methods, as suggested, but provide a back-door method for the tripod driver to add whatever it finds in the graph. I'm not a fan of this route as that back-door method could be easily misused, but it would move the null-prevention into the right place.

[rsinger]

If extendedgraph didn't load them, you could save the graph and remove the
original null value.

I guess what I'm saying is that I'd expect our solution to clean up this
bug inline.

On Monday, July 20, 2015, lordtatty [email protected] wrote:

Agree that this is the way tripod should be working, but what about
handling existing data which has already been set as null? If we prevent
extended graph from accepting a null value then any null values will be
orphaned, as there will be no way to delete them (because there will be no
way to have a 'before' graph with a null value in it). This could also have
knock-on effects for anything which is relying on those triples existing.

Agree that anything which is affected by this is effectively a bug, but
ExtendedGraph still needs to be able to load null values from the graph,
tripod should prevent them from being saved.

Alternatively If we did want to lock down ExtendedGraph then we would
could prevent null values being added by typical methods, as suggested, but
provide a back-door method for the tripod driver to add whatever it finds
in the graph. I'm not a fan of this route as that back-door method could be
easily misused, but it would move the null-prevention into the right place.

—
Reply to this email directly or view it on GitHub
#81 (comment).

[lordtatty]

But if extended graph can't load it, then we can't have a 'before' graph to calculate a change set from to be able to remove it. Can we? At the very least Tripod needs to be able to add null values to ExtendedGraph, even if the user shouldn't.

[rsinger]

That's kind of what I'm saying: ExtendedGraph shouldn't allow users to add a null value, but MongoGraph should be able to self-heal any documents that are like that upon save.

[lordtatty]

@rsinger - @RobotRobot and I discussed this earlier and decided that the healing process isn't tripod's concern, and will need to be dealt with separately in any app which uses it. So my PR is preventing nulls from loading in to ExtendedGraph/MongoGraph entirely.

[rsinger]

I think this is going to be hard to clean up if Tripod isn't self-healing. Why wouldn't we do that?