feat(AppFramework): Add full support for date / time / datetime columns #47329

susnux · 2024-08-19T15:11:54Z

Summary

This adds support for all Doctrine supported types, for the column types only the immutable variants needed to be added. But especially those types are the important ones, as our Entity class works by detecting changes through setters. Meaning if it is mutable, changes like $entity->date->modfiy() can not be detected, so the immutable types make more sense here.

Similar the parameter types needed to be added.

Enity and QBMapper needed to be adjusted so they support (auto map) those types, required when insert or update an entity.

Also added more tests, especially to make sure the mapper really serializes the values correctly.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

lib/public/DB/Types.php

nickvergessen · 2024-08-20T06:13:23Z

The next major (which we will have to update to soon) changed behaviour of DateTime: https://github.com/doctrine/dbal/blob/4.1.x/UPGRADE.md#bc-break-stricter-datetime-types
Can you make sure this does not cause a bc break on our side? Can we start with forward compatible code already?

susnux · 2024-08-20T08:29:22Z

@nickvergessen this is what I implemented already, whats breaking is that datetime does not allow \DateTimeImmutable anymore. Thats why I added both datetime and datetime_immutable.

lib/public/AppFramework/Db/Entity.php

nickvergessen · 2024-08-21T08:46:50Z

Running talk CI against it in ci: Run tests against server PR feat/add-datetime-qbmapper-support spreed#13063 to have more test angles

lib/public/DB/QueryBuilder/IQueryBuilder.php

lib/public/AppFramework/Db/Entity.php

ChristophWurst

Nice

ChristophWurst · 2024-10-17T12:38:42Z

apps/oauth2/lib/Db/AccessToken.php

-		$this->addType('id', 'int');
-		$this->addType('tokenId', 'int');
-		$this->addType('clientId', 'int');
+		$this->addType('id', Types::INTEGER);
+		$this->addType('tokenId', Types::INTEGER);
+		$this->addType('clientId', Types::INTEGER);
 		$this->addType('hashedCode', 'string');
 		$this->addType('encryptedToken', 'string');
-		$this->addType('codeCreatedAt', 'int');
-		$this->addType('tokenCount', 'int');
+		$this->addType('codeCreatedAt', Types::INTEGER);
+		$this->addType('tokenCount', Types::INTEGER);


Someone should write a Rector rule for this

