PHPC-2206: Implement BSON\Value class #1412

alcaeus · 2023-04-21T13:58:42Z

This PR introduces the MongoDB\BSON\Value class as a wrapper for bson_value_t. The Document, PackedArray, and Iterator classes will be updated to return value instances instead of native values. This allows for a better handling of BSON types whenever raw BSON is consumed. For example, by using the Value class, users are able to differentiate between a 32-bit integers and 64-bit integers, even when the value does not fall in the 64-bit range.

The class contains a generic getValue method that will return the value contained in the Value instance. For documents and arrays, this returns Document and PackedArray instances respectively. For BSON types, it returns the corresponding MongoDB\BSON\Type instance, and for primitives it will return the PHP value.

In addition to the getValue method, it comes with a set of is* and get* methods for each BSON type. For added convenience, this includes accessors for integers (if one does not need to differentiate between 32-bit and 64-bit values), as well as numbers in general (if one does not need to differentiate between integers and doubles).

The PR also extracts some previously static factories from phongo_bson.c to the various BSON source files, as they are now used in multiple places. This will eventually be extracted into its own pull request. For the time being, the PR does not change the encoding and decoding of bson_value_t. As is, a bson_value_t is converted to a zval by adding it to an empty document, converting the document and extracting the relevant key. When converting a zval to bson_value_t this is handled the same way. Ideally, we'd skip this round-tripping and work with the bson_value_t directly. I'd like to complete this in a separate pull request as this is a larger effort if we want to avoid code duplication.

TODO:

Return Value instances when accessing document offsets
Return Value instances when accessing array offsets
Return Value instances in BSON iterator
Refactor encoding and decoding of bson_value_t instances.

src/BSON/Value.c

src/phongo_compat.h

tests/bson/bson-value-fromPHP-001.phpt

src/BSON/Value.stub.php

src/BSON/Value.c

src/BSON/ObjectId.h

src/BSON/DBPointer.h

src/BSON/Value.stub.php

src/BSON/Value.c

src/BSON/Value.stub.php

src/BSON/Value.c

src/BSON/Document.c

src/BSON/PackedArray.c

src/BSON/Value.c

src/phongo_bson_encode.c

src/phongo_compat.h

tests/bson/bson-document-clone-001.phpt

tests/bson/bson-value-var_export-001.phpt

jmikola

Just a few questions and some things to follow up on, but I think we're all set here.

Enjoy your customary Gordon Ramsey GIF:

.

jmikola · 2023-06-15T03:11:03Z

src/BSON/Value.stub.php

+    final public function serialize(): string {}
+
+    /** @param string $serialized */
+    final public function unserialize($serialized): void {}


I realize this is consistent with every other stub, but why do we have an @param annotation here?

The interface has a string type since php/php-src@273731f#diff-c8adc7ea754be4705d22956277447f2a8cdcd19e3185caa1513391d864a82384R44

It had no type hint in 7.4: https://github.com/php/php-src/blob/dca8d5565b947270a29f232bc54efd9df3b92b94/Zend/zend_interfaces.c#L572-L580

Given that, should we be using #if PHP_VERSION_ID >= 80000 to conditionally declare a type hint? If so, can you create a ticket for that?

This is due to the stub tool (gen_stub.php), which complains about parameters without types. Each parameter either needs to have a declared type or a corresponding @param annotation. The same goes for return types, where a return type or @return annotation are necessary.

jmikola · 2023-06-15T03:13:19Z

src/MongoDB/ClientEncryption.c


 	if (!mongoc_client_encryption_decrypt(clientencryption->client_encryption, &ciphertext, &value, &error)) {
 		phongo_throw_exception_from_bson_error_t(&error);
 		goto cleanup;
 	}

