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

Custom _id for GridFS documents (#2487) #2489

Merged
merged 11 commits into from
Feb 24, 2023
Merged

Custom _id for GridFS documents (#2487) #2489

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
eeedec2
Fix deprecation notices in CommandLogger (#2485)
IonBazan Nov 30, 2022
8675eea
Add _id and disableMD5 to UploadOptions
andrey-tech Dec 12, 2022
bcf16cf
Improve UploadOptions
andrey-tech Dec 12, 2022
2fc3250
Set mongodb/mongodb: ^1.4.0
andrey-tech Dec 12, 2022
9c679cd
Rename _id to id in UploadOptions, add two tests
andrey-tech Dec 13, 2022
b9520fd
Change prepareOptions() and test
andrey-tech Dec 21, 2022
f074878
Remove disableMD5 from UploadOptions, set mongodb/mongodb ^1.15.0
andrey-tech Feb 18, 2023
7215a9f
Relax mongodb/mongodb requirements
IonBazan Feb 21, 2023
933357d
Merge branch '2.5.x' into issue-2487
IonBazan Feb 21, 2023
4a2c346
Relax mongodb/mongodb requirements
IonBazan Feb 21, 2023
f9ee696
Remove MongoDB 4.2 from testing
alcaeus Feb 21, 2023
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
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"doctrine/persistence": "^2.4 || ^3.0",
"friendsofphp/proxy-manager-lts": "^1.0",
"jean85/pretty-package-versions": "^1.3.0 || ^2.0.1",
"mongodb/mongodb": "^1.2.0",
"mongodb/mongodb": "^1.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

Might as well bump this to 1.15 while you're at it. 1.4 is ancient and no longer supported, we should encourage people to upgrade :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@malarzm Do you mind to set ^1.15.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set mongodb/mongodb to ^1.15.0.

"psr/cache": "^1.0 || ^2.0 || ^3.0",
"symfony/console": "^3.4 || ^4.1 || ^5.0 || ^6.0",
"symfony/deprecation-contracts": "^2.2 || ^3.0",
Expand Down
6 changes: 3 additions & 3 deletions lib/Doctrine/ODM/MongoDB/APM/CommandLogger.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,12 @@ public function unregister(): void
$this->registered = false;
}

public function commandStarted(CommandStartedEvent $event)
public function commandStarted(CommandStartedEvent $event): void
{
$this->startedCommands[$event->getRequestId()] = $event;
}

public function commandSucceeded(CommandSucceededEvent $event)
public function commandSucceeded(CommandSucceededEvent $event): void
{
$commandStartedEvent = $this->findAndRemoveCommandStartedEvent($event->getRequestId());
if (! $commandStartedEvent) {
Expand All @@ -59,7 +59,7 @@ public function commandSucceeded(CommandSucceededEvent $event)
$this->logCommand(Command::createForSucceededCommand($commandStartedEvent, $event));
}

public function commandFailed(CommandFailedEvent $event)
public function commandFailed(CommandFailedEvent $event): void
{
$commandStartedEvent = $this->findAndRemoveCommandStartedEvent($event->getRequestId());
if (! $commandStartedEvent) {
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
*
* @internal
*
* @method ClassMetadata[] getAllMetadata()
* @method list<ClassMetadata> getAllMetadata()
* @method ClassMetadata[] getLoadedMetadata()
* @method ClassMetadata getMetadataFor($className)
*/
Expand Down
10 changes: 10 additions & 0 deletions lib/Doctrine/ODM/MongoDB/Repository/DefaultGridFSRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ private function getDocumentBucket(): Bucket

/**
* @psalm-return array{
* _id?: mixed,
* disableMD5?: bool,
* chunkSizeBytes?: int,
* metadata?: object
* }
Expand All @@ -108,6 +110,14 @@ private function prepareOptions(?UploadOptions $uploadOptions = null): array
$chunkSizeBytes = $uploadOptions->chunkSizeBytes ?: $this->class->getChunkSizeBytes();
$options = [];

if ($uploadOptions->id !== null) {
$options['_id'] = $uploadOptions->id;
}

if ($uploadOptions->disableMD5 !== null) {
$options['disableMD5'] = $uploadOptions->disableMD5;
}

if ($chunkSizeBytes !== null) {
$options['chunkSizeBytes'] = $chunkSizeBytes;
}
Expand Down
10 changes: 8 additions & 2 deletions lib/Doctrine/ODM/MongoDB/Repository/UploadOptions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,15 @@

final class UploadOptions
{
/** @var object|null */
public $metadata;
/** @var mixed */
public $id;

/** @var bool|null */
public $disableMD5;
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a specific use-case for the disableMD5 option? The reason I omitted this was that the specification marks the option as deprecated and it will be removed from the PHP driver in the next major release. I would prefer not adding it in now, as we'd have to do another BC break for 3.x then (as opposed to breaking it once for people migrating to ODM 2.0).

Copy link
Member

Choose a reason for hiding this comment

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

This one's on me (#2487 (comment)) I didn't know it's deprecated :)

Copy link
Member

Choose a reason for hiding this comment

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

Hehe, yes. We deprecated it since using MD5 is forbidden when being compliant with FIPS, and personally I wouldn't rely on MD5 for checksums anymore. @andrey-tech do you mind dropping it? I don't think we should introduce the option at this point, especially since we haven't had anyone ask for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alcaeus I don't mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@malarzm Do you mind dropping it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrey-tech since it's deprecated and it was not added on purpose, we should not add it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deprecated option disableMD5 has been removed from this PR.


/** @var int|null */
public $chunkSizeBytes;

/** @var object|null */
public $metadata;
}
8 changes: 1 addition & 7 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,6 @@ parameters:
count: 1
path: tests/Doctrine/ODM/MongoDB/Tests/DocumentRepositoryTest.php

# 'strategy' offset is defined as nullable, but here there is no check here
-
message: "#^Offset 'strategy' does not exist on array\\{\\}\\|array\\{type\\?\\: string, fieldName\\: string, name\\: string, isCascadeRemove\\: bool, isCascadePersist\\: bool, isCascadeRefresh\\: bool, isCascadeMerge\\: bool, isCascadeDetach\\: bool, \\.\\.\\.\\}\\.$#"
count: 1
path: lib/Doctrine/ODM/MongoDB/UnitOfWork.php

# When iterating over SimpleXMLElement, we cannot know the key values
-
message: "#^Parameter \\#2 \\$mapping of method Doctrine\\\\ODM\\\\MongoDB\\\\Mapping\\\\Driver\\\\XmlDriver\\:\\:addFieldMapping\\(\\) expects array#"
Expand Down Expand Up @@ -177,7 +171,7 @@ parameters:
# compatibility layer for doctrine/persistence ^2.4 || ^3.0
-
message: "#.*#"
count: 3
count: 1
path: lib/Doctrine/ODM/MongoDB/Event/OnClearEventArgs

-
Expand Down
46 changes: 46 additions & 0 deletions tests/Doctrine/ODM/MongoDB/Tests/Repository/DefaultGridFSRepositoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Documents\FileWithoutChunkSize;
use Documents\FileWithoutMetadata;
use Documents\User;
use MongoDB\BSON\ObjectId;

use function assert;
use function fclose;
Expand Down Expand Up @@ -46,6 +47,51 @@ public function testOpenUploadStreamReturnsWritableResource(): void
self::assertNull($file->getMetadata());
}

public function testOpenUploadStreamUsesIdFromOptions(): void
{
$uploadOptions = new UploadOptions();
$uploadOptions->id = new ObjectId('1234567890abcdef12345678');

$uploadStream = $this->getRepository()->openUploadStream('somefile.txt', $uploadOptions);
self::assertIsResource($uploadStream);

fwrite($uploadStream, 'contents');
fclose($uploadStream);

$file = $this->getRepository()->findOneBy(['filename' => 'somefile.txt']);
assert($file instanceof File);
self::assertInstanceOf(File::class, $file);

self::assertSame('somefile.txt', $file->getFilename());
self::assertSame('1234567890abcdef12345678', $file->getId());
self::assertSame(8, $file->getLength());
self::assertSame(12345, $file->getChunkSize());
self::assertEqualsWithDelta(new DateTime(), $file->getUploadDate(), 1);
self::assertNull($file->getMetadata());
}

public function testOpenUploadStreamUsesDisableMD5FromOptions(): void
{
$uploadOptions = new UploadOptions();
$uploadOptions->disableMD5 = true;

$uploadStream = $this->getRepository()->openUploadStream('somefile.txt', $uploadOptions);
self::assertIsResource($uploadStream);

fwrite($uploadStream, 'contents');
fclose($uploadStream);

$file = $this->getRepository()->findOneBy(['filename' => 'somefile.txt']);
assert($file instanceof File);
self::assertInstanceOf(File::class, $file);

self::assertSame('somefile.txt', $file->getFilename());
self::assertSame(8, $file->getLength());
self::assertSame(12345, $file->getChunkSize());
self::assertEqualsWithDelta(new DateTime(), $file->getUploadDate(), 1);
self::assertNull($file->getMetadata());
}

public function testOpenUploadStreamUsesChunkSizeFromOptions(): void
{
$uploadOptions = new UploadOptions();
Expand Down