Skip to content

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

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

Merged
susnux merged 4 commits into from
Oct 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee02e32
feat(AppFramework): Add full support for date / time / datetime columns
susnux Aug 19, 2024
e314d52
fix: Adjust parameter type usage and add SQLite support
susnux Aug 20, 2024
db94e10
fix: Prevent breaking change in IQueryBuilder
susnux Aug 21, 2024
0e54c2b
fix: Adjust Entity types
susnux Sep 19, 2024
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
fix: Adjust Entity types 
Signed-off-by: Ferdinand Thiessen <[email protected]>
  • Loading branch information
@susnux
susnux committed Oct 17, 2024
commit 0e54c2bd43853891deac92f4ef9842c40ca64feb
13 changes: 7 additions & 6 deletions apps/contactsinteraction/lib/Db/RecentContact.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
namespace OCA\ContactsInteraction\Db;

use OCP\AppFramework\Db\Entity;
use OCP\DB\Types;

/**
* @method void setActorUid(string $uid)
Expand All @@ -33,11 +34,11 @@ class RecentContact extends Entity {
protected int $lastContact = -1;

public function __construct() {
$this->addType('actorUid', 'string');
$this->addType('uid', 'string');
$this->addType('email', 'string');
$this->addType('federatedCloudId', 'string');
$this->addType('card', 'blob');
$this->addType('lastContact', 'int');
$this->addType('actorUid', Types::STRING);
$this->addType('uid', Types::STRING);
$this->addType('email', Types::STRING);
$this->addType('federatedCloudId', Types::STRING);
$this->addType('card', Types::BLOB);
$this->addType('lastContact', Types::INTEGER);
}
}
7 changes: 4 additions & 3 deletions apps/dav/lib/CalDAV/Proxy/Proxy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
namespace OCA\DAV\CalDAV\Proxy;

use OCP\AppFramework\Db\Entity;
use OCP\DB\Types;

/**
* @method string getOwnerId()
Expand All @@ -28,8 +29,8 @@ class Proxy extends Entity {
protected $permissions;

public function __construct() {
$this->addType('ownerId', 'string');
$this->addType('proxyId', 'string');
$this->addType('permissions', 'int');
$this->addType('ownerId', Types::STRING);
$this->addType('proxyId', Types::STRING);
$this->addType('permissions', Types::INTEGER);
}
}
9 changes: 5 additions & 4 deletions apps/dav/lib/Db/Direct.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
namespace OCA\DAV\Db;

use OCP\AppFramework\Db\Entity;
use OCP\DB\Types;

/**
* @method string getUserId()
Expand All @@ -34,9 +35,9 @@ class Direct extends Entity {
protected $expiration;

public function __construct() {
$this->addType('userId', 'string');
$this->addType('fileId', 'int');
$this->addType('token', 'string');
$this->addType('expiration', 'int');
$this->addType('userId', Types::STRING);
$this->addType('fileId', Types::INTEGER);
$this->addType('token', Types::STRING);
$this->addType('expiration', Types::INTEGER);
}
}
11 changes: 6 additions & 5 deletions apps/oauth2/lib/Db/AccessToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
namespace OCA\OAuth2\Db;

use OCP\AppFramework\Db\Entity;
use OCP\DB\Types;

/**
* @method int getTokenId()
Expand Down Expand Up @@ -36,12 +37,12 @@ class AccessToken extends Entity {
protected $tokenCount;

public function __construct() {
$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);
Comment on lines -39 to +46
Copy link
Member

@ChristophWurst ChristophWurst Oct 17, 2024

Choose a reason for hiding this comment

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

Someone should write a Rector rule for this

Copy link
Member

@nickvergessen nickvergessen Oct 20, 2024

Choose a reason for hiding this comment

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

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 🙈

Copy link
Member

@ChristophWurst ChristophWurst Oct 21, 2024

Choose a reason for hiding this comment

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

Came back to check if this was intentional.

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

Copy link
Contributor Author

@susnux susnux Oct 21, 2024

Choose a reason for hiding this comment

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

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!

Copy link
Member

@ChristophWurst ChristophWurst Oct 21, 2024

Choose a reason for hiding this comment

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

Fair enough, int indeed wasn't documented!

Copy link
Member

@nickvergessen nickvergessen Oct 22, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@nickvergessen nickvergessen Oct 22, 2024

Choose a reason for hiding this comment

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

Ref #48837

}
}
3 changes: 2 additions & 1 deletion apps/oauth2/lib/Db/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
namespace OCA\OAuth2\Db;

use OCP\AppFramework\Db\Entity;
use OCP\DB\Types;

/**
* @method string getClientIdentifier()
Expand All @@ -28,7 +29,7 @@ class Client extends Entity {
protected $secret;

public function __construct() {
$this->addType('id', 'int');
$this->addType('id', Types::INTEGER);
$this->addType('name', 'string');
$this->addType('redirectUri', 'string');
$this->addType('clientIdentifier', 'string');
Expand Down
7 changes: 4 additions & 3 deletions apps/user_status/lib/Db/UserStatus.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
namespace OCA\UserStatus\Db;

use OCP\AppFramework\Db\Entity;
use OCP\DB\Types;

/**
* Class UserStatus
Expand Down Expand Up @@ -73,13 +74,13 @@ class UserStatus extends Entity {
public function __construct() {
$this->addType('userId', 'string');
$this->addType('status', 'string');
$this->addType('statusTimestamp', 'int');
$this->addType('statusTimestamp', Types::INTEGER);
$this->addType('isUserDefined', 'boolean');
$this->addType('messageId', 'string');
$this->addType('customIcon', 'string');
$this->addType('customMessage', 'string');
$this->addType('clearAt', 'int');
$this->addType('clearAt', Types::INTEGER);
$this->addType('isBackup', 'boolean');
$this->addType('statusMessageTimestamp', 'int');
$this->addType('statusMessageTimestamp', Types::INTEGER);
}
}
5 changes: 3 additions & 2 deletions core/Db/LoginFlowV2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
namespace OC\Core\Db;

use OCP\AppFramework\Db\Entity;
use OCP\DB\Types;

/**
* @method int getTimestamp()
Expand Down Expand Up @@ -55,8 +56,8 @@ class LoginFlowV2 extends Entity {
protected $appPassword;

public function __construct() {
$this->addType('timestamp', 'int');
$this->addType('started', 'int');
$this->addType('timestamp', Types::INTEGER);
$this->addType('started', Types::INTEGER);
$this->addType('pollToken', 'string');
$this->addType('loginToken', 'string');
$this->addType('publicKey', 'string');
Expand Down
15 changes: 8 additions & 7 deletions lib/private/Authentication/Token/PublicKeyToken.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

use OCP\AppFramework\Db\Entity;
use OCP\Authentication\Token\IToken;
use OCP\DB\Types;

/**
* @method void setId(int $id)
Expand Down Expand Up @@ -88,16 +89,16 @@ public function __construct() {
$this->addType('passwordHash', 'string');
$this->addType('name', 'string');
$this->addType('token', 'string');
$this->addType('type', 'int');
$this->addType('remember', 'int');
$this->addType('lastActivity', 'int');
$this->addType('lastCheck', 'int');
$this->addType('type', Types::INTEGER);
$this->addType('remember', Types::INTEGER);
$this->addType('lastActivity', Types::INTEGER);
$this->addType('lastCheck', Types::INTEGER);
$this->addType('scope', 'string');
$this->addType('expires', 'int');
$this->addType('expires', Types::INTEGER);
$this->addType('publicKey', 'string');
$this->addType('privateKey', 'string');
$this->addType('version', 'int');
$this->addType('passwordInvalid', 'bool');
$this->addType('version', Types::INTEGER);
$this->addType('passwordInvalid', Types::BOOLEAN);
}

public function getId(): int {
Expand Down
3 changes: 2 additions & 1 deletion lib/private/Updater/Changes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
namespace OC\Updater;

use OCP\AppFramework\Db\Entity;
use OCP\DB\Types;

/**
* Class Changes
Expand Down Expand Up @@ -39,7 +40,7 @@ class Changes extends Entity {
public function __construct() {
$this->addType('version', 'string');
$this->addType('etag', 'string');
$this->addType('lastCheck', 'int');
$this->addType('lastCheck', Types::INTEGER);
$this->addType('data', 'string');
}
}
7 changes: 3 additions & 4 deletions lib/public/AppFramework/Db/Entity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
/**
* @method int getId()
* @method void setId(int $id)
* @psalm-type AllowedTypes = 'json'|'blob'|'datetime'|'string'|'int'|'integer'|'bool'|'boolean'|'float'|'double'|'array'|'object'
* @since 7.0.0
* @psalm-consistent-constructor
*/
Expand All @@ -26,7 +25,7 @@ abstract class Entity {
public $id;

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

@nickvergessen nickvergessen Oct 20, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@susnux susnux Oct 20, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@susnux susnux Oct 20, 2024

Choose a reason for hiding this comment

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

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).

Copy link
Member

@nickvergessen nickvergessen Oct 20, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@susnux susnux Oct 21, 2024

Choose a reason for hiding this comment

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

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!

private array $_fieldTypes = ['id' => 'integer'];

/**
Expand Down Expand Up @@ -67,7 +66,7 @@ public static function fromRow(array $row): static {


/**
* @return array<string, AllowedTypes> with attribute and type
* @return array<string, \OCP\DB\Types::*> with attribute and type
* @since 7.0.0
*/
public function getFieldTypes(): array {
Expand Down Expand Up @@ -260,7 +259,7 @@ public function getUpdatedFields(): array {
* that value once its being returned from the database
*
* @param string $fieldName the name of the attribute
* @param AllowedTypes $type the type which will be used to match a cast
* @param \OCP\DB\Types::* $type the type which will be used to match a cast
* @since 7.0.0
*/
protected function addType(string $fieldName, string $type): void {
Expand Down