-	if (!php_phongo_bson_value_to_zval(&value, zvalue)) {
+	/** Use the legacy decoder to return PHP types instead of BSON types */


☝️ Outstanding question

jmikola · 2023-06-15T03:16:00Z

src/BSON/Value.c

+
+	bson = bson_new_from_data(intern->value.value.v_doc.data, intern->value.value.v_doc.data_len);
+
+	phongo_packedarray_new(return_value, bson, false);


Sorry for not following up here. A separate ticket SGTM to we don't lose track of this.

jmikola · 2023-06-15T03:19:12Z

src/phongo_compat.h

@@ -22,6 +22,10 @@
 #include <Zend/zend_string.h>
 #include <Zend/zend_portability.h>

+/* Include stdbool.h as it might not have been implicitly loaded yet */
+#include <stdbool.h>


Is this still relevant? I don't see bool used anywhere in this file. Just zend_bool.

You are right - this was left over from a previous version. I've removed it.

Correction: this is still necessary, at least on some versions of PHP. With PHP 8 or newer, zend_bool was changed to use a standard bool instead of a unsigned char (see php/php-src@4b77a15). This adds an include for stdbool.h in zend_types.h, which we include in phongo_compat.h.

We can either leave the include in place here (to ensure we have stdbool.h available when needed on PHP < 8) or include it explicitly in every file that makes use of a bool type. I'm fine leaving it in phongo_compat.h for the time being.

If php/php-src@4b77a15 is the first place zend_bool is defined, I don't understand how stdbool.h would not already be included by that point. We're already including Zend/zend_types.h at the top of this file.

I remember encountering this before but I never fully wrapped my head around it.

OK to leave as-is but we can consider opening a ticket to get to the bottom of it later.

For PHP versions before 8.0 zend_types.h does not include stdbool.h, triggering the error there. Given that, I think including it in phongo_compat.h makes sense.

src/BSON/Value.c

jmikola · 2023-06-15T03:20:26Z

src/MongoDB/ClientEncryption.c

@@ -921,7 +921,8 @@ static void phongo_clientencryption_encrypt(php_phongo_clientencryption_t* clien
 		goto cleanup;
 	}

-	if (!php_phongo_bson_value_to_zval(&ciphertext, zciphertext)) {
+	/** Use the legacy decoder to return PHP types instead of BSON types */
+	if (!phongo_bson_value_to_zval_legacy(&ciphertext, zciphertext)) {


☝️ Outstanding question

src/MongoDB/Session.c

src/phongo_bson.c

This includes a small change to return PHP objects for compatibility in ClientEncryption::decrypt and ClientEncryption::encrypt

With the new logic, debug output will always show raw BSON data and PHP data for documents and packed arrays. Serialised data will only contain raw BSON data. This also changes Int64 serialisation to always serialise as Int64 instances, regardless of value or platform.

Since the encrypted data will always be a BSON binary object, there is no reason to use the legacy function to avoid receiving a Document or PackedArray instance.

alcaeus · 2023-06-15T07:40:41Z

Ignoring Windows test failures for the time being. There were successful Windows builds on PHP 7.2 (which is usually the most critical version). That said, we'll have to revisit these tests as they are quite fragile (usually due to the same timeout when starting MongoDB).

alcaeus self-assigned this Apr 21, 2023

alcaeus commented Apr 21, 2023

View reviewed changes

jmikola reviewed Apr 24, 2023

View reviewed changes

jmikola reviewed Apr 25, 2023

View reviewed changes

src/BSON/Value.stub.php Outdated Show resolved Hide resolved

jmikola reviewed Apr 25, 2023

View reviewed changes

src/BSON/Value.c Show resolved Hide resolved

alcaeus force-pushed the phpc-2206-bson-value branch 2 times, most recently from 2bc452c to 0e42526 Compare April 26, 2023 06:38

alcaeus force-pushed the phpc-2206-bson-value branch from 6372cc3 to 836c393 Compare May 12, 2023 08:31

alcaeus force-pushed the phpc-2206-bson-value branch 2 times, most recently from bcd7cb3 to 5e0a49c Compare May 24, 2023 10:35

alcaeus marked this pull request as ready for review May 24, 2023 11:01

alcaeus requested a review from jmikola May 24, 2023 11:01

jmikola reviewed May 26, 2023

View reviewed changes

jmikola mentioned this pull request May 30, 2023

PHPC-2197: Support queryable encryption range indexes #1409

Merged

alcaeus force-pushed the phpc-2206-bson-value branch 2 times, most recently from af94ce0 to c9a0a80 Compare June 12, 2023 11:58

alcaeus requested a review from jmikola June 12, 2023 13:21

jmikola approved these changes Jun 15, 2023

View reviewed changes

alcaeus added 12 commits June 15, 2023 08:41

First implementation for MongoDB\BSON\Value

6a480eb

Add is* methods to MongoDB\BSON\Value

525f797

Extract functions to create BSON instances

9b630d5

Add getters for all BSON types

79cfe1c

Apply code review feedback

321ea81

Rename BSON factory functions

5218768

Return bool in BSON factories

cd3afaf

Clean up int64 compatibility macro includes

f9c2518

Use macro to throw unexpected BSON type exceptions

cf20261

Extract int64 tests for BSON value

dbd44ae

Add error test for BSON value factory

f10a45a

Implement debug output for BSON value instances

e168bc5

alcaeus added 22 commits June 15, 2023 08:41

Return bson_value_t instances in PackedArray::get

b0148c3

Return BSON type instances in bson_value_t

fe2dd61

This includes a small change to return PHP objects for compatibility in ClientEncryption::decrypt and ClientEncryption::encrypt

Remove obsolete php_phongo_bson_iter_to_zval function

6bcc5a3

Use PHP values in debug output

f6d5d30

Comprehensively test all BSON types

e0361a3

Implement serialisation of BSON Value instances

2847fc3

Drop isInt and getInt methods from BSON value class

a5fc863

Always return int64 instances in Value::getInt64

6ec050c

Create Int64 instances directly

c23eacb

Add comment explaining extra property in get_debug_info

0b23fb8

Clarify exception message when initialising Value instance

e17a012

Simplify invocations of get_properties_hash with string type

abc7d76

Handle zval conversion error in get_properties_hash

9e601ac

Add comment noting phongo_bson_value_to_zval will throw

04eb4db

Fix zval capitalisation

5715be7

Handle bson value conversion error on serialisation

b0e22e4

Document exceptions when converting values to zval

b8a9d50

Clean up unused macros

07e543a

Add missing __set_state methods

dc8fb6e

Test MongoDB\BSON\Value::__set_state()

7d8b495

Skip __set_state test on PHP 7.2

1e8c927

alcaeus force-pushed the phpc-2206-bson-value branch from c9a0a80 to d1eaf0f Compare June 15, 2023 06:41

alcaeus added 2 commits June 15, 2023 08:47

Change docblock to regular comment

6ff4c4c

Use new BSON value conversion function when encrypting data

d96bc6c

Since the encrypted data will always be a BSON binary object, there is no reason to use the legacy function to avoid receiving a Document or PackedArray instance.

alcaeus force-pushed the phpc-2206-bson-value branch from d1eaf0f to d96bc6c Compare June 15, 2023 06:47

alcaeus merged commit 358fd2f into mongodb:master Jun 15, 2023

alcaeus deleted the phpc-2206-bson-value branch June 15, 2023 07:40

alcaeus mentioned this pull request Jun 16, 2023

PHPC-2242, PHPC-2243, PHPC-2244: BSON class improvements #1435

Merged


		bson = bson_new_from_data(intern->value.value.v_doc.data, intern->value.value.v_doc.data_len);

		phongo_packedarray_new(return_value, bson, false);

PHPC-2206: Implement BSON\Value class #1412

PHPC-2206: Implement BSON\Value class #1412

Uh oh!

Conversation

alcaeus commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmikola 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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alcaeus commented Jun 15, 2023

Uh oh!

Uh oh!

alcaeus commented Apr 21, 2023 •

edited

Loading