Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PHPC-2412: Deprecate CursorId class #1616

Merged
merged 10 commits into from
Sep 5, 2024
Merged

PHPC-2412: Deprecate CursorId class #1616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
baf9064
PHPC-2412: Deprecate CursorId class
alcaeus Jul 25, 2024
1b5c2e9
Use zend_bool instead of bool
alcaeus Jul 25, 2024
9e5f614
Remove fixed length assertion in test
alcaeus Jul 25, 2024
6a7187d
Correctly set default parameter value
alcaeus Jul 25, 2024
6143507
Remove int cast to avoid warnings on 32-bit platforms
alcaeus Jul 25, 2024
48da73c
Fix wrong expectation for CursorId debug format
alcaeus Sep 2, 2024
d3ea5dd
Use new cursor ID logic in tests where possible
alcaeus Sep 4, 2024
9aac2e8
Remove deprecation comment from CursorId stub
alcaeus Sep 4, 2024
a3cbc53
Restore cursorid serialisation test
alcaeus Sep 5, 2024
62a3392
Restore original class initialisation order
alcaeus Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/MongoDB/Cursor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,22 @@ static PHP_METHOD(MongoDB_Driver_Cursor, toArray)
static PHP_METHOD(MongoDB_Driver_Cursor, getId)
{
php_phongo_cursor_t* intern;
zend_bool asInt64 = false;

intern = Z_CURSOR_OBJ_P(getThis());

PHONGO_PARSE_PARAMETERS_NONE();
PHONGO_PARSE_PARAMETERS_START(0, 1)
Z_PARAM_OPTIONAL
Z_PARAM_BOOL(asInt64)
PHONGO_PARSE_PARAMETERS_END();

php_phongo_cursor_id_new_from_id(return_value, mongoc_cursor_get_id(intern->cursor));
if (asInt64) {
phongo_int64_new(return_value, mongoc_cursor_get_id(intern->cursor));
} else {
php_error_docref(NULL, E_DEPRECATED, "The method \"MongoDB\\Driver\\Cursor::getId\" will no longer return a \"MongoDB\\Driver\\CursorId\" instance in the future. Pass \"true\" as argument to change to the new behavior and receive a \"MongoDB\\BSON\\Int64\" instance instead.");

php_phongo_cursor_id_new_from_id(return_value, mongoc_cursor_get_id(intern->cursor));
}
}

/* Returns the Server object to which this cursor is attached */
Expand Down
8 changes: 7 additions & 1 deletion src/MongoDB/Cursor.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@ public function current(): array|object|null {}
public function current() {}
#endif

final public function getId(): CursorId {}
#if PHP_VERSION_ID >= 80000
/** @tentative-return-type */
final public function getId(bool $asInt64 = false): CursorId|\MongoDB\BSON\Int64 {}
#else
/** @return CursorId|\MongoDB\BSON\Int64 */
final public function getId(bool $asInt64 = false) {}
#endif

final public function getServer(): Server {}

Expand Down
7 changes: 6 additions & 1 deletion src/MongoDB/CursorInterface.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,13 @@

