PHPC-314: Implement type map syntax for documents within field paths #793

derickr · 2018-04-04T15:42:50Z

First step of implementing the fieldPath syntax. It doesn't do wild cards yet, or typemaps for scalar types.

jmikola · 2018-04-16T13:30:41Z

tests/bson/typemap-002.phpt

-        parent::__construct($data);
-    }
-}
-


Looks like this was always dead code. Correct?

jmikola · 2018-04-16T13:32:22Z

tests/bson/typemap-003.phpt

+var_dump(is_array($documents[0]->array));
+var_dump($documents[0]->object instanceof MyArrayObject);
+var_dump(is_array($documents[0]->object['string']));
+var_dump(is_array($documents[0]->object->string));


Isn't this more of a test of ArrayObject::ARRAY_AS_PROPS?

jmikola · 2018-04-16T13:37:17Z

tests/bson/typemap-003.phpt

+$document2 = [
+    '_id' => 2,
+    'array' => [4, 5, 6],
+    'object' => ['associative' => ['elementen', 'elements' ]]


If I understand correctly, this test is verifying that the Cursor's type map can be overridden and that the default behavior applies if a field path does not match. I think it'd be clearer to use names like "array1" and "array2" instead of "string" and "associative" for these BSON arrays, though.

This test was originally to see if we could set the typemap twice without leaking or memory issues.

But I added a functional bit to test that a bit better, and might ended up having just a test for PROPS_AS_ARRAY. Because of the memory issue/doubble setting, I would like to keep it though.

I'm still not sure how ARRAY_AS_PROPS relates to the double setting of a type map, but no objections if you'd like to keep this in place.

It doesn't — ARRAY_AS_PROPS was just added to make the asserts a little easier to write.

jmikola · 2018-04-16T13:38:33Z

tests/bson/typemap-004.phpt

+    }
+
+    $documents = $cursor->toArray();
+    return $documents;


I suppose these lines can be consolidated into return $cursor->toArray().

Sure, will fix that. I probably had a debug statement in between at some point.... which might still be useful in case an IDE is used though?

I don't follow the relation to an IDE. If you're referring to an IDE indicating the type information for a $documents variable on these two lines, I don't think that's useful.

No, if there is no variable, an IDE can't inspect its contents, as it can't see the direct return values of a function.

jmikola · 2018-04-16T13:40:48Z

tests/bson/typemap-005.phpt

+$documents = fetch($manager);
+var_dump($documents[0] instanceof stdClass);
+var_dump(is_array($documents[0]->array0));
+var_dump(is_object($documents[0]->array1));


This seems redundant in light of the instanceof assertion that follows.

Perhaps, but I'm just being thorough.

jmikola · 2018-04-16T14:30:03Z

src/bson.c

+	if (map->field_paths.allocated < map->field_paths.size + 1) {
+		map->field_paths.allocated += PHONGO_FIELD_PATH_EXPANSION;
+		map->field_paths.map = erealloc(map->field_paths.map, sizeof(php_phongo_field_path_map_element) * map->field_paths.allocated);
+	}


Should we have an "ensure" function for this, or is this really the only place we need to increase allocation?

It's the only place, and we always add to the end. Which is not the case when we build the current conversion field path for error messages and mapping, as there we leave nested sections as often as we go into them. I'll add a comment.

jmikola · 2018-04-16T14:30:39Z

src/bson.c

+{
+	php_phongo_field_path_map_element* tmp = ecalloc(1, sizeof(php_phongo_field_path_map_element));
+
+	tmp->entry = php_phongo_field_path_alloc(true);


In this case, the element names should always be estrdup()-ed?

Yes, and they will be through https://github.com/mongodb/mongo-php-driver/pull/793/files#diff-f7d29a680148f52d6601f59ed787f577R139 if true is used.

jmikola · 2018-04-16T14:31:24Z

src/bson.c

+	return tmp;
+}
+
+bool php_phongo_bson_state_add_field_path(php_phongo_bson_typemap* map, char* field_path_original, php_phongo_bson_typemap_types type, zend_class_entry* ce TSRMLS_DC)


If field_path_map_element_set_info() doesn't need TSRMLS_DC, does this?

Doesn't look like it, will remove it.

jmikola · 2018-04-16T14:33:03Z

src/bson.c

