Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

BSON class implementations #1387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alcaeus merged 45 commits into mongodb:master from alcaeus:feature/phpc-326-bson-classes
Dec 16, 2022

Conversation

@alcaeus
Copy link
Member

@alcaeus alcaeus commented Nov 29, 2022
edited
Loading

PHPC-326, PHPC-324

This PR adds new classes to represent BSON arrays and documents to the MongoDB\BSON namespace. Both classes are backed by a bson_t and are currently read-only. The interfaces for both classes differ a bit, mainly due to constraints in how libbson handles bson_t:

  • Document can be constructed from a JSON string, but ArrayList can't as libbson doesn't offer any functionality for that.
  • Document has accessors to get canonical and extended JSON strings for a document, but ArrayList doesn't as libbson doesn't support that.
  • Both classes have a private constructor, as there's no point in creating an empty instance. This may be relaxed in the future once we allow editing BSON structures.

To iterate a BSON structure, the new Iterator class was introduced. Again, this class can't be instantiated directly (mainly due to ZPP not handling union types properly), but will be returned as part of the above classes' implementation of IteratorAggregate.

To receive BSON instances, the new typeMap option bson was introduced. It can be used on the root level, but also for the document and array types, as well as in field path mappings for specific fields. The most common use case will be for root to be a BSON structure. An Iterator instance will always return a literal value (for strings, numbers, etc.), a BSON type (for object IDs, binary objects, etc.), or another Document or ArrayList instance depending when iterating.

Note that the introduction of the Document class makes the MongoDB\BSON\toPHP functions (and everything related to them) obsolete. I would suggest deprecating those in a separate pull request once this is merged.

Todo:

  • Check memory handling (ensure we don't accidentally free a bson_t while there's still an Iterator instance around)
  • Allow accessing offsets directly (public function get(string $key) in the Document class and equivalent in ArrayList)
  • Define documentation requirements
  • Change get() in BSON classes to throw on non-existent keys (OutOfBoundsException vs. RuntimeException)
    • ArrayList::get()
    • Document::get()
    • Create ticket for future optimisation: memoize sequential has()/get() calls
  • Return type of Iterator::key(): split tests between PHP 7 (returns null) and PHP 8 (throws TypeError)
  • Validate strings using bson_validate_utf8() before returning them
  • Default root type map key to be array in ArrayList::toPHP()
  • Remove length field in serialised output
  • Refactorings
    • Use short array syntax throughout tests
    • Rename odm field to odm_ce in php_phongo_bson_state
    • Rename classname variable to type in php_phongo_bson_state_parse_type
  • Expand iterator tests to comprehensively test types:
    • Nested ArrayList
    • Nested Document
    • stdClass
    • Packed arrays
    • int
    • string
    • ObjectId
  • Update Document::fromBSON() tests with better explanations
  • Comprehensively test invalid payload in serialisation tests for __set_state(), __unserialize(), and unserialize()

  • Benchmark Iterator performance when using iterator handlers directly (instead of Iterator interface)
    • Check expectation that PHP prioritises handlers over the interface

@alcaeus alcaeus force-pushed the feature/phpc-326-bson-classes branch 3 times, most recently from c4c86f6 to fa126cb Compare November 30, 2022 09:11
@alcaeus alcaeus self-assigned this Nov 30, 2022
@alcaeus alcaeus marked this pull request as ready for review November 30, 2022 10:39
@alcaeus alcaeus force-pushed the feature/phpc-326-bson-classes branch 3 times, most recently from 89fc705 to 32f9a56 Compare December 1, 2022 14:17
alcaeus added 16 commits December 6, 2022 08:19
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This completes my first pass through all of the PR. Two additional notes that I couldn't leave as inline comments follow.

I saw that tests/bson/bson-document-getIterator-001.phpt was rendered as a binary file because you included the raw debug output for a Document (string(18) "��foo�bar") instead of using a wildcard pattern. That was likely an oversight you can fix.

Separately, this PR reminded me of PHPC-889. PHPC previously decoded a Javascript scope lazily when it was accessed, which deferred decoding errors. With that change (in 1.7.0), the scope bson_t was converted to a zval when the Javascript instance was created. I think that's fine to leave as-is because Javascript scopes are deprecated and aren't a performance concern -- but I wanted to make a note of it since it's somewhat related to the work we're doing in this PR re: lazy BSON access.

alcaeus added 22 commits December 15, 2022 14:02
The previous method relied on the type system of PHP to throw an exception when accessing the key of an exhausted iterator, but this does not work on non-debug versions of PHP.
@alcaeus alcaeus force-pushed the feature/phpc-326-bson-classes branch from afd3feb to 73f444f Compare December 15, 2022 13:21
@jmikola jmikola self-requested a review December 15, 2022 14:47
@alcaeus alcaeus force-pushed the feature/phpc-326-bson-classes branch from 73f444f to 52c6c72 Compare December 15, 2022 14:49
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alcaeus alcaeus merged commit a237193 into mongodb:master Dec 16, 2022
@alcaeus alcaeus deleted the feature/phpc-326-bson-classes branch December 16, 2022 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmikola jmikola jmikola approved these changes

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /