90% - Prevent null values being added to ExtendedGraph #82

lordtatty · 2015-07-21T10:46:24Z

To address issue #81

This will ensure that subjects, predicates, and resource values are always strings. Literal values can be any scalar - we would like to enforce strings for literal values too, but doing so may break legacy databases which use non-strings as values.

scaleupcto · 2015-07-21T10:58:52Z

src/classes/ExtendedGraph.class.php

+     *
+     * @return bool
+     */
+    protected function isValueValid($value){


change name to isValidLiteralValue() ?

Wouldn't we want to check resource values as well though? isValidTripleValue?

From Tripod's perspective, don't we just need to ensure that subject, predicate, and object are all strings?

non-empty strings

Agree, but enforcing strings as opposed to null could be very painful for any graphs which are already storing non-string values (eg. integers). This current PR to ignore null values could well cause enough headaches without taking out integers at the same time. I do think we should ultimately enforce strings-only, but maybe one step at a time, once we are comfortable with no nulls.

Or arrays of strings and integers.

I guess what I'm saying is that we should be doing type checking here so we're not just allowing some other bad data that is equally as incompatible as a null value.

How do we deal with legacy data, though? We have no idea what crazy things have ended up in the graph. The php data types are:

String. Integer. Float (floating point numbers - also called double) Boolean. Array. Object. NULL. Resource.

Integer, floats and booleans could all have ended up in the graph. Is it possible to add an array via the public ExtendedGraph methods?

Agree that really we should be ignoring anything that isn't a string, but I guess the question is how much we are prepared to break anything currently using tripod to enforce this.

Tripod only supports plain literals. Full-stop. If other data types are in the documents they would produce invalid RDF, anyway, because we do not support typed literals at all.

Discussed this with @rsinger Agreed to continue with the current approach. I have just tested to see if add_to_literal can accept arrays, and it can, so we can't simply ignore any non-scalar.

rsinger · 2015-07-21T13:40:42Z

src/classes/ExtendedGraph.class.php

+     * @return bool
+     */
+    protected function isValidTripleValue($value){
+        if(!is_string($value) && !is_int($value) && !is_array($value) && !is_float($value) && !is_bool($value)){


Why not is_scalar here?

Uh yep, I meant to use that, no idea why I didn't.... one moment.

lordtatty · 2015-07-21T21:37:43Z

@rsinger @RobotRobot Something I just noticed is https://github.com/talis/tripod-php/blob/master/src/classes/ExtendedGraph.class.php#L141 - @return boolean true if the triple was new, false if it already existed in the graph.

What should be return of add_resource_triple and add_literal_triple be if we aren't adding the triple due to the value being invalid? Null? False? At the moment it is false.

lordtatty · 2015-07-22T07:40:29Z

@rsinger @RobotRobot also at the moment we are allowing scalars for both literals and resources. Should we restrict resources to only strings? I really can't imagine how having anything other than a string can be legitimate for a uri.

scaleupcto · 2015-07-22T11:09:03Z

resources should definitely always be strings. So I think we should validate them as such, rather than scalars.

rsinger · 2015-07-22T12:07:20Z

src/mongo/MongoGraph.class.php

     * @param array $mongoValueObject
-     * @return array
+     * @return array| false an array of values or false if the value is not valid


array| false isn't a valid phpdoc, it needs to be array|bool

rsinger · 2015-07-22T12:16:27Z

This looks good. Should we validate $s and $p while we're here? Is it possible to add invalid values for them currently?

lordtatty · 2015-07-22T12:32:21Z

@rsinger I feel like maybe that should be raised as a separate issue so we can get this branch down.

rsinger · 2015-07-22T12:33:04Z

Why can't you just run $s and $p through isValidResourceProperty?

rsinger · 2015-07-22T12:34:08Z

I mean, my real question is, can you even add invalid values there? I don't see why wouldn't fix it here, if so.

lordtatty · 2015-07-22T13:22:00Z

@rsinger my only real reservation to fixing that in this branch is that I'll have to add a whole bunch of tests to cover that, and this needs to get shipped today, ideally. It's currently passing as it is so I think the right strategy is to merge this down and fix up the $s and $p in another branch...

rsinger · 2015-07-22T13:25:12Z

Can you just confirm whether or not it's even an issue? There is no point in making a release to prevent us from adding duff data if we've still left it open to add duff data.

lordtatty · 2015-07-22T15:10:06Z

Added checks for subjects and predicates

lordtatty · 2015-07-22T18:57:59Z

This will now also throw an exception if trying to add a triple with a blank subject

rsinger · 2015-07-22T18:59:09Z

src/classes/ExtendedGraph.class.php

+     *
+     * @return bool
+     */
+    protected function isValidTripleValue($value){


Triple value?

I thought that would be a good generic name to cover subject, predicate and value.

I think it made more sense when it was named resourcevalue

It's used for more than just validating a resource value though - the only thing it doesn't validate now is a literal value. isValidTripleComponentValue?

Should I be breaking this out into isValidSubject, isValidPredicate, and isValidVaue? It might make sense since we are now ensuring that the subject is not an empty string, as we can just add the check into there...

No, they're all just uris.

isTripleValue() really doesn't make a lot of sense.

The graph is made up of subjects ($s) predicates ($p) and objects ($o).

Subjects and predicates are always resources (another name for URIs), objects can be either a resource or a literal.

So it makes more sense to have two methods that reflect that.

isValidResource() - this covers subjects, predicates or objects that are of type URI. Those are all resources - a different term for URI.

isValidLiteral() covers objects that are literals only.

You are currently doing validation in two places - once in _add_triples() and again in the individual methods - pick one or the other - don't do both - that just wastes CPU cycles unnecessarily.

Finally you're null checking outside these methods. Do the null checking inside the isValidResource(). If the validity check fails for resources or predicates (i.e. returns false) I think the calling method might validly throw an exception, but for objects we are saying it is allowed but ignored (to keep the peace).

I see your point, in my head I was separating subjects and resource values unnecessarily here, will fix up.

I hadn't spotted that duplicate validation - that was a leftover from a previous iteration. Removing.

scaleupcto · 2015-07-22T20:05:22Z

src/mongo/MongoGraph.class.php

+                if(!isset($value['r']) || !$this->isValidTripleValue($value['r'])){
+                    return;
+                }
+                if($value['r'] === ""){


again - why is your validity checking not inside the method validating the value? Instead if isValidWhatever returns false then throw your exception.

lordtatty · 2015-07-22T21:53:29Z

@RobotRobot @rsinger Fixed up from PR comments:

Renamed isValidTripleValue back to isValidResourceValue, and added the empty string check into there.
Removed an unnecessary predicate validation which is being covered by the the labeller, and added appropriate tests to cover that.
toGraphValueObject no longer returns false if the array is empty.

scaleupcto · 2015-07-23T07:58:05Z

src/classes/ExtendedGraph.class.php

+     * @param string $value
+     * @return bool
+     */
+    protected function isValidLiteralValue($value){


Too verbose, rename isValidLiteral()

scaleupcto · 2015-07-23T08:00:03Z

👍 just rename those methods, then merge

…d-to-extendedgraph 90% - Prevent null values being added to ExtendedGraph

Prevent null values being added to ExtendedGraph

1b8edf8

scaleupcto reviewed Jul 21, 2015
View reviewed changes

lordtatty added 3 commits July 21, 2015 12:48

Added tests

ec3fb44

Make valid values stricter as per PR comments

5c89879

renamed method as per PR

ac2071e

rsinger reviewed Jul 21, 2015
View reviewed changes

simplified conditional from PR

54fbb4f

lordtatty changed the title ~~30% - Prevent null values being added to ExtendedGraph~~ 80% - Prevent null values being added to ExtendedGraph Jul 21, 2015

lordtatty added 3 commits July 22, 2015 12:30

Fixed up from PR comments

cdb99be

Improve testing

3a963e4

Better testing

a8cfc26

rsinger reviewed Jul 22, 2015
View reviewed changes

Fixed up docblock

6a96dcb

Enforce subjects and predicates are also strings

460f258

lordtatty changed the title ~~80% - Prevent null values being added to ExtendedGraph~~ 90% - Prevent null values being added to ExtendedGraph Jul 22, 2015

Ensure exception is thrown if a blank subject is added to ExtendedGraph

5396f60

rsinger reviewed Jul 22, 2015
View reviewed changes

scaleupcto reviewed Jul 22, 2015
View reviewed changes

lordtatty added 2 commits July 22, 2015 22:48

Fixed up from PR comments

60d0e3c

Fixed up formatting

a895200

scaleupcto reviewed Jul 23, 2015
View reviewed changes

Renamed Methods as per PR

c8ef3f6

lordtatty added a commit that referenced this pull request Jul 23, 2015

Merge pull request #82 from talis/do-not-allow-null-values-to-be-adde…

89044e8

…d-to-extendedgraph 90% - Prevent null values being added to ExtendedGraph

lordtatty merged commit 89044e8 into master Jul 23, 2015

lordtatty deleted the do-not-allow-null-values-to-be-added-to-extendedgraph branch July 23, 2015 09:01

90% - Prevent null values being added to ExtendedGraph #82

90% - Prevent null values being added to ExtendedGraph #82

Conversation

lordtatty commented Jul 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordtatty commented Jul 21, 2015

lordtatty commented Jul 22, 2015

scaleupcto commented Jul 22, 2015

Choose a reason for hiding this comment

rsinger commented Jul 22, 2015

lordtatty commented Jul 22, 2015

rsinger commented Jul 22, 2015

rsinger commented Jul 22, 2015

lordtatty commented Jul 22, 2015

rsinger commented Jul 22, 2015

lordtatty commented Jul 22, 2015

lordtatty commented Jul 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordtatty commented Jul 22, 2015

Choose a reason for hiding this comment

scaleupcto commented Jul 23, 2015