+
+	/* Loop over all the segments. A segment is delimited by a "." */
+	element = php_strtok_r(field_path_str, ".", &saveptr);
+	while (element) {


Is there a case where we don't enter the while loop on the first iteration?

It may be good to have error tests for field paths such as "...", ".foo", and "" so we can test how that is handled.

I don't think we won't ever enter, but I will add test cases. I know that strtok handles ... as . for example.

Sigh, strtok_r is pretty useless, as it can't handle empty elements (like in foo..bar), so I've had to write this, which also means that we needed to keep some TSRMLS_[DC]C things around.

jmikola · 2018-04-16T14:34:37Z

src/bson.c

+				return false;
+			}
+
+			if (!php_phongo_bson_state_add_field_path(map, ZSTR_VAL(string_key), map_type, map_ce TSRMLS_CC)) {


php_phongo_bson_state_add_field_path() always returns true. In the event a failure case is introduced, I suppose we'll want to ensure it throws an exception. Correct?

Is that why you're preemptively passing in TSRMLS_CC here (consistency with php_phongo_bson_state_parse_type())?

I'd wish to say that I added TSRMLS_CC because of that, but that's not true :-) But yes, perhaps we should leave it as long as the strtok loop can create failure notifications with the three cases that you suggested.

Aye, let's revisit this if you decide to add exceptions for tokenization errors.

Which I've now just done :-).

derickr

I've updated the PR with changes related to your comments - except for the ones that you didn't comment on after my first reply.

derickr · 2018-04-17T09:18:59Z

src/bson.c

-		field_path->elements         = erealloc(field_path->elements, sizeof(char**) * field_path->allocated_levels);
+		field_path->allocated     = field_path->size + PHONGO_FIELD_PATH_EXPANSION;
+		field_path->elements      = erealloc(field_path->elements, sizeof(char**) * field_path->allocated);
+		field_path->element_types = erealloc(field_path->element_types, sizeof(php_phongo_bson_field_path_item_types*) * field_path->allocated);


[Narrator: It did]

derickr · 2018-04-17T10:37:40Z

src/bson.c

+
+	/* Loop over all the segments. A segment is delimited by a "." */
+	element = php_strtok_r(field_path_str, ".", &saveptr);
+	while (element) {


Sigh, strtok_r is pretty useless, as it can't handle empty elements (like in foo..bar), so I've had to write this, which also means that we needed to keep some TSRMLS_[DC]C things around.

derickr · 2018-04-17T10:38:08Z

src/bson.c

+				return false;
+			}
+
+			if (!php_phongo_bson_state_add_field_path(map, ZSTR_VAL(string_key), map_type, map_ce TSRMLS_CC)) {


Which I've now just done :-).

derickr · 2018-04-17T10:39:01Z

tests/bson/typemap-003.phpt

+$document2 = [
+    '_id' => 2,
+    'array' => [4, 5, 6],
+    'object' => ['associative' => ['elementen', 'elements' ]]


It doesn't — ARRAY_AS_PROPS was just added to make the asserts a little easier to write.

derickr · 2018-04-17T10:39:48Z

tests/bson/typemap-004.phpt

+    }
+
+    $documents = $cursor->toArray();
+    return $documents;


No, if there is no variable, an IDE can't inspect its contents, as it can't see the direct return values of a function.

derickr · 2018-04-17T10:52:27Z

tests/bson/typemap-006.phpt

+var_dump($documents[0] instanceof stdClass);
+var_dump(is_array($documents[0]->array));
+var_dump($documents[0]->array[0] instanceof MyWildCardArrayObject);
+var_dump($documents[0]->array[1] instanceof MyArrayObject);


Hah!::

class MyWildcardArrayObject extends MyArrayObject {};

So of course it matches :-) I will get rid of the extends, and repeat the MyArrayObject implementation.

Luckily the implementation was fine, just not the test.

derickr · 2018-04-17T10:54:58Z

tests/bson/typemap-006.phpt

+var_dump($documents[0] instanceof stdClass);
+var_dump(is_object($documents[0]->object));
+var_dump($documents[0]->object->one instanceof MyArrayObject);
+var_dump($documents[0]->object->two instanceof MyWildCardArrayObject);


Same as above.

derickr · 2018-04-17T10:55:26Z

tests/bson/typemap-007.phpt

+    }
+}
+
+class MyWildCardArrayObject extends MyArrayObject {};


[Narrator: They were changed to MyWildcardArrayObject anyway]

derickr · 2018-04-17T10:58:13Z

tests/bson/typemap-007.phpt

