Skip to content

chore(files_versions): Use new metadata API for versions #44175

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
skjnldsv merged 1 commit into from
Mar 14, 2024
Merged

chore(files_versions): Use new metadata API for versions #44175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 0 additions & 9 deletions apps/files_versions/lib/Db/VersionEntity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,6 @@ public function jsonSerialize(): array {
];
}

public function getLabel(): string {
return $this->metadata['label'] ?? '';
}

public function setLabel(string $label): void {
$this->metadata['label'] = $label;
$this->markFieldUpdated('metadata');
}

/**
* @abstract given a key, return the value associated with the key in the metadata column
* if nothing is found, we return an empty string
Expand Down
2 changes: 1 addition & 1 deletion apps/files_versions/lib/Listener/MetadataFileEvents.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public function post_write_hook(Node $node): void {
// check if our version manager supports setting the metadata
if ($this->versionManager instanceof IMetadataVersionBackend) {
$author = $user->getUID();
$this->versionManager->setMetadataValue($node, 'author', $author);
$this->versionManager->setMetadataValue($node, $node->getMTime(), 'author', $author);
}
}
}
4 changes: 2 additions & 2 deletions apps/files_versions/lib/Sabre/Plugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public function afterGet(RequestInterface $request, ResponseInterface $response)

public function propFind(PropFind $propFind, INode $node): void {
if ($node instanceof VersionFile) {
$propFind->handle(self::VERSION_LABEL, fn () => $node->getLabel());
$propFind->handle(self::VERSION_LABEL, fn () => $node->getMetadataValue('label'));
$propFind->handle(self::VERSION_AUTHOR, fn () => $node->getMetadataValue("author"));
$propFind->handle(FilesPlugin::HAS_PREVIEW_PROPERTYNAME, fn () => $this->previewManager->isMimeSupported($node->getContentType()));
}
Expand All @@ -104,7 +104,7 @@ public function propPatch($path, PropPatch $propPatch): void {
$node = $this->server->tree->getNodeForPath($path);

if ($node instanceof VersionFile) {
$propPatch->handle(self::VERSION_LABEL, fn ($label) => $node->setLabel($label));
$propPatch->handle(self::VERSION_LABEL, fn (string $label) => $node->setMetadataValue('label', $label));
}
}
}
33 changes: 14 additions & 19 deletions apps/files_versions/lib/Sabre/VersionFile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

use OCA\Files_Versions\Versions\IDeletableVersionBackend;
use OCA\Files_Versions\Versions\IMetadataVersion;
use OCA\Files_Versions\Versions\IMetadataVersionBackend;
use OCA\Files_Versions\Versions\INameableVersion;
use OCA\Files_Versions\Versions\INameableVersionBackend;
use OCA\Files_Versions\Versions\IVersion;
Expand All @@ -38,15 +39,10 @@
use Sabre\DAV\IFile;

class VersionFile implements IFile {
/** @var IVersion */
private $version;

/** @var IVersionManager */
private $versionManager;

