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.

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

Support mapping time series collections #2687

Merged
merged 16 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Remove unused isTimeSeries option
  • Loading branch information
alcaeus committed Oct 18, 2024
commit bfaa88d7393e9c41ae913fa2896838899dc03796
5 changes: 0 additions & 5 deletions lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -800,9 +800,6 @@
*/
public $isReadOnly;

/** READ ONLY: indicates whether the collection is a time series collection */
public bool $isTimeSeries = false;

/** READ ONLY: stores metadata about the time series collection */
public ?TimeSeries $timeSeriesOptions = null;
Copy link
Member

Choose a reason for hiding this comment

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

Could you use readonly here? AFAIK, that would just mean that markAsTimeSeries(), which sets this, can only be called once. Perhaps that's something to defer until later, as I expect it could be done across the board for all "READ ONLY" metadata properties.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes and no. What we'd want here is asymetric visibility, where changing the property is only done through accessors (such as markAsTimeSeries) that can incorporate checks, while reading is always allowed. readonly in PHP actually means write-once, meaning that one wouldn't be able to override the time series configuration (e.g. to change granularity) when using inheritance mapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

readonly = you can change the assigned object properties. You cannot change the reference to the object. And I think - I'm not sure though - that you can only make assignment in the constructor 🤔 ?

Copy link
Member Author

@alcaeus alcaeus Oct 18, 2024

Choose a reason for hiding this comment

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

I think - I'm not sure though - that you can only make assignment in the constructor

I thought something like that as well, but then it seems we are wrong, and the property can be initialised after the constructor, but only from within the class scope: https://3v4l.org/cV5QL

For our case, this still wouldn't work however, as we can't assign a default value to a readonly property but have to leave it uninitialised, which means that accessing the property always needs to be guarded by an isset check :(


Expand Down Expand Up @@ -2185,7 +2182,6 @@ public function markAsTimeSeries(TimeSeries $options): void
{
$this->validateTimeSeriesOptions($options);

$this->isTimeSeries = true;
$this->timeSeriesOptions = $options;
}

Expand Down Expand Up @@ -2542,7 +2538,6 @@ public function __sleep()
'idGenerator',
'indexes',
'shardKey',
'isTimeSeries',
'timeSeriesOptions',
];

Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ODM/MongoDB/SchemaManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,7 @@ public function createDocumentCollection(string $documentName, ?int $maxTimeMs =
$options['validationLevel'] = $class->getValidationLevel();
}

if ($class->isTimeSeries) {
if ($class->timeSeriesOptions !== null) {
$options['timeseries'] = array_filter(
jmikola marked this conversation as resolved.
Show resolved Hide resolved
[
'timeField' => $class->timeSeriesOptions->timeField,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,6 @@ public function testTimeSeriesDocument(): void
{
$metadata = $this->dm->getClassMetadata(AbstractMappingDriverTimeSeriesDocument::class);

self::assertTrue($metadata->isTimeSeries);
self::assertEquals(
new ODM\TimeSeries('time', 'metadata', Granularity::Seconds, 86400),
$metadata->timeSeriesOptions,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -987,15 +987,15 @@ public function testDefaultTimeSeriesMapping(): void
{
$metadata = $this->dm->getClassMetadata(TimeSeriesTestDocument::class);

self::assertFalse($metadata->isTimeSeries);
self::assertNull($metadata->timeSeriesOptions);
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 a meaningful test? I'm also a bit confused that these tests reference a TimeSeriesDocument class defined in the same file, while you also have a separate TimeSeriesDocument class defined in its own file.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it isn't. This was left over from refactoring out the isTimeSeries field, where I replaced all occurrences of checking the field with the corresponding check on the timeSeriesOptions property. I removed the test since it's redundant.

As for the classes being used, there are lots of classes defined in this test file, I believe to avoid issues where changing the mapping of one class to test new functionality affects related tests. I've followed this for consistency, but we'd probably do well to refactor this in the near future.

Copy link
Member

Choose a reason for hiding this comment

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

Aye. Ideally, I suppose these tests would be split into their own files or define anonymous classes within the test itself. I understand there's so much prior art that it'd be a chore to clean everything up at once, though.

}

public function testTimeSeriesMappingOnlyWithTimeField(): void
{
$metadata = $this->dm->getClassMetadata(TimeSeriesTestDocument::class);
$metadata->markAsTimeSeries(new ODM\TimeSeries('time'));

self::assertTrue($metadata->isTimeSeries);
self::assertNotNull($metadata->timeSeriesOptions);
self::assertSame('time', $metadata->timeSeriesOptions->timeField);
}

Expand All @@ -1012,7 +1012,7 @@ public function testTimeSeriesMappingWithMetadataField(): void
$metadata = $this->dm->getClassMetadata(TimeSeriesTestDocument::class);
$metadata->markAsTimeSeries(new ODM\TimeSeries('time', 'metadata'));

self::assertTrue($metadata->isTimeSeries);
self::assertNotNull($metadata->timeSeriesOptions);
self::assertSame('metadata', $metadata->timeSeriesOptions->metaField);
}

Expand Down