+echo "\nSetting 'object.parent1.$' path to 'MyWildCardArrayObject' and 'object.$.$' to 'MyArrayObject'\n";
+$documents = fetch($manager, ["fieldPaths" => [
+    'object.parent1.$'    => "MyWildCardArrayObject",
+    'object.$.$'          => "MyArrayObject",


Done, added this.

derickr · 2018-04-17T10:59:37Z

tests/cursor/cursor-setTypeMap_error-003.phpt

+        $cursor->setTypeMap($typeMap);
+    }, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";
+
+    echo "\n";


jmikola · 2018-04-17T13:34:58Z

src/bson.c

+
+	if (field_path_original[strlen(field_path_original) - 1] == '.') {
+		phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "A 'fieldPaths' key may not end with a '.'");
+		return false;


Assuming both of these cases are handled by the "empty segment" error below, I'd propose allowing that to catch this. IMO, ".foo", "foo..bar", and "foo." all contain empty segments.

I had that first, but thought it better we have more precise error messages. This one is also faster to check as nothing has been allocated yet.

jmikola · 2018-04-17T13:40:36Z

tests/cursor/cursor-setTypeMap_error-003.phpt

@@ -0,0 +1,67 @@
+--TEST--
+Cursor::setTypeMap(): fieldPaths must be an array, with single key/string elements


and it tests the same thing as the error-001 and error-002 tests, so it should be the same here — and not as toPHP_error

Not sure I follow. My suggestion was that once we finalize these tests for Cursor::setTypeMap() we copy them for toPHP() to ensure that function passes as well.

Separate from this, can we have all of the current tests organized under cursor/cursor-setTypeMap*? Any other tests that get copied can then go under bson/bson-toPHP*

EDIT: Just saw your reply in #793 (comment) about wanting to keep things under bson/typemap*. Point noted, but will we have tests for toPHP() or do you think that's superfluous?

jmikola · 2018-04-17T13:56:21Z

tests/bson/bson-toPHP-007.phpt

+--TEST--
+MongoDB\BSON\toPHP(): fieldPath typemaps without server
+--SKIPIF--
+<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>


Server-less tests shouldn't import this, as it ultimately requires PHONGO-SERVERS.json to exist. The extension check in basic-skipif.inc isn't very helpful anyway.

I suggest just omitting SKIPIF here, as is done in other server-less tests.

…r each level

…inlined

strtok_r() can't handle empty elements :-/

derickr requested review from jmikola and kvwalker April 4, 2018 15:42

kvwalker approved these changes Apr 6, 2018

View reviewed changes

derickr force-pushed the PHPC-314-typemap-for-fieldpaths branch 2 times, most recently from 625d3c4 to 67328be Compare April 13, 2018 12:10

jmikola reviewed Apr 16, 2018

View reviewed changes

derickr commented Apr 17, 2018

View reviewed changes

derickr force-pushed the PHPC-314-typemap-for-fieldpaths branch 2 times, most recently from 500db06 to 7fba7f5 Compare April 17, 2018 12:21

jmikola reviewed Apr 17, 2018

View reviewed changes

derickr force-pushed the PHPC-314-typemap-for-fieldpaths branch from 99394d7 to d0e8318 Compare April 17, 2018 14:05

jmikola approved these changes Apr 17, 2018

View reviewed changes

derickr added 12 commits April 18, 2018 11:02

PHPC-314: Read fieldPaths elements and check for correct map types

f5e4976

PHPC-314: Record whether we're dealing with a document or an array fo…

d4d1fa3

…r each level

PHPC-314: Parse fieldPaths element of the typemap into a tree

bdda892

PHPC-1154: Implement wild card syntax for typemap field paths

eddcd4b

PHPC-1154: Fix memory leak with typemaps and MongoDB\BSON\toPHP

e7e9fe7

s/allocated/allocated_size

f5965ce

Use correct size in reallocs

dac1d78

Rename free_elements to owns_elements, as that is more descriptive

6da5d4b

Hint to the compiler that map_element_matches_field_path() should be …

bb31cd4

…inlined

Abandon strtok_r() in favour of a hand rolled algorithm

2cd3e4f

strtok_r() can't handle empty elements :-/

Tweaks to test cases

b42165e

Move test case that doesn't touch server to BSON\toPHP()

da8853a

derickr force-pushed the PHPC-314-typemap-for-fieldpaths branch from d0e8318 to da8853a Compare April 18, 2018 10:04

derickr merged commit da8853a into mongodb:master Apr 18, 2018

derickr added a commit that referenced this pull request Apr 18, 2018

Merged pull request #793

eab4a21

		@@ -0,0 +1,67 @@
		--TEST--
		Cursor::setTypeMap(): fieldPaths must be an array, with single key/string elements

PHPC-314: Implement type map syntax for documents within field paths #793

PHPC-314: Implement type map syntax for documents within field paths #793

Uh oh!

Conversation

derickr commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jmikola Apr 17, 2018 •

edited

Loading

jmikola Apr 17, 2018 •

edited

Loading