public function __construct(IVersion $version, IVersionManager $versionManager) {
$this->version = $version;
$this->versionManager = $versionManager;
public function __construct(
private IVersion $version,
private IVersionManager $versionManager
) {
}

public function put($data) {
Expand Down Expand Up @@ -93,17 +89,14 @@ public function setName($name) {
throw new Forbidden();
}

public function getLabel(): ?string {
if ($this->version instanceof INameableVersion && $this->version->getSourceFile()->getSize() > 0) {
return $this->version->getLabel();
} else {
return null;
}
}
public function setMetadataValue(string $key, string $value): bool {
$backend = $this->version->getBackend();

public function setLabel($label): bool {
if ($this->versionManager instanceof INameableVersionBackend) {
$this->versionManager->setVersionLabel($this->version, $label);
if ($backend instanceof IMetadataVersionBackend) {
$backend->setMetadataValue($this->version->getSourceFile(), $this->version->getRevisionId(), $key, $value);

Check notice

Code scanning / Psalm

ArgumentTypeCoercion

Argument 1 of OCA\Files_Versions\Versions\IMetadataVersionBackend::setMetadataValue expects OCP\Files\Node, but parent type OCP\Files\FileInfo provided

Check notice

Code scanning / Psalm

PossiblyInvalidArgument

Argument 2 of OCA\Files_Versions\Versions\IMetadataVersionBackend::setMetadataValue expects int, but possibly different type int|string provided
return true;
} elseif ($key === 'label' && $backend instanceof INameableVersionBackend) {
Copy link
Contributor Author

@artonge artonge Mar 13, 2024

Choose a reason for hiding this comment

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

Ensure compatibility with backends that are not migrated yet.

$backend->setVersionLabel($this->version, $value);

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCA\Files_Versions\Versions\INameableVersionBackend::setVersionLabel has been marked as deprecated
return true;
} else {
return false;
Expand All @@ -113,6 +106,8 @@ public function setLabel($label): bool {
public function getMetadataValue(string $key): ?string {
if ($this->version instanceof IMetadataVersion) {
return $this->version->getMetadataValue($key);
} elseif ($key === 'label' && $this->version instanceof INameableVersion) {
Copy link
Contributor Author

@artonge artonge Mar 13, 2024

Choose a reason for hiding this comment

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

Ensure compatibility with backends that are not migrated yet.

return $this->version->getLabel();

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCA\Files_Versions\Versions\INameableVersion::getLabel has been marked as deprecated
}
return null;
}
Expand Down
4 changes: 2 additions & 2 deletions apps/files_versions/lib/Storage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ public static function expireOlderThanMaxForUser($uid) {
$versionEntity = $versionsMapper->findVersionForFileId($node->getId(), $version);
$versionEntities[$info->getId()] = $versionEntity;

if ($versionEntity->getLabel() !== '') {
if ($versionEntity->getMetadataValue('label') !== null && $versionEntity->getMetadataValue('label') !== '') {
return false;
}
} catch (NotFoundException $e) {
Expand Down Expand Up @@ -929,7 +929,7 @@ public static function expire($filename, $uid) {
$pathparts = pathinfo($path);
$timestamp = (int)substr($pathparts['extension'] ?? '', 1);
$versionEntity = $versionsMapper->findVersionForFileId($file->getId(), $timestamp);
if ($versionEntity->getLabel() !== '') {
if ($versionEntity->getMetadataValue('label') !== '') {
continue;
}
$versionsMapper->delete($versionEntity);
Expand Down
12 changes: 2 additions & 10 deletions apps/files_versions/lib/Versions/IMetadataVersionBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,10 @@ interface IMetadataVersionBackend {
* Sets a key value pair in the metadata column corresponding to the node's version.
*
* @param Node $node the node that triggered the Metadata event listener, aka, the file version
* @param int $revision the key for the json value of the metadata column
* @param string $key the key for the json value of the metadata column
* @param string $value the value that corresponds to the key in the metadata column
* @since 29.0.0
*/
public function setMetadataValue(Node $node, string $key, string $value): void;

/**
* Retrieves a corresponding value from the metadata column using the key.
*
* @param Node $node the node that triggered the Metadata event listener, aka, the file version
* @param string $key the key for the json value of the metadata column
* @since 29.0.0
*/
public function getMetadataValue(Node $node, string $key): ?string;
Copy link
Contributor Author

@artonge artonge Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The metadata can already be retrieved from the IMetadataVersion. So this method seem unnecessary. @emoral435 does it make sense to remove it? Can still be added later if needed, but I don't see a reason for it.

Copy link
Contributor

@emoral435 emoral435 Mar 13, 2024

Choose a reason for hiding this comment

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

Agreed 👍

public function setMetadataValue(Node $node, int $revision, string $key, string $value): void;
Copy link
Contributor Author

@artonge artonge Mar 13, 2024

Choose a reason for hiding this comment

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

I added a $revision param to be able to target a specific version. @emoral435, ok with you?

Copy link
Contributor

@emoral435 emoral435 Mar 13, 2024

Choose a reason for hiding this comment

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

I like this change! Ok with me.

}
78 changes: 16 additions & 62 deletions apps/files_versions/lib/Versions/LegacyVersionsBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,25 +46,14 @@
use OCP\IUserManager;
use OCP\IUserSession;

class LegacyVersionsBackend implements IVersionBackend, INameableVersionBackend, IDeletableVersionBackend, INeedSyncVersionBackend, IMetadataVersionBackend {
private IRootFolder $rootFolder;
private IUserManager $userManager;
private VersionsMapper $versionsMapper;
private IMimeTypeLoader $mimeTypeLoader;
private IUserSession $userSession;

class LegacyVersionsBackend implements IVersionBackend, IDeletableVersionBackend, INeedSyncVersionBackend, IMetadataVersionBackend {
public function __construct(
IRootFolder $rootFolder,
IUserManager $userManager,
VersionsMapper $versionsMapper,
IMimeTypeLoader $mimeTypeLoader,
IUserSession $userSession,
private IRootFolder $rootFolder,
private IUserManager $userManager,
private VersionsMapper $versionsMapper,
private IMimeTypeLoader $mimeTypeLoader,
private IUserSession $userSession,
) {
$this->rootFolder = $rootFolder;
$this->userManager = $userManager;
$this->versionsMapper = $versionsMapper;
$this->mimeTypeLoader = $mimeTypeLoader;
$this->userSession = $userSession;
}

public function useBackendForStorage(IStorage $storage): bool {
Expand Down Expand Up @@ -160,7 +149,7 @@ public function getVersionsForFile(IUser $user, FileInfo $file): array {
$file,
$this,
$user,
$versions['db']->getLabel(),
$versions['db']->getMetadata() ?? [],
);

array_push($davVersions, $version);
Expand All @@ -185,7 +174,7 @@ public function createVersion(IUser $user, FileInfo $file) {
}

public function rollback(IVersion $version) {
if (!$this->currentUserHasPermissions($version, \OCP\Constants::PERMISSION_UPDATE)) {
if (!$this->currentUserHasPermissions($version->getSourceFile(), \OCP\Constants::PERMISSION_UPDATE)) {
throw new Forbidden('You cannot restore this version because you do not have update permissions on the source file.');
}

Expand Down Expand Up @@ -234,24 +223,8 @@ public function getVersionFile(IUser $user, FileInfo $sourceFile, $revision): Fi
return $file;
}

public function setVersionLabel(IVersion $version, string $label): void {
if (!$this->currentUserHasPermissions($version, \OCP\Constants::PERMISSION_UPDATE)) {
throw new Forbidden('You cannot label this version because you do not have update permissions on the source file.');
}

$versionEntity = $this->versionsMapper->findVersionForFileId(
$version->getSourceFile()->getId(),
$version->getTimestamp(),
);
if (trim($label) === '') {
$label = null;
}
$versionEntity->setLabel($label ?? '');
$this->versionsMapper->update($versionEntity);
}

public function deleteVersion(IVersion $version): void {
if (!$this->currentUserHasPermissions($version, \OCP\Constants::PERMISSION_DELETE)) {
if (!$this->currentUserHasPermissions($version->getSourceFile(), \OCP\Constants::PERMISSION_DELETE)) {
throw new Forbidden('You cannot delete this version because you do not have delete permissions on the source file.');
}

Expand Down Expand Up @@ -295,8 +268,7 @@ public function deleteVersionsEntity(File $file): void {
$this->versionsMapper->deleteAllVersionsForFileId($file->getId());
}

private function currentUserHasPermissions(IVersion $version, int $permissions): bool {
$sourceFile = $version->getSourceFile();
private function currentUserHasPermissions(FileInfo $sourceFile, int $permissions): bool {
$currentUserId = $this->userSession->getUser()?->getUID();

if ($currentUserId === null) {
Expand All @@ -314,32 +286,14 @@ private function currentUserHasPermissions(IVersion $version, int $permissions):
return ($sourceFile->getPermissions() & $permissions) === $permissions;
}

public function setMetadataValue(Node $node, string $key, string $value): void {
// Do not handle folders.
if ($node instanceof File) {

try {
$versionEntity = $this->versionsMapper->findVersionForFileId($node->getId(), $node->getMTime());
} catch (\Exception $e) {
throw $e; // the version does not exist or too many versions exist
}

$currentMetadata = $versionEntity->getMetadata() ?? [];

$currentMetadata[$key] = $value;
$versionEntity->setMetadata($currentMetadata);
$this->versionsMapper->update($versionEntity);
public function setMetadataValue(Node $node, int $revision, string $key, string $value): void {
if (!$this->currentUserHasPermissions($node, \OCP\Constants::PERMISSION_UPDATE)) {
throw new Forbidden('You cannot update the version\'s metadata because you do not have update permissions on the source file.');
}

}
$versionEntity = $this->versionsMapper->findVersionForFileId($node->getId(), $revision);

public function getMetadataValue(Node $node, string $key): ?string {
try {
$versionEntity = $this->versionsMapper->findVersionForFileId($node->getId(), $node->getMTime());
return $versionEntity->getMetadataValue($key);
} catch (\InvalidArgumentException $e) {
// we tried to find a version or key that doesn't exist
return null;
}
$versionEntity->setMetadataValue($key, $value);
$this->versionsMapper->update($versionEntity);
}
}
71 changes: 12 additions & 59 deletions apps/files_versions/lib/Versions/Version.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,61 +26,21 @@
namespace OCA\Files_Versions\Versions;

use OCP\Files\FileInfo;
use OCP\Files\Node;
use OCP\IUser;

class Version implements IVersion, INameableVersion, IMetadataVersion {
/** @var int */
private $timestamp;

/** @var int|string */
private $revisionId;

/** @var string */
private $name;

private string $label;

/** @var int|float */
private $size;

/** @var string */
private $mimetype;

/** @var string */
private $path;

/** @var FileInfo */
private $sourceFileInfo;

/** @var IVersionBackend */
private $backend;

/** @var IUser */
private $user;

class Version implements IVersion, IMetadataVersion {
public function __construct(
int $timestamp,
$revisionId,
string $name,
int|float $size,
string $mimetype,
string $path,
FileInfo $sourceFileInfo,
IVersionBackend $backend,
IUser $user,
string $label = ''
private int $timestamp,
private int|string $revisionId,
private string $name,
private int|float $size,
private string $mimetype,
private string $path,
private FileInfo $sourceFileInfo,
private IVersionBackend $backend,
private IUser $user,
private array $metadata = [],
) {
$this->timestamp = $timestamp;
$this->revisionId = $revisionId;
$this->name = $name;
$this->label = $label;
$this->size = $size;
$this->mimetype = $mimetype;
$this->path = $path;
$this->sourceFileInfo = $sourceFileInfo;
$this->backend = $backend;
$this->user = $user;
}

public function getBackend(): IVersionBackend {
Expand All @@ -107,10 +67,6 @@ public function getSourceFileName(): string {
return $this->name;
}

public function getLabel(): string {
return $this->label;
}

public function getMimeType(): string {
return $this->mimetype;
}
Expand All @@ -124,9 +80,6 @@ public function getUser(): IUser {
}

public function getMetadataValue(string $key): ?string {
if ($this->backend instanceof IMetadataVersionBackend && $this->sourceFileInfo instanceof Node) {
return $this->backend->getMetadataValue($this->sourceFileInfo, "author");
}
return null;
Comment on lines -127 to -130
Copy link
Contributor Author

@artonge artonge Mar 13, 2024

Choose a reason for hiding this comment

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

The constructor now receives the metadata, so we can simplify the logic here.

return $this->metadata[$key] ?? null;
}
}
Loading