interface CursorInterface extends \Traversable
{
#if PHP_VERSION_ID >= 80000
/** @tentative-return-type */
public function getId(): CursorId;
public function getId(): CursorId|\MongoDB\BSON\Int64;
Copy link
Member

Choose a reason for hiding this comment

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

Is this worth using a use statement, or do those not work in stubs? I feel like we've discussed this before and there's probably a reason none of the existing stubs have use statements.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, gen_stub.php does not support this (at least in version 8.2 which I currently use to regenerate arginfo files):

❯❯❯ ./build/gen_stub.php
In ./src/MongoDB/Cursor.stub.php:
Unexpected node Stmt_Use

#else
/** @return CursorId|\MongoDB\BSON\Int64 */
public function getId();
#endif

/** @tentative-return-type */
public function getServer(): Server;
Expand Down
20 changes: 18 additions & 2 deletions src/MongoDB/CursorInterface_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 21 additions & 2 deletions src/MongoDB/Cursor_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions tests/cursor/cursor-session-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ $iterator = new IteratorIterator($cursor);
$iterator->rewind();
$iterator->next();

printf("Cursor ID is zero: %s\n", (string) $cursor->getId() === '0' ? 'yes' : 'no');
printf("Cursor ID is zero: %s\n", $cursor->getId(true) == 0 ? 'yes' : 'no');
Copy link
Member

Choose a reason for hiding this comment

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

Note that if $cursor->getId(true) return null, this condition null == 0 would be true. Should you test the type of the returned value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I'll double check this, as it may have been related to the casting issues fixed in #1617.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at PHP_METHOD(MongoDB_Driver_Cursor, getId) above, would Cursor::getId() ever return null? It looks like the return value would either be a CursorId or Int64.

The return type in Cursor.stub.php also wouldn't allow a null value (at least for PHP 8.0+).

Copy link
Member Author

Choose a reason for hiding this comment

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

TL;DR: this is not an issue, because getId() would never return null, and even if it did the comparison logic we rely on for Int64 instances handles this correctly.

The dirty details: before the change to getId(true), we'd always get a CursorId instance, which does not implement cast or comparison handlers, so the only sensible way to assert on it was by casting it to a string and doing a typesafe comparison.

Now that we return an Int64 instance, we can rely on PHP's internal logic for checking equality (not identity) in objects. Note that when using ===, there's an early exit in zend_is_identical if the types do not match. So $int64 === 0 will always evaluate to false based on the different types. == on the other hand calls zend_is_equal, which in turn calls zend_compare, which relies on an object's compare handler, and thus defers to the logic we implemented for comparing Int64 instances with other values (and importantly does not include a cast to avoid issues on 32-bit platforms).

I did not add a specific test for the case you mentioned, as zend_compare does not call into the compare handler when comparing an object to null, instead handling it specifically to ensure correct behaviour.

var_dump($cursor);

$iterator->next();
Expand All @@ -35,7 +35,7 @@ $iterator->next();
* is exhausted. While this is primarily done to ensure implicit sessions for
* command cursors are returned to the pool ASAP, it also applies to explicit
* sessions. */
printf("\nCursor ID is zero: %s\n", (string) $cursor->getId() === '0' ? 'yes' : 'no');
printf("\nCursor ID is zero: %s\n", $cursor->getId(true) == 0 ? 'yes' : 'no');
var_dump($cursor);

?>
Expand Down
4 changes: 2 additions & 2 deletions tests/cursor/cursor-session-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ $iterator->next();
/* Implicit sessions for query cursors are never exposed to PHPC, as they are
* handled internally by libmongoc. Cursor debug ouput should never report such
* sessions. */
printf("Cursor ID is zero: %s\n", (string) $cursor->getId() === '0' ? 'yes' : 'no');
printf("Cursor ID is zero: %s\n", $cursor->getId(true) == 0 ? 'yes' : 'no');
var_dump($cursor);

$iterator->next();

printf("\nCursor ID is zero: %s\n", (string) $cursor->getId() === '0' ? 'yes' : 'no');
printf("\nCursor ID is zero: %s\n", $cursor->getId(true) == 0 ? 'yes' : 'no');
var_dump($cursor);

?>
Expand Down
4 changes: 2 additions & 2 deletions tests/cursor/cursor-session-003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ $iterator = new IteratorIterator($cursor);
$iterator->rewind();
$iterator->next();

printf("Cursor ID is zero: %s\n", (string) $cursor->getId() === '0' ? 'yes' : 'no');
printf("Cursor ID is zero: %s\n", $cursor->getId(true) == 0 ? 'yes' : 'no');
var_dump($cursor);

$iterator->next();
Expand All @@ -39,7 +39,7 @@ $iterator->next();
* is exhausted. While this is primarily done to ensure implicit sessions for
* command cursors are returned to the pool ASAP, it also applies to explicit
* sessions. */
printf("\nCursor ID is zero: %s\n", (string) $cursor->getId() === '0' ? 'yes' : 'no');
printf("\nCursor ID is zero: %s\n", $cursor->getId(true) == 0 ? 'yes' : 'no');
var_dump($cursor);

?>
Expand Down
4 changes: 2 additions & 2 deletions tests/cursor/cursor-session-004.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ $iterator = new IteratorIterator($cursor);
$iterator->rewind();
$iterator->next();

printf("Cursor ID is zero: %s\n", (string) $cursor->getId() === '0' ? 'yes' : 'no');
printf("Cursor ID is zero: %s\n", $cursor->getId(true) == 0 ? 'yes' : 'no');
var_dump($cursor);

$iterator->next();
Expand All @@ -38,7 +38,7 @@ $iterator->next();
* libmongoc, PHPC-1152 emulates its own implicit sessions for command cursors
* in order to ensure that command cursors always share the same session as the
* originating command. */
printf("\nCursor ID is zero: %s\n", (string) $cursor->getId() === '0' ? 'yes' : 'no');
printf("\nCursor ID is zero: %s\n", $cursor->getId(true) == 0 ? 'yes' : 'no');
var_dump($cursor);

?>
Expand Down
2 changes: 1 addition & 1 deletion tests/cursor/cursor-tailable_error-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ echo throws(function() use ($manager) {
if ($numAwaitAttempts === 5) {
$cursor->getServer()->executeCommand(DATABASE_NAME, new MongoDB\Driver\Command([
'killCursors' => COLLECTION_NAME,
'cursors' => [ $cursor->getId() ],
'cursors' => [ $cursor->getId(true) ],
]));
}

Expand Down
45 changes: 45 additions & 0 deletions tests/cursor/cursorid-getId-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
--TEST--
MongoDB\Driver\Cursor::getId
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php skip_if_not_live(); ?>
<?php skip_if_not_clean(); ?>
--FILE--
<?php

require_once __DIR__ . "/../utils/basic.inc";

$manager = create_test_manager();

$bulk = new MongoDB\Driver\BulkWrite();
$bulk->insert(['_id' => 1]);
$bulk->insert(['_id' => 2]);
$bulk->insert(['_id' => 3]);
$manager->executeBulkWrite(NS, $bulk);

$cursor = $manager->executeQuery(NS, new MongoDB\Driver\Query([], ['batchSize' => 2]));

var_dump($cursor->getId());
var_dump($cursor->getId(false));
var_dump($cursor->getId(true));

?>
===DONE===
<?php exit(0); ?>
--EXPECTF--
Deprecated: MongoDB\Driver\Cursor::getId(): The method "MongoDB\Driver\Cursor::getId" will no longer return a "MongoDB\Driver\CursorId" instance in the future. Pass "true" as argument to change to the new behavior and receive a "MongoDB\BSON\Int64" instance instead. in %s
object(MongoDB\Driver\CursorId)#%d (%d) {
["id"]=>
%rint\(%d\)|string\(%d\) "%d"%r
}

Deprecated: MongoDB\Driver\Cursor::getId(): The method "MongoDB\Driver\Cursor::getId" will no longer return a "MongoDB\Driver\CursorId" instance in the future. Pass "true" as argument to change to the new behavior and receive a "MongoDB\BSON\Int64" instance instead. in %s
object(MongoDB\Driver\CursorId)#%d (%d) {
["id"]=>
%rint\(%d\)|string\(%d\) "%d"%r
}
object(MongoDB\BSON\Int64)#%d (%d) {
["integer"]=>
string(%d) "%d"
}
===DONE===
1 change: 1 addition & 0 deletions tests/cursorid/cursorid-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ hex_dump(fromPHP(['cid' => $cursorId]));
===DONE===
<?php exit(0); ?>
--EXPECTF--
Deprecated: MongoDB\Driver\Cursor::getId(): The method "MongoDB\Driver\Cursor::getId" will no longer return a "MongoDB\Driver\CursorId" instance in the future. Pass "true" as argument to change to the new behavior and receive a "MongoDB\BSON\Int64" instance instead. in %s
jmikola marked this conversation as resolved.
Show resolved Hide resolved
0 : 12 00 00 00 12 63 69 64 00 %x %x %x %x %x %x %x [.....cid.%s]
10 : %x 00 [%s.]
===DONE===
3 changes: 2 additions & 1 deletion tests/cursorid/cursorid-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ printf("Killed expected cursor: %s\n", (string) $cursorId === (string) $result->
?>
===DONE===
<?php exit(0); ?>
--EXPECT--
--EXPECTF--
Deprecated: MongoDB\Driver\Cursor::getId(): The method "MongoDB\Driver\Cursor::getId" will no longer return a "MongoDB\Driver\CursorId" instance in the future. Pass "true" as argument to change to the new behavior and receive a "MongoDB\BSON\Int64" instance instead. in %s
Killed 1 cursor(s)
Killed expected cursor: yes
alcaeus marked this conversation as resolved.
Show resolved Hide resolved
===DONE===
17 changes: 6 additions & 11 deletions tests/functional/cursorid-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,17 @@ $query = new MongoDB\Driver\Query(array(), array(

$cursor = $manager->executeQuery(NS, $query);

$cursorid = $cursor->getId();
$s1 = (string)$cursorid;
var_dump(
$cursorid,
$s1
);
var_dump($s1 > 0);
$cursorid = $cursor->getId(true);
var_dump($cursorid);
var_dump($cursorid != 0);

?>
===DONE===
<?php exit(0); ?>
--EXPECTF--
object(MongoDB\Driver\CursorId)#%d (%d) {
["id"]=>
%rint\(\d+\)|string\(\d+\) "\d+"%r
object(MongoDB\BSON\Int64)#%d (%d) {
["integer"]=>
string(%d) "%d"
}
string(%d) "%d"
bool(true)
===DONE===