Skip to content

PHPLIB-1206 Allow global registration of GridFS buckets with gridfs:// protocol #1138

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
merged 1 commit into from
Dec 18, 2023

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jul 21, 2023

Fix PHPLIB-1206
Fix #1137

Allows to register an alias for the stream wrapper. File urls looks like:

gridfs://<bucket-alias>/<filename>

Where <bucket-alias> is an alias for the bucket

$client = new Client();
$bucket = $client->selectDatabase('test_db')->selectGridFSBucket();
$bucket->registerGlobalStreamWrapperAlias('mybucket');

file_put_contents("gridfs://mybucket/filename.txt", 'Hello GridFS!');

if (file_exists("gridfs://mybucket/filename.txt")) {
    echo file_get_contents("gridfs://mybucket/filename.txt") . PHP_EOL;
}

Todo:

  • Add reference doc for the method Bucket::registerGlobalStreamWrapperAlias

@GromNaN GromNaN requested a review from jmikola July 24, 2023 11:25
@GromNaN GromNaN marked this pull request as ready for review July 24, 2023 12:56
@GromNaN GromNaN marked this pull request as draft July 25, 2023 06:47
@alcaeus
Copy link
Member

alcaeus commented Nov 27, 2023

A couple of high level questions about the API. Let's take this code from the PR description:

$bucket = $client->selectDatabase('test_db')->selectGridFSBucket();
$bucket->registerAsDefaultStreamWrapper();

file_put_contents("gridfs://test_db/fs.files/filename.txt", 'Hello GridFS!');

If we're already registering a bucket as the default stream wrapper, why does the URI still contain the database and bucket names? The bucket instance is tied to a client, database, and bucket, so I'd expect to use something like this:

file_put_contents('gridfs://filename.txt', 'Hello GridFS!');

This might create conflicts with us using GridFS internally. However, we could work around that by allowing to registerer a new stream wrapper for each bucket:

$client->selectDatabase('test_db')->selectGridFSBucket()->registerStreamWrapper('foo');
$client->selectDatabase('test_db')->selectGridFSBucket(['name' => 'cache'])->registerStreamWrapper('cache');

file_put_contents('foo://filename.txt', 'Hello GridFS!'); // Writes to the fs.files collection
file_put_contents('cache://filename.txt', 'Hello GridFS!'); // Writes to the cache.files collection

Not only would this make accessing files easier, but it would also allow for using more than a single bucket if users so desire.

@GromNaN
Copy link
Member Author

GromNaN commented Nov 27, 2023

I'm thinking to an other path pattern: gridfs://<bucket-alias>/<filename>

  • <bucket-alias>: name associated with a closure that get the context from the stream_open data ($path, $mode, $options).
  • <filename> : the path associated to the file

The context resolver will be a map instead of a single closure.

We can reuse the same StreamWrapper class and the same protocol gridfs.

@GromNaN GromNaN force-pushed the PHPLIB-1206 branch 2 times, most recently from 04a1cc8 to 6ebcc42 Compare November 28, 2023 21:50
@GromNaN GromNaN changed the title PHPLIB-1206 Add default bucket for GridFS StreamWrapper PHPLIB-1206 Allow global registration of GridFS buckets with gridfs:// protocol Nov 30, 2023
@GromNaN GromNaN changed the title PHPLIB-1206 Allow global registration of GridFS buckets with gridfs:// protocol PHPLIB-1206 Allow global registration of GridFS buckets with gridfs:// protocol Nov 30, 2023
@GromNaN GromNaN force-pushed the PHPLIB-1206 branch 2 times, most recently from ed857c2 to cf4c665 Compare December 6, 2023 13:55
@GromNaN GromNaN marked this pull request as ready for review December 6, 2023 15:07
@GromNaN GromNaN force-pushed the PHPLIB-1206 branch 5 times, most recently from 8891ad0 to 41c915b Compare December 13, 2023 14:32
@GromNaN GromNaN force-pushed the PHPLIB-1206 branch 2 times, most recently from 94e3848 to 002e0d4 Compare December 13, 2023 16:20
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.

Two outstanding comments about url_stat(), which GitHub thinks are outdated.

@GromNaN GromNaN requested review from jmikola and alcaeus December 15, 2023 09:56
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.

I'll defer to you and Andreas to decide if disableMD5 should be changed.

@jmikola
Copy link
Member

jmikola commented Dec 15, 2023

This CI failure looks relevant:

1) MongoDB\Tests\GridFS\StreamWrapperFunctionalTest::testOpenSpecificRevision
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'version 1'
+'version 0'

/home/runner/work/mongo-php-library/mongo-php-library/tests/GridFS/StreamWrapperFunctionalTest.php:300

I noticed some other CI failures related to Atlas Search spec tests failing to match error messages, but that's probably related to DRIVERS-2794 as it only fails on mongodb-latest.


// Insert 3 revisions, wait 1ms between each to ensure they have different uploadDate
file_put_contents($filename, 'version 0');
usleep(1000);
Copy link
Member Author

Choose a reason for hiding this comment

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

@jmikola Added this 1ms delay to fix #1138 (comment)

@GromNaN GromNaN merged commit b1147cd into mongodb:master Dec 18, 2023
@GromNaN GromNaN deleted the PHPLIB-1206 branch December 18, 2023 10:01
alcaeus added a commit to alcaeus/mongo-php-library that referenced this pull request Jan 15, 2024
* master:
  PHPLIB-1323 Implement `unlink` for GridFS stream wrapper (mongodb#1206)
  PHPLIB-1330: Sync tests for failCommand errorLabels reqs (mongodb#1214)
  PHPLIB-1246: Test PHP 8.3 on Evergreen (mongodb#1213)
  PHPLIB-1324 Implement `rename` for GridFS stream wrapper  (mongodb#1207)
  PHPLIB-1248 Add examples on GridFS (mongodb#1196)
  Deprecate setting GridFS disableMD5 to false explicitly (mongodb#1205)
  PHPLIB-1326: Use more permissive top-level runOnRequirements (mongodb#1210)
  PHPLIB-1206 Add bucket alises for context resolver using GridFS StreamWrapper (mongodb#1138)
  Bump actions/upload-artifact from 3 to 4 (mongodb#1208)
  PHPLIB-1275: Replace apiargs usage in docs with extracts (mongodb#1203)
  Fix title formatting in Client::removeSubscriber() docs (mongodb#1204)
  PHPLIB-1304: Pull mongohouse image from ECR repo (mongodb#1202)
  Fix evergreen failures (mongodb#1200)
  Enable workflows to run for GitHub Merge Queue (mongodb#1199)
  PHPLIB-1313 Ensure the GridFS stream is saved when the script ends (mongodb#1197)
  PHPLIB-1309 Add addSubscriber/removeSubscriber to Client class to ease configuration (mongodb#1195)
  Master is now 1.18-dev
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The StreamWrapper class is currently unusable without the bucket
3 participants