Also this is not a clean replacement. Types::INTEGER is 'integer' not 'int' which was used in almost all apps (at least the ones I maintain which now have dozens of Psalm errors 🙈

Came back to check if this was intentional.

@susnux would there be any downside of allow "int" again?

Came back to check if this was intentional.

Yes it was, as int, double, and bool were never documented, only integer, float and boolean were part of our documentation (since Nextcloud 20: https://docs.nextcloud.com/server/20/developer_manual/basics/storage/database.html#types ).
So the idea was to still allow them (it does not break) but make usage of it an error for static code checking to make developers aware they are using something undocumented.

Meaning to make it easier for us to change types if needed as we only need to change a single source of truth (the DB types).

would there be any downside of allow "int" again?

From my side: just duplicates a state, but lets add it back to make it less noisy for developers!

Fair enough, int indeed wasn't documented!

addType('[^']+', 'int')

Results: 116

addType('[^']+', 'integer')

Results: 175

So while it was never documented, it was working and is used almost 40% of the time 🙈 So yeah we can not have it in docs going forward, but should keep an eye on keeping it working, so apps don't break unnecessarily.

For bool its more based with 17 (bool) to 68 (boolean), but still 20%

double I didn't find a single result (in apps we marketed, ship, support or maintain), neither for array or object.

This adds support for all Doctrine supported types, for the column types only the immutable variants needed to be added. But especially those types are the important ones, as our **Entity** class works by detecting changes through setters. Meaning if it is mutable, changes like `$entity->date->modfiy()` can not be detected, so the immutable types make more sense here. Similar the parameter types needed to be added. `Enity` and `QBMapper` needed to be adjusted so they support (auto map) those types, required when insert or update an entity. Also added more tests, especially to make sure the mapper really serializes the values correctly. Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de> Co-authored-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com> Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

nickvergessen · 2024-10-20T08:20:11Z

lib/public/AppFramework/Db/Entity.php

@@ -24,7 +25,7 @@ abstract class Entity {
 	public $id;

 	private array $_updatedFields = [];
-	/** @var array<string, AllowedTypes> */
+	/** @var array<string, \OCP\DB\Types::*> */


I'm really not sure about this change.

It indicates that 'bigint', 'smallint' and 'text' would be accepted, but settype( will break?

As mentioned on the other part it "dropped" the short forms bool and int

Also array and double are no longer?

In general I also don't like the mix of dictionaries. \OCP\DB\Types was database field types. Now it's directly being used for PHP types as well. If this is intended, we need to map all, otherwise we need to use a different set of constants.

First of all the AllowedTypes was never released, in stable30 we just annotate as string and the setter is just using settype:

server/lib/public/AppFramework/Db/Entity.php

Line 86 in 496f7ae

protected function setter(string $name, array $args): void {

We already have 3 different mapping for db types: OCP\DB\Types, IQueryBuilder and the migration column types. With this we would have 4 different. But the underlying doctrine abstraction only knows 1.5 (query builder and table column types but they are somewhat connected).

So the IQueryBuilder types make sense as they are for a different purpose (directly query the DB and converting parameters). But the column types are the same as the field types as the fields are just the abstraction of the DB types, meaning from an app perspective you would give the column type to the migration and to the ORM (the Entity class) and the class handles then the conversion between the DB and PHP.

It indicates that 'bigint', 'smallint' and 'text' would be accepted, but settype( will break?

Good point I think we should fix this here!

As mentioned on the other part it "dropped" the short forms bool and int

Only for psalm, it still works so does not break. I do not think it makes sense to add arbitrary aliases, no?

Also array and double are no longer?

Well there is only float in doctrine and for PHP it makes no difference as float and double are the same in PHP.
So double is just again an alias not available on doctrine. Yet it still will work and not break (only psalm will complain).
For array I am not sure, I guess it would be json, no? I do not see any other type that can be mapped to array with doctrine for columns. But also this is not hard breaking as settype still will work.

We also could have just added more types, but then we end up with 3 different type sources for the same task (mapping PHP -> DB types).
With this way you could even think further and add attributes to the Entity properties to define types for automatic conversion (and if you like the ORM way then even migrations could be created from this).

We already have 3 different mapping for db types: OCP\DB\Types, IQueryBuilder and the migration column types.

Not sure what "migration column types" is? Migrations should use OCP\DB\Types …
OCP\Migration\Attributes\ColumnType TIL. Sounds like that tries to be a replacement for OCP\DB\Types. Either it should be dropped again, or it needs to be extended with the change of this PR.
Should we deprecate OCP\DB\Types then? We don't need 2 things trying to do the same thing, because one will always be the neglected sibling (as we just learned here).

Then for now let's make sure all \OCP\DB\Types are supported by the Entity.
And we add a small psalm/type-hint for the old deprecated simple strings int|bool|double and still map them to their new counterparts.

Should we deprecate OCP\DB\Types then?

Not sure the OCP\Migration\Attributes\ColumnType is very specific also the namespace... It is a nice enum, but I think the namespace is confusing, so maybe the other way round?

Then for now let's make sure all \OCP\DB\Types are supported by the Entity.

Makes sense, thank you for your feedback!

susnux added enhancement 3. to review Waiting for reviews labels Aug 19, 2024

susnux added this to the Nextcloud 31 milestone Aug 19, 2024

susnux requested review from nickvergessen, Altahrim, juliusknorr and come-nc August 19, 2024 15:11

nickvergessen reviewed Aug 19, 2024

View reviewed changes

lib/public/DB/Types.php Outdated Show resolved Hide resolved

susnux force-pushed the feat/add-datetime-qbmapper-support branch from 2346049 to 2d9b552 Compare August 19, 2024 16:57

susnux requested a review from nickvergessen August 19, 2024 16:58

come-nc approved these changes Aug 20, 2024

View reviewed changes

lib/public/AppFramework/Db/Entity.php Outdated Show resolved Hide resolved

susnux force-pushed the feat/add-datetime-qbmapper-support branch 5 times, most recently from e62ee82 to c059051 Compare August 20, 2024 13:46

nickvergessen mentioned this pull request Aug 21, 2024

ci: Run tests against server PR feat/add-datetime-qbmapper-support nextcloud/spreed#13063

Closed

susnux commented Aug 21, 2024

View reviewed changes

lib/public/DB/QueryBuilder/IQueryBuilder.php Outdated Show resolved Hide resolved

susnux force-pushed the feat/add-datetime-qbmapper-support branch from 478135b to 87fb634 Compare September 19, 2024 12:39

github-advanced-security bot found potential problems Sep 19, 2024

View reviewed changes

lib/public/AppFramework/Db/Entity.php Fixed Show fixed Hide fixed

lib/public/AppFramework/Db/Entity.php Fixed Show fixed Hide fixed

susnux force-pushed the feat/add-datetime-qbmapper-support branch from 3046e20 to a236626 Compare September 19, 2024 22:43

susnux requested review from ChristophWurst, miaulalala and tcitworld as code owners September 19, 2024 22:43

susnux force-pushed the feat/add-datetime-qbmapper-support branch from a236626 to 45e799b Compare September 20, 2024 01:25

ArtificialOwl approved these changes Oct 17, 2024

View reviewed changes

ChristophWurst approved these changes Oct 17, 2024

View reviewed changes

ChristophWurst added the pending documentation This pull request needs an associated documentation update label Oct 17, 2024

Altahrim approved these changes Oct 17, 2024

View reviewed changes

susnux and others added 4 commits October 17, 2024 18:31

fix: Adjust parameter type usage and add SQLite support

e314d52

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

fix: Prevent breaking change in IQueryBuilder

db94e10

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

fix: Adjust Entity types

0e54c2b

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

susnux force-pushed the feat/add-datetime-qbmapper-support branch from 45e799b to 0e54c2b Compare October 17, 2024 16:31

susnux requested review from st3iny, SebastianKrupinski and kesselb as code owners October 17, 2024 16:31

susnux merged commit 2ef74b9 into master Oct 18, 2024
177 checks passed

susnux deleted the feat/add-datetime-qbmapper-support branch October 18, 2024 17:05

susnux mentioned this pull request Oct 18, 2024

feat(developer): Document new database field types nextcloud/documentation#12293

Merged

nickvergessen reviewed Oct 20, 2024

View reviewed changes

ChristophWurst mentioned this pull request Oct 21, 2024

fix: Entity parameter types nextcloud/calendar#6436

Merged

nickvergessen mentioned this pull request Oct 22, 2024

fix(entity): Fix mapping of old/sub-types to actually supported datab… #48837

Merged

4 tasks

st3iny mentioned this pull request Oct 22, 2024

feat: classify emails by importance based on subjects nextcloud/mail#10277

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(AppFramework): Add full support for date / time / datetime columns #47329

feat(AppFramework): Add full support for date / time / datetime columns #47329

susnux commented Aug 19, 2024

nickvergessen commented Aug 20, 2024

susnux commented Aug 20, 2024

nickvergessen commented Aug 21, 2024

ChristophWurst left a comment

ChristophWurst Oct 17, 2024

nickvergessen Oct 20, 2024

ChristophWurst Oct 21, 2024

susnux Oct 21, 2024

ChristophWurst Oct 21, 2024

nickvergessen Oct 22, 2024

nickvergessen Oct 22, 2024

nickvergessen Oct 20, 2024

susnux Oct 20, 2024

susnux Oct 20, 2024

nickvergessen Oct 20, 2024

susnux Oct 21, 2024

feat(AppFramework): Add full support for date / time / datetime columns #47329

feat(AppFramework): Add full support for date / time / datetime columns #47329

Conversation

susnux commented Aug 19, 2024

Summary

Checklist

nickvergessen commented Aug 20, 2024

susnux commented Aug 20, 2024

nickvergessen commented Aug 21, 2024

ChristophWurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment