PHPLIB-547: Add helpers to list database and collection names #758

alcaeus · 2020-06-29T12:05:11Z

https://jira.mongodb.org/browse/PHPLIB-547

jmikola · 2020-06-30T15:42:15Z

docs/reference/method/MongoDBClient-listDatabaseNames.txt

+Return Values
+-------------
+
+A generator containing the name of each database on the server.


Can we link to the PHP class with:

:php:`Generator <class.generator.php>`

Also, would it be more correct to write "a generator that yields that name of each..." or "a generator, which yields...", rather than "containing"?

Since this hasn't been changed yet, please see my other comment about only documenting a Traversable or Iterator. There may not be any reason to inform users that we're using Generators internally.

docs/reference/method/MongoDBDatabase-listCollectionNames.txt

jmikola · 2020-06-30T15:47:41Z

src/Client.php

+     * @throws InvalidArgumentException for parameter/option parsing errors
+     * @throws DriverRuntimeException for other driver errors (e.g. connection errors)
+     */
+    public function listDatabaseNames(array $options = [])


Do we want to use : Generator as a return type hint here, or are we putting off adding those to our public API until we do so across the board?

I've started adding them to new methods, but forgot them in this case. I added type hints now.

Just left a related comment about this on the Database::listCollectionNames method. Perhaps it makes sense to only document that we return a Traversable or Iterator.

src/Command/ListCollections.php

src/Operation/ListCollectionNames.php

tests/SpecTests/RetryableReadsSpecTest.php

src/Command/ListCollections.php

src/Command/ListDatabases.php

jmikola · 2020-07-01T14:29:58Z

src/Command/ListDatabases.php

+            throw new UnexpectedValueException('listDatabases command did not return a "databases" array');
+        }
+
+        // Using array_column here emulates nameOnly behavior for servers that don't support it and ensures a consistent return type


This is what I'd suggest moving to ListDatabaseNames, which currently just yields from this result directly. I suppose doing yield from array_column($result['databases'], 'name') would be cheaper than using the CallbackIterator, especially since the result is already an array (unlike a potentially large listCollections response, where iterators may conserve memory).

array_column won't work here since the ListCollections command returns a CachingIterator over the result cursor, which is why I added the iterator on top of that.

We're talking about ListDatabases here, though, which always returns an array. Am I misunderstanding?

Apologies, I got confused. ListDatabaseNames uses array_column since we're already dealing with an array.

jmikola · 2020-07-01T14:32:38Z

src/Database.php

+     *
+     * @see ListCollectionNames::__construct() for supported options
+     * @param array $options
+     * @return Generator


In the interest of a conservative public API, I wonder if we should just document that we return a Traversable or Iterator. This would be similar to how I only documented a generic CollectionInfoIterator return type for the listCollections method. Granted, that was the abstract the user from thinking about legacy and cursor-based responses, but it was also suggesting that the only thing they need care about was that the current element is a CollectionInfo object.

Generator has additional methods like send() that I expect would have no use, so the fact that we happen to use a generator here seems like an implementation detail best not revealed.

Generators can only be iterated once and can't be rewound, so I think the distinction is important to make sure people don't expect to repeatedly iterate over the same list of names.

Ah, good point. That sounds like a possible deviation from the rewindable iterators returned by listDatabases and listCollections, though. Is it possible to wrap the generator with our CachingIterator. That would save users from having to ever care about this.

If we do make this change, I would then suggest we document an Iterator return value, since Traversable is not inherently rewindable.

Two side notes:

52df7a1 refactored our CachingIterator to use IteratorIterator instead of Generators, but I believe it forgot to update comments in the file that still refer to yield. I opened a PR to correct this: Remove mention of Generator in CachingIterator comments #763

I just found https://github.com/bpolaszek/rewindable-generator, which is a project that addresses this issue with Generators, but it unfortunately uses SPL's CachingIterator, which has its own problems (see this blog comment, which I found a while ago in this tweet). As such, it actually breaks if you don't fully iterator the Generator before rewinding, which is something our own CachingIterator addresses (an early rewind() exhausts the inner iterator).

Good point. ListCollectionNames now returns the CallbackIterator directly since we already have a CachingIterator below. ListDatabaseNames returns an ArrayIterator to iterate over the array of names. This can then always be changed to return a CachingIterator or other iterator should the listDatabases command ever be changed to returning a cursor.

src/Operation/ListDatabaseNames.php

tests/Command/ListCollectionsTest.php

jmikola · 2020-07-01T14:43:15Z

src/Client.php

+     * @throws InvalidArgumentException for parameter/option parsing errors
+     * @throws DriverRuntimeException for other driver errors (e.g. connection errors)
+     */
+    public function listDatabaseNames(array $options = [])


Just left a related comment about this on the Database::listCollectionNames method. Perhaps it makes sense to only document that we return a Traversable or Iterator.

jmikola · 2020-07-01T14:43:55Z

docs/reference/method/MongoDBClient-listDatabaseNames.txt

+Return Values
+-------------
+
+A generator containing the name of each database on the server.


Since this hasn't been changed yet, please see my other comment about only documenting a Traversable or Iterator. There may not be any reason to inform users that we're using Generators internally.

alcaeus requested a review from jmikola June 29, 2020 12:05

alcaeus self-assigned this Jun 29, 2020

jmikola reviewed Jun 30, 2020

View reviewed changes

jmikola reviewed Jul 1, 2020

View reviewed changes

alcaeus force-pushed the phplib-547 branch from 726ccb5 to fba9d10 Compare July 2, 2020 07:07

alcaeus requested a review from jmikola July 2, 2020 07:07

jmikola mentioned this pull request Jul 2, 2020

Remove mention of Generator in CachingIterator comments #763

Merged

alcaeus force-pushed the phplib-547 branch 2 times, most recently from 802f8d4 to 5aac54c Compare July 3, 2020 11:50

jmikola approved these changes Jul 3, 2020

View reviewed changes

alcaeus added 2 commits July 7, 2020 09:08

PHPLIB-547: Implement listDatabaseNames helper

28d5377

PHPLIB-547: Implement listCollectionNames helper

2e39482

alcaeus force-pushed the phplib-547 branch from 5aac54c to 2e39482 Compare July 7, 2020 07:08

alcaeus merged commit de6950b into mongodb:v1.7 Jul 7, 2020

alcaeus deleted the phplib-547 branch July 7, 2020 07:34

jmikola mentioned this pull request Jul 16, 2020

PHPLIB-547: Fix links to new enumeration methods #770

Merged

PHPLIB-547: Add helpers to list database and collection names #758

PHPLIB-547: Add helpers to list database and collection names #758

Uh oh!

Conversation

alcaeus commented Jun 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

jmikola Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola Jul 2, 2020 •

edited

Loading