From b615ebfd6bfeeae2bf4fdbb9f3d69653e59a6131 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Sun, 3 Jul 2022 21:24:57 +0200 Subject: [PATCH 01/11] Added basic migration to add date columns to DB Signed-off-by: Christian Wolf --- cookbook.code-workspace | 5 ++ .../Version000000Date20220703174647.php | 59 +++++++++++++++++ .../Version000000Date20220703174647Test.php | 63 +++++++++++++++++++ 3 files changed, 127 insertions(+) create mode 100644 lib/Migration/Version000000Date20220703174647.php create mode 100644 tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php diff --git a/cookbook.code-workspace b/cookbook.code-workspace index c2955098a..0ecefa8b1 100644 --- a/cookbook.code-workspace +++ b/cookbook.code-workspace @@ -47,6 +47,11 @@ ], "path-intellisense.mappings": { "cookbook": "${workspaceFolder}/src", + }, + "editor.quickSuggestions": { + "other": "on", + "comments": "on", + "strings": "on" } } } diff --git a/lib/Migration/Version000000Date20220703174647.php b/lib/Migration/Version000000Date20220703174647.php new file mode 100644 index 000000000..31332c223 --- /dev/null +++ b/lib/Migration/Version000000Date20220703174647.php @@ -0,0 +1,59 @@ +getTable('cookbook_names'); + + if(! $table->hasColumn('dateCreated')) { + $table->addColumn('dateCreated', 'date', [ + 'notnull' => false, + ]); + } + + if(!$table->hasColumn('dateModified')) { + $table->addColumn('dateModified', 'date', [ + 'notnull' => false, + ]); + } + + return $schema; + } + + /** + * @param IOutput $output + * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param array $options + */ + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + } +} diff --git a/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php b/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php new file mode 100644 index 000000000..275643493 --- /dev/null +++ b/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php @@ -0,0 +1,63 @@ +db->getQueryBuilder(); + $qb->insert('cookbook_names') + ->values([ + 'recipe_id' => ':recipe', + 'user_id' => ':user', + 'name' => ':name', + ]); + $qb->setParameter('name', 'name of the recipe'); + $qb->setParameter('user', 'username'); + $qb->setParameter('recipe', 1234); + + $this->assertEquals(1, $qb->execute()); + + $table = $this->schema->getTable('cookbook_names'); + $this->assertFalse($table->hasColumn('dateCreated')); + $this->assertFalse($table->hasColumn('dateModified')); + + // Run the migration under test + $this->migrationService->migrate('000000Date20220703174647'); + $this->renewSchema(); + + $table = $this->schema->getTable('cookbook_names'); + $this->assertTrue($table->hasColumn('dateCreated')); + $this->assertTrue($table->hasColumn('dateModified')); + + $qb = $this->db->getQueryBuilder(); + $qb->select('dateCreated', 'dateModified')->from('cookbook_names'); + $res = $qb->execute(); + $data = $res->fetchAll(); + + $this->assertEquals(1, count($data)); + $row = $data[0]; + $this->assertEquals(2, count($row)); + $this->assertNull($row['dateCreated']); + $this->assertNull($row['dateModified']); + + + //$this->assertEquals([null, null], $data[0]); + } + + + protected function getPreviousMigrationName(): string { + return '000000Date20210701093123'; + } +} From a43dd01976e5e27c9ac2979722ddb43d804a54ae Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 08:28:58 +0200 Subject: [PATCH 02/11] Create basic working structure Signed-off-by: Christian Wolf --- cookbook.code-workspace | 7 ++- lib/Db/DbTypesPolyfillHelper.php | 12 ++++ lib/Db/RecipeDb.php | 58 +++++++++++++++---- .../Version000000Date20220703174647.php | 4 +- lib/Service/DbCacheService.php | 13 +++++ lib/Service/RecipeService.php | 9 +-- 6 files changed, 83 insertions(+), 20 deletions(-) diff --git a/cookbook.code-workspace b/cookbook.code-workspace index 0ecefa8b1..1615d9dac 100644 --- a/cookbook.code-workspace +++ b/cookbook.code-workspace @@ -40,7 +40,7 @@ "**/vendor/**/{Tests,tests}/**", "**/.history/**", "**/vendor/**/vendor/**", - "3rdparty/**" + // "3rdparty/**" ], "cSpell.words": [ "Nextcloud" @@ -52,6 +52,9 @@ "other": "on", "comments": "on", "strings": "on" - } + }, + "intelephense.environment.includePaths": [ + "${workspaceFolder:base}/3rdparty/doctrine/dbal/src" + ] } } diff --git a/lib/Db/DbTypesPolyfillHelper.php b/lib/Db/DbTypesPolyfillHelper.php index 1d294cf89..8262e0bcc 100644 --- a/lib/Db/DbTypesPolyfillHelper.php +++ b/lib/Db/DbTypesPolyfillHelper.php @@ -14,6 +14,9 @@ class DbTypesPolyfillHelper { */ private $string; + /** @var string */ + private $date; + public function __construct(Util $util) { switch ($util->getVersion()[0]) { case 18: @@ -21,11 +24,13 @@ public function __construct(Util $util) { case 20: $this->int = \Doctrine\DBAL\Types\Type::INTEGER; $this->string = \Doctrine\DBAL\Types\Type::STRING; + $this->date = \Doctrine\DBAL\Types\Type::DATE; break; default: $this->int = \OCP\DB\Types::INTEGER; $this->string = \OCP\DB\Types::STRING; + $this->date = \OCP\DB\Types::DATE; break; } } @@ -43,4 +48,11 @@ final public function INT() { final public function STRING() { return $this->string; } + + /** + * @return string + */ + final public function DATE() { + return $this->date; + } } diff --git a/lib/Db/RecipeDb.php b/lib/Db/RecipeDb.php index cb01d1078..24c480e83 100755 --- a/lib/Db/RecipeDb.php +++ b/lib/Db/RecipeDb.php @@ -2,6 +2,7 @@ namespace OCA\Cookbook\Db; +use DateTimeImmutable; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; use OCP\AppFramework\Db\DoesNotExistException; @@ -91,7 +92,7 @@ public function deleteRecipeById(int $id) { public function findAllRecipes(string $user_id) { $qb = $this->db->getQueryBuilder(); - $qb->select(['r.recipe_id', 'r.name', 'k.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords']) ->from(self::DB_TABLE_RECIPES, 'r') ->where('r.user_id = :user') ->orderBy('r.name'); @@ -229,7 +230,7 @@ public function getRecipesByCategory(string $category, string $user_id) { // for the recipe, but those don't seem to work: // $qb->select(['r.recipe_id', 'r.name', 'GROUP_CONCAT(k.name) AS keywords']) // not working // $qb->select(['r.recipe_id', 'r.name', DB::raw('GROUP_CONCAT(k.name) AS keywords')]) // not working - $qb->select(['r.recipe_id', 'r.name', 'k.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords']) ->from(self::DB_TABLE_CATEGORIES, 'c') ->where('c.name = :category') ->andWhere('c.user_id = :user') @@ -242,7 +243,7 @@ public function getRecipesByCategory(string $category, string $user_id) { $qb->groupBy(['r.name', 'r.recipe_id', 'k.name']); $qb->orderBy('r.name'); } else { - $qb->select(['r.recipe_id', 'r.name', 'k.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords']) ->from(self::DB_TABLE_RECIPES, 'r') ->leftJoin('r', self::DB_TABLE_KEYWORDS, 'k', 'r.recipe_id = k.recipe_id') ->leftJoin( @@ -279,7 +280,7 @@ public function getRecipesByKeywords(string $keywords, string $user_id) { $qb = $this->db->getQueryBuilder(); - $qb->select(['r.recipe_id', 'r.name', 'kk.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'kk.name AS keywords']) ->from(self::DB_TABLE_KEYWORDS, 'k') ->where('k.name IN (:keywords)') ->andWhere('k.user_id = :user') @@ -314,7 +315,7 @@ public function findRecipes(array $keywords, string $user_id) { $qb = $this->db->getQueryBuilder(); - $qb->select(['r.recipe_id', 'r.name', 'k.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords']) ->from(self::DB_TABLE_RECIPES, 'r'); $qb->leftJoin('r', self::DB_TABLE_KEYWORDS, 'k', 'k.recipe_id = r.recipe_id'); @@ -454,7 +455,9 @@ public function insertRecipes(array $recipes, string $userId) { ->values([ 'recipe_id' => ':id', 'user_id' => ':userid', - 'name' => ':name' + 'name' => ':name', + 'dateCreated' => ':dateCreated', + 'dateModified' => ':dateModified', ]); $qb->setParameter('userid', $userId); @@ -462,6 +465,15 @@ public function insertRecipes(array $recipes, string $userId) { foreach ($recipes as $recipe) { $qb->setParameter('id', $recipe['id'], $this->types->INT()); $qb->setParameter('name', $recipe['name'], $this->types->STRING()); + + $dateCreated = $this->parseDate($recipe['dateCreated']); + if($dateCreated === null) { + $dateCreated = $this->parseDate('now'); + } + $qb->setParameter('dateCreated', $dateCreated, $this->types->DATE()); + + $dateModified = $this->parseDate($recipe['dateModified']); + $qb->setParameter('dateModified', $dateModified, $this->types->DATE()); $qb->execute(); } @@ -472,20 +484,30 @@ public function updateRecipes(array $recipes, string $userId) { return; } - $qb = $this->db->getQueryBuilder(); - foreach ($recipes as $recipe) { + $qb = $this->db->getQueryBuilder(); $qb->update(self::DB_TABLE_RECIPES) ->where('recipe_id = :id', 'user_id = :uid'); - $literal = []; - $literal['name'] = $qb->expr()->literal($recipe['name'], IQueryBuilder::PARAM_STR); - $qb->set('name', $literal['name']); + // $literal = []; + // $literal['name'] = $qb->expr()->literal($recipe['name'], IQueryBuilder::PARAM_STR); + // $qb->set('name', $literal['name']); + $qb->set('name', $qb->createNamedParameter($recipe['name'], IQueryBuilder::PARAM_STR)); + + $dateCreated = $this->parseDate($recipe['dateCreated'] ?? 'now'); + $dateModified = $this->parseDate($recipe['dateModified']) ?? $dateCreated; + + $qb->set('dateCreated', $qb->expr()->literal($dateCreated, IQueryBuilder::PARAM_DATE)); + $qb->set('dateModified', $qb->expr()->literal($dateModified, IQueryBuilder::PARAM_DATE)); $qb->setParameter('id', $recipe['id']); $qb->setParameter('uid', $userId); - $qb->execute(); + try { + $qb->execute(); + } catch (\Exception $ex) { + throw $ex; + } } } @@ -617,4 +639,16 @@ public function removeKeywordPairs(array $pairs, string $userId) { $qb->execute(); } + + private function parseDate(?string $date) { + if (is_null($date)) { + return null; + } + + try{ + return new DateTimeImmutable($date); + } catch (\Exception $ex) { + return new DateTimeImmutable(); + } + } } diff --git a/lib/Migration/Version000000Date20220703174647.php b/lib/Migration/Version000000Date20220703174647.php index 31332c223..9f3f55dbb 100644 --- a/lib/Migration/Version000000Date20220703174647.php +++ b/lib/Migration/Version000000Date20220703174647.php @@ -35,13 +35,13 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt $table = $schema->getTable('cookbook_names'); if(! $table->hasColumn('dateCreated')) { - $table->addColumn('dateCreated', 'date', [ + $table->addColumn('dateCreated', 'datetime_immutable', [ 'notnull' => false, ]); } if(!$table->hasColumn('dateModified')) { - $table->addColumn('dateModified', 'date', [ + $table->addColumn('dateModified', 'datetime_immutable', [ 'notnull' => false, ]); } diff --git a/lib/Service/DbCacheService.php b/lib/Service/DbCacheService.php index cf386e772..990477737 100644 --- a/lib/Service/DbCacheService.php +++ b/lib/Service/DbCacheService.php @@ -142,6 +142,10 @@ private function parseJSONFile(File $jsonFile): array { $id = (int) $jsonFile->getParent()->getId(); $json['id'] = $id; + if(!isset($json['dateModified'])) { + $json['dateModified'] = null; + } + return $json; } @@ -157,6 +161,8 @@ private function fetchDbRecipeInformations() { $obj = []; $obj['name'] = $row['name']; $obj['id'] = $id; + $obj['dateCreated'] = $row['dateCreated']; + $obj['dateModified'] = $row['dateModified']; $ret[$id] = $obj; } @@ -220,6 +226,13 @@ private function isDbEntryUpToDate($id) { if ($dbEntry['name'] !== $fileEntry['name']) { return false; } + if($dbEntry['dateCreated'] !== $fileEntry['dateCreated']) { + return false; + } + + if($dbEntry['dateModified'] !== $fileEntry['dateModified']) { + return false; + } return true; } diff --git a/lib/Service/RecipeService.php b/lib/Service/RecipeService.php index f90def5f3..116410a44 100755 --- a/lib/Service/RecipeService.php +++ b/lib/Service/RecipeService.php @@ -942,10 +942,11 @@ public function getAllCategoriesInSearchIndex() { */ private function addDatesToRecipes(array &$recipes) { foreach ($recipes as $i => $recipe) { - // TODO Add data to database instead of reading from files - $r = $this->getRecipeById($recipe['recipe_id']); - $recipes[$i]['dateCreated'] = $r['dateCreated']; - $recipes[$i]['dateModified'] = $r['dateModified']; + if(! array_key_exists('dateCreated', $recipe) || ! array_key_exists('dateModified', $recipe)) { + $r = $this->getRecipeById($recipe['recipe_id']); + $recipes[$i]['dateCreated'] = $r['dateCreated']; + $recipes[$i]['dateModified'] = $r['dateModified']; + } } } From d5a7eb0ed0330647de680640d990342fc5c7447a Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 08:45:29 +0200 Subject: [PATCH 03/11] Enhance DB access to use prepared statements Signed-off-by: Christian Wolf --- lib/Db/RecipeDb.php | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/lib/Db/RecipeDb.php b/lib/Db/RecipeDb.php index 24c480e83..0d40129c6 100755 --- a/lib/Db/RecipeDb.php +++ b/lib/Db/RecipeDb.php @@ -198,7 +198,7 @@ public function findAllCategories(string $user_id) { ) ) ->where( - $qb->expr()->eq('r.user_id', $qb->expr()->literal($user_id)), + $qb->expr()->eq('r.user_id', $qb->createNamedParameter($user_id, IQueryBuilder::PARAM_STR)), $qb->expr()->isNull('c.name') ); @@ -256,7 +256,7 @@ public function getRecipesByCategory(string $category, string $user_id) { ) ) ->where( - $qb->expr()->eq('r.user_id', $qb->expr()->literal($user_id)), + $qb->expr()->eq('r.user_id', $qb->createNamedParameter($user_id, IQueryBuilder::PARAM_STR)), $qb->expr()->isNull('c.name') ); } @@ -433,8 +433,8 @@ public function deleteRecipes(array $ids, string $userId) { foreach ($ids as $id) { $qb->orWhere( $qb->expr()->andX( - "recipe_id = $id", - $qb->expr()->eq("user_id", $qb->expr()->literal($userId)) + $qb->expr()->eq('recipe_id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)), + $qb->expr()->eq("user_id", $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)) )); } @@ -489,16 +489,13 @@ public function updateRecipes(array $recipes, string $userId) { $qb->update(self::DB_TABLE_RECIPES) ->where('recipe_id = :id', 'user_id = :uid'); - // $literal = []; - // $literal['name'] = $qb->expr()->literal($recipe['name'], IQueryBuilder::PARAM_STR); - // $qb->set('name', $literal['name']); $qb->set('name', $qb->createNamedParameter($recipe['name'], IQueryBuilder::PARAM_STR)); $dateCreated = $this->parseDate($recipe['dateCreated'] ?? 'now'); $dateModified = $this->parseDate($recipe['dateModified']) ?? $dateCreated; - $qb->set('dateCreated', $qb->expr()->literal($dateCreated, IQueryBuilder::PARAM_DATE)); - $qb->set('dateModified', $qb->expr()->literal($dateModified, IQueryBuilder::PARAM_DATE)); + $qb->set('dateCreated', $qb->createNamedParameter($dateCreated, IQueryBuilder::PARAM_DATE)); + $qb->set('dateModified', $qb->createNamedParameter($dateModified, IQueryBuilder::PARAM_DATE)); $qb->setParameter('id', $recipe['id']); $qb->setParameter('uid', $userId); @@ -558,7 +555,7 @@ public function updateCategoryOfRecipe(int $recipeId, string $categoryName, stri $qb = $this->db->getQueryBuilder(); $qb->update(self::DB_TABLE_CATEGORIES) ->where('recipe_id = :rid', 'user_id = :user'); - $qb->set('name', $qb->expr()->literal($categoryName, IQueryBuilder::PARAM_STR)); + $qb->set('name', $qb->createNamedParameter($categoryName, IQueryBuilder::PARAM_STR)); $qb->setParameter('rid', $recipeId, $this->types->INT()); $qb->setParameter('user', $userId, $this->types->STRING()); $qb->execute(); @@ -630,9 +627,9 @@ public function removeKeywordPairs(array $pairs, string $userId) { foreach ($pairs as $p) { $qb->orWhere( $qb->expr()->andX( - $qb->expr()->eq('user_id', $qb->expr()->literal($userId)), - $qb->expr()->eq('recipe_id', $qb->expr()->literal($p['recipeId'])), - $qb->expr()->eq('name', $qb->expr()->literal($p['name'])) + $qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)), + $qb->expr()->eq('recipe_id', $qb->createNamedParameter($p['recipeId'], IQueryBuilder::PARAM_INT)), + $qb->expr()->eq('name', $qb->createNamedParameter($p['name'], IQueryBuilder::PARAM_STR)) ) ); } From 44e85bf4c8f4d43be4b16b4728f56e344d80a9c6 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 12:42:24 +0200 Subject: [PATCH 04/11] Created filter for recipe files Signed-off-by: Christian Wolf --- lib/Helper/Filter/AbstractRecipeFilter.php | 26 ++++ lib/Helper/Filter/DB/RecipeDatesFilter.php | 120 ++++++++++++++++++ .../Filter/DB/RecipeDatesFilterTest.php | 101 +++++++++++++++ 3 files changed, 247 insertions(+) create mode 100644 lib/Helper/Filter/AbstractRecipeFilter.php create mode 100644 lib/Helper/Filter/DB/RecipeDatesFilter.php create mode 100644 tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php diff --git a/lib/Helper/Filter/AbstractRecipeFilter.php b/lib/Helper/Filter/AbstractRecipeFilter.php new file mode 100644 index 000000000..ccfa0798e --- /dev/null +++ b/lib/Helper/Filter/AbstractRecipeFilter.php @@ -0,0 +1,26 @@ +patternDate = join('|', [ + '^' . self::PATTERN_DATE . '$', + '^' . self::PATTERN_DATE . 'T' . self::PATTERN_TIME . '$' + ]); + } + + public function apply(array &$json, File $recipe): bool { + $ret = false; + + // First ensure the entries are present in general + $this->ensureEntryExists($json, self::DATE_CREATED, $ret); + $this->ensureEntryExists($json, self::DATE_MODIFIED, $ret); + + // Ensure the date formats are valid + $this->checkDateFormat($json, self::DATE_CREATED, $ret); + $this->checkDateFormat($json, self::DATE_MODIFIED, $ret); + + if(is_null($json['dateCreated'])) { + if (is_null($json['dateModified'])) { + // No dates have been set. Fall back to time of file + $json['dateCreated'] = $this->getTimeFromFile($recipe); + $ret = true; + } else { + // Copy over the modification time to the creation time + $json['dateCreated'] = $json['dateModified']; + $ret = true; + } + } + /* + The else case is not considered: + If only the creation time is given, this is a valid recipe (no modifications so far). + If both are given, no problem is present. + */ + + return $ret; + } + + private function getTimeFromFile(File $file): string { + $timestamp = $file->getCreationTime(); + if($timestamp === 0) { + $timestamp = $file->getUploadTime(); + } + if($timestamp === 0) { + $timestamp = $file->getMTime(); + } + + return $this->getDateFromTimestamp($timestamp); + } + + private function getDateFromTimestamp(int $timestamp): string { + $date = new DateTime(); + $date->setTimestamp($timestamp); + + return $date->format(DateTime::ISO8601); + } + + private function ensureEntryExists(array &$json, string $name, bool &$ret) { + if(!array_key_exists($name, $json)) { + $json[$name] = null; + $ret = true; + } + } + + private function checkDateFormat(array &$json, string $name, bool &$ret) { + if($json[$name] === null) { + return; + } + + // Check for valid date format + if(preg_match('/' . $this->patternDate . '/', $json[$name]) === 1) { + return; + } + + // Last desperate approach: Is it a timestamp? + if(preg_match('/^\d+$/', $json[$name])) { + if($json[$name] > 0) { + $json[$name] = $this->getDateFromTimestamp($json[$name]); + $ret = true; + return; + } + } + + // We cannot read the format. Removing it from teh recipe + $json[$name] = null; + $ret = true; + return; + } +} diff --git a/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php b/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php new file mode 100644 index 000000000..5ccb9a0d2 --- /dev/null +++ b/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php @@ -0,0 +1,101 @@ +dut = new RecipeDatesFilter(); + } + + public function dpFromJson() { + yield ['2022-07-06T11:08:54', null, '2022-07-06T11:08:54', null, false]; + yield [1657098534, 0, '2022-07-06T09:08:54+0000', null, true]; + yield [1657098534, 1657098540, '2022-07-06T09:08:54+0000', '2022-07-06T09:09:00+0000', true]; + yield [null, 1657098540, '2022-07-06T09:09:00+0000', '2022-07-06T09:09:00+0000', true]; + yield [0, 1657098540, '2022-07-06T09:09:00+0000', '2022-07-06T09:09:00+0000', true]; + } + + /** + * @dataProvider dpFromJson + */ + public function testFilterFromJson($created, $modified, $expectedCreation, $expectedModification, $updated) { + $recipe = [ + 'name' => 'my Recipe', + 'dateCreated' => $created, + 'dateModified' => $modified + ]; + $copy = $recipe; + + $file = $this->createStub(File::class); + + $ret = $this->dut->apply($recipe, $file); + + $this->assertEquals($updated, $ret, 'Reporting of modification status'); + $this->assertEquals($expectedCreation, $recipe['dateCreated'], 'Wrong creation date'); + $this->assertEquals($expectedModification, $recipe['dateModified'], 'Wrong modification date'); + + unset($recipe['dateCreated']); + unset($recipe['dateModified']); + unset($copy['dateCreated']); + unset($copy['dateModified']); + + $this->assertEquals($copy, $recipe, 'Other entries must not change.'); + } + + public function dpFromFile() { + yield ['2022-07-06T09:08:54+0000', false, false, 1657098534, 1657098535, 1657098536]; + yield ['2022-07-06T09:08:55+0000', false, false, 0, 1657098535, 1657098536]; + yield ['2022-07-06T09:08:56+0000', false, false, 0, 0, 1657098536]; + yield ['2022-07-06T09:08:54+0000', true, true, 1657098534, 1657098535, 1657098536]; + } + + /** + * @dataProvider dpFromFile + */ + public function testFilterFromFile($creation, $creationPresent, $modificationPresent, $creationTime, $uploadTime, $mTime) { + $recipe = [ + 'name' => 'my Recipe', + ]; + if($creationPresent) { + $recipe['dateCreated'] = null; + } + if($modificationPresent) { + $recipe['dateModified'] = null; + } + + $copy = $recipe; + + /** @var Stub|File */ + $file = $this->createStub(File::class); + $file->method('getCreationTime')->willReturn($creationTime); + $file->method('getUploadTime')->willReturn($uploadTime); + $file->method('getMTime')->willReturn($mTime); + + $ret = $this->dut->apply($recipe, $file); + + $this->assertTrue($ret, 'Reporting of modification status'); + + $this->assertEquals($creation, $recipe['dateCreated'], 'Wrong creation date'); + $this->assertNull($recipe['dateModified'], 'Wrong modification date'); + + unset($recipe['dateCreated']); + unset($recipe['dateModified']); + unset($copy['dateCreated']); + unset($copy['dateModified']); + + $this->assertEquals($copy, $recipe, 'Other entries must not change.'); + + } +} From 2303776c50188336b2c9392bf9a244f74ebada86 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 12:43:34 +0200 Subject: [PATCH 05/11] Fix PHP code styling Signed-off-by: Christian Wolf --- .php-cs-fixer.dist.php | 18 ++++++--- lib/Db/RecipeDb.php | 8 ++-- lib/Helper/Filter/AbstractRecipeFilter.php | 6 +-- lib/Helper/Filter/DB/RecipeDatesFilter.php | 37 ++++++++++--------- .../Version000000Date20220703174647.php | 7 ++-- lib/Service/DbCacheService.php | 6 +-- lib/Service/RecipeService.php | 2 +- .../Version000000Date20220703174647Test.php | 4 +- .../Filter/DB/RecipeDatesFilterTest.php | 31 ++++++++++------ 9 files changed, 67 insertions(+), 52 deletions(-) diff --git a/.php-cs-fixer.dist.php b/.php-cs-fixer.dist.php index bbc30f485..bd39a76c8 100644 --- a/.php-cs-fixer.dist.php +++ b/.php-cs-fixer.dist.php @@ -21,23 +21,31 @@ public function getRules() : array { 'phpdoc_single_line_var_spacing' => true, 'phpdoc_var_without_name' => true ]; - return array_merge(['@PSR12' => true], $parentRules, $additionalRules); + $ret = array_merge(['@PSR12' => true], $parentRules, $additionalRules); + // print_r($ret); + return $ret; } } $config = new CookbookConfig(); -$config +$finder = $config ->getFinder() - ->ignoreVCSIgnored(true) + // ->ignoreVCSIgnored(true) ->exclude('build') ->exclude('l10n') -// ->notPath('lib/Vendor') ->exclude('src') ->exclude('node_modules') ->exclude('vendor') ->exclude('.github') - ->in(__DIR__); + ->in(__DIR__ ); +$config->setFinder($finder); + +// foreach($finder as $f) { +// print_r($f); +// } + +// print_r($config); return $config; diff --git a/lib/Db/RecipeDb.php b/lib/Db/RecipeDb.php index 0d40129c6..efb1d39b8 100755 --- a/lib/Db/RecipeDb.php +++ b/lib/Db/RecipeDb.php @@ -465,13 +465,13 @@ public function insertRecipes(array $recipes, string $userId) { foreach ($recipes as $recipe) { $qb->setParameter('id', $recipe['id'], $this->types->INT()); $qb->setParameter('name', $recipe['name'], $this->types->STRING()); - + $dateCreated = $this->parseDate($recipe['dateCreated']); - if($dateCreated === null) { + if ($dateCreated === null) { $dateCreated = $this->parseDate('now'); } $qb->setParameter('dateCreated', $dateCreated, $this->types->DATE()); - + $dateModified = $this->parseDate($recipe['dateModified']); $qb->setParameter('dateModified', $dateModified, $this->types->DATE()); @@ -642,7 +642,7 @@ private function parseDate(?string $date) { return null; } - try{ + try { return new DateTimeImmutable($date); } catch (\Exception $ex) { return new DateTimeImmutable(); diff --git a/lib/Helper/Filter/AbstractRecipeFilter.php b/lib/Helper/Filter/AbstractRecipeFilter.php index ccfa0798e..7dbd1ab3c 100644 --- a/lib/Helper/Filter/AbstractRecipeFilter.php +++ b/lib/Helper/Filter/AbstractRecipeFilter.php @@ -7,19 +7,19 @@ /** * An abstract filter on a recipe. - * + * * A filter should have a single purpose that is serves and implement this interface */ interface AbstractRecipeFilter { /** * Filter the given recipe according to the filter class specification. - * + * * This function can make changes to the recipe array to carry out the needed changes. * In order to signal if the JSON file needs updating, the return value must be true if and only if the recipe was changed. * * @param array $json The recipe data as array * @param File $recipe The file containing the recipe for further processing - * @return boolean true, if and only if the recipe was changed + * @return bool true, if and only if the recipe was changed * @throws InvalidRecipeException if the recipe was not correctly filtered */ public function apply(array &$json, File $recipe): bool; diff --git a/lib/Helper/Filter/DB/RecipeDatesFilter.php b/lib/Helper/Filter/DB/RecipeDatesFilter.php index ccfe76910..d86cdfe98 100644 --- a/lib/Helper/Filter/DB/RecipeDatesFilter.php +++ b/lib/Helper/Filter/DB/RecipeDatesFilter.php @@ -3,22 +3,20 @@ namespace OCA\Cookbook\Helper\Filter\DB; use DateTime; -use DateTimeImmutable; use OCA\Cookbook\Helper\Filter\AbstractRecipeFilter; use OCP\Files\File; /** * Ensure the dates of a recipe are valid - * + * * This filter will update the recipe to have both valid dateCreated and dateModified. * If the dates are given in correct format, nothing is changed. - * + * * If only the dateModified is given, the dateCreated is set to the same value. - * + * * If neither is given, the file modification time of the JSON file is taken into account. */ class RecipeDatesFilter implements AbstractRecipeFilter { - private const DATE_CREATED = 'dateCreated'; private const DATE_MODIFIED = 'dateModified'; @@ -28,15 +26,14 @@ class RecipeDatesFilter implements AbstractRecipeFilter { /** @var string */ private $patternDate; - public function __construct() - { + public function __construct() { $this->patternDate = join('|', [ '^' . self::PATTERN_DATE . '$', '^' . self::PATTERN_DATE . 'T' . self::PATTERN_TIME . '$' ]); } - public function apply(array &$json, File $recipe): bool { + public function apply(array &$json, File $recipe): bool { $ret = false; // First ensure the entries are present in general @@ -47,7 +44,7 @@ public function apply(array &$json, File $recipe): bool { $this->checkDateFormat($json, self::DATE_CREATED, $ret); $this->checkDateFormat($json, self::DATE_MODIFIED, $ret); - if(is_null($json['dateCreated'])) { + if (is_null($json['dateCreated'])) { if (is_null($json['dateModified'])) { // No dates have been set. Fall back to time of file $json['dateCreated'] = $this->getTimeFromFile($recipe); @@ -67,45 +64,49 @@ public function apply(array &$json, File $recipe): bool { return $ret; } + /** + * Undocumented function + * + */ private function getTimeFromFile(File $file): string { $timestamp = $file->getCreationTime(); - if($timestamp === 0) { + if ($timestamp === 0) { $timestamp = $file->getUploadTime(); } - if($timestamp === 0) { + if ($timestamp === 0) { $timestamp = $file->getMTime(); } return $this->getDateFromTimestamp($timestamp); } - + private function getDateFromTimestamp(int $timestamp): string { $date = new DateTime(); $date->setTimestamp($timestamp); - + return $date->format(DateTime::ISO8601); } private function ensureEntryExists(array &$json, string $name, bool &$ret) { - if(!array_key_exists($name, $json)) { + if (!array_key_exists($name, $json)) { $json[$name] = null; $ret = true; } } private function checkDateFormat(array &$json, string $name, bool &$ret) { - if($json[$name] === null) { + if ($json[$name] === null) { return; } // Check for valid date format - if(preg_match('/' . $this->patternDate . '/', $json[$name]) === 1) { + if (preg_match('/' . $this->patternDate . '/', $json[$name]) === 1) { return; } // Last desperate approach: Is it a timestamp? - if(preg_match('/^\d+$/', $json[$name])) { - if($json[$name] > 0) { + if (preg_match('/^\d+$/', $json[$name])) { + if ($json[$name] > 0) { $json[$name] = $this->getDateFromTimestamp($json[$name]); $ret = true; return; diff --git a/lib/Migration/Version000000Date20220703174647.php b/lib/Migration/Version000000Date20220703174647.php index 9f3f55dbb..9eecd96b4 100644 --- a/lib/Migration/Version000000Date20220703174647.php +++ b/lib/Migration/Version000000Date20220703174647.php @@ -13,7 +13,6 @@ * Auto-generated migration step: Please modify to your needs! */ class Version000000Date20220703174647 extends SimpleMigrationStep { - /** * @param IOutput $output * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` @@ -33,14 +32,14 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt $schema = $schemaClosure(); $table = $schema->getTable('cookbook_names'); - - if(! $table->hasColumn('dateCreated')) { + + if (! $table->hasColumn('dateCreated')) { $table->addColumn('dateCreated', 'datetime_immutable', [ 'notnull' => false, ]); } - if(!$table->hasColumn('dateModified')) { + if (!$table->hasColumn('dateModified')) { $table->addColumn('dateModified', 'datetime_immutable', [ 'notnull' => false, ]); diff --git a/lib/Service/DbCacheService.php b/lib/Service/DbCacheService.php index 990477737..868d3d72c 100644 --- a/lib/Service/DbCacheService.php +++ b/lib/Service/DbCacheService.php @@ -142,7 +142,7 @@ private function parseJSONFile(File $jsonFile): array { $id = (int) $jsonFile->getParent()->getId(); $json['id'] = $id; - if(!isset($json['dateModified'])) { + if (!isset($json['dateModified'])) { $json['dateModified'] = null; } @@ -226,11 +226,11 @@ private function isDbEntryUpToDate($id) { if ($dbEntry['name'] !== $fileEntry['name']) { return false; } - if($dbEntry['dateCreated'] !== $fileEntry['dateCreated']) { + if ($dbEntry['dateCreated'] !== $fileEntry['dateCreated']) { return false; } - if($dbEntry['dateModified'] !== $fileEntry['dateModified']) { + if ($dbEntry['dateModified'] !== $fileEntry['dateModified']) { return false; } diff --git a/lib/Service/RecipeService.php b/lib/Service/RecipeService.php index 116410a44..76d1ef671 100755 --- a/lib/Service/RecipeService.php +++ b/lib/Service/RecipeService.php @@ -942,7 +942,7 @@ public function getAllCategoriesInSearchIndex() { */ private function addDatesToRecipes(array &$recipes) { foreach ($recipes as $i => $recipe) { - if(! array_key_exists('dateCreated', $recipe) || ! array_key_exists('dateModified', $recipe)) { + if (! array_key_exists('dateCreated', $recipe) || ! array_key_exists('dateModified', $recipe)) { $r = $this->getRecipeById($recipe['recipe_id']); $recipes[$i]['dateCreated'] = $r['dateCreated']; $recipes[$i]['dateModified'] = $r['dateModified']; diff --git a/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php b/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php index 275643493..c8a48bb0d 100644 --- a/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php +++ b/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php @@ -2,8 +2,6 @@ namespace OCA\Cookbook\tests\Migration\Setup\Migrations; -use OCP\DB\QueryBuilder\IQueryBuilder; - include_once __DIR__ . '/AbstractMigrationTestCase.php'; class Version000000Date20220703174647Test extends AbstractMigrationTestCase { @@ -51,7 +49,7 @@ public function testRedundantEntriesInDB(/*$data, $updatedUsers*/) { $this->assertEquals(2, count($row)); $this->assertNull($row['dateCreated']); $this->assertNull($row['dateModified']); - + //$this->assertEquals([null, null], $data[0]); } diff --git a/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php b/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php index 5ccb9a0d2..88d263018 100644 --- a/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php +++ b/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php @@ -14,8 +14,7 @@ class RecipeDatesFilterTest extends TestCase { /** @var RecipeDatesFilter */ private $dut; - protected function setUp(): void - { + protected function setUp(): void { $this->dut = new RecipeDatesFilter(); } @@ -29,6 +28,11 @@ public function dpFromJson() { /** * @dataProvider dpFromJson + * @param mixed $created + * @param mixed $modified + * @param mixed $expectedCreation + * @param mixed $expectedModification + * @param mixed $updated */ public function testFilterFromJson($created, $modified, $expectedCreation, $expectedModification, $updated) { $recipe = [ @@ -53,7 +57,7 @@ public function testFilterFromJson($created, $modified, $expectedCreation, $expe $this->assertEquals($copy, $recipe, 'Other entries must not change.'); } - + public function dpFromFile() { yield ['2022-07-06T09:08:54+0000', false, false, 1657098534, 1657098535, 1657098536]; yield ['2022-07-06T09:08:55+0000', false, false, 0, 1657098535, 1657098536]; @@ -63,39 +67,44 @@ public function dpFromFile() { /** * @dataProvider dpFromFile + * @param mixed $creation + * @param mixed $creationPresent + * @param mixed $modificationPresent + * @param mixed $creationTime + * @param mixed $uploadTime + * @param mixed $mTime */ public function testFilterFromFile($creation, $creationPresent, $modificationPresent, $creationTime, $uploadTime, $mTime) { $recipe = [ 'name' => 'my Recipe', ]; - if($creationPresent) { + if ($creationPresent) { $recipe['dateCreated'] = null; } - if($modificationPresent) { + if ($modificationPresent) { $recipe['dateModified'] = null; } $copy = $recipe; - + /** @var Stub|File */ $file = $this->createStub(File::class); $file->method('getCreationTime')->willReturn($creationTime); $file->method('getUploadTime')->willReturn($uploadTime); $file->method('getMTime')->willReturn($mTime); - + $ret = $this->dut->apply($recipe, $file); - + $this->assertTrue($ret, 'Reporting of modification status'); $this->assertEquals($creation, $recipe['dateCreated'], 'Wrong creation date'); $this->assertNull($recipe['dateModified'], 'Wrong modification date'); - + unset($recipe['dateCreated']); unset($recipe['dateModified']); unset($copy['dateCreated']); unset($copy['dateModified']); - - $this->assertEquals($copy, $recipe, 'Other entries must not change.'); + $this->assertEquals($copy, $recipe, 'Other entries must not change.'); } } From 6c6f1465a0fecb5c8d548559f7ed4c67b0ebfceb Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 13:00:21 +0200 Subject: [PATCH 06/11] Removed direct access to RecipeDb object from SearchProvider Signed-off-by: Christian Wolf --- lib/Db/RecipeDb.php | 10 ++++++++-- lib/Search/Provider.php | 14 ++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/Db/RecipeDb.php b/lib/Db/RecipeDb.php index efb1d39b8..81fa8c6c4 100755 --- a/lib/Db/RecipeDb.php +++ b/lib/Db/RecipeDb.php @@ -92,7 +92,7 @@ public function deleteRecipeById(int $id) { public function findAllRecipes(string $user_id) { $qb = $this->db->getQueryBuilder(); - $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords', 'c.name AS category']) ->from(self::DB_TABLE_RECIPES, 'r') ->where('r.user_id = :user') ->orderBy('r.name'); @@ -103,6 +103,12 @@ public function findAllRecipes(string $user_id) { 'k.user_id = :user' ) ); + $qb->leftJoin('r', self::DB_TABLE_CATEGORIES, 'c', + $qb->expr()->andX( + 'c.recipe_id = r.recipe_id', + 'c.user_id = r.user_id' + ) + ); $cursor = $qb->execute(); $result = $cursor->fetchAll(); @@ -315,7 +321,7 @@ public function findRecipes(array $keywords, string $user_id) { $qb = $this->db->getQueryBuilder(); - $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords', 'c.name AS category']) ->from(self::DB_TABLE_RECIPES, 'r'); $qb->leftJoin('r', self::DB_TABLE_KEYWORDS, 'k', 'k.recipe_id = r.recipe_id'); diff --git a/lib/Search/Provider.php b/lib/Search/Provider.php index f99eb3718..6f5d3a990 100644 --- a/lib/Search/Provider.php +++ b/lib/Search/Provider.php @@ -3,7 +3,6 @@ namespace OCA\Cookbook\Search; use OCA\Cookbook\AppInfo\Application; -use OCA\Cookbook\Db\RecipeDb; use OCA\Cookbook\Service\RecipeService; use OCP\IL10N; use OCP\IUser; @@ -24,17 +23,16 @@ class Provider implements IProvider { /** @var IURLGenerator */ private $urlGenerator; - /** @var RecipeDb */ - private $recipeDb; - /** @var RecipeService */ private $recipeService; - public function __construct(IL10n $il10n, IURLGenerator $urlGenerator, - RecipeDb $recipeDb, RecipeService $recipeService) { + public function __construct( + IL10n $il10n, + IURLGenerator $urlGenerator, + RecipeService $recipeService + ) { $this->l = $il10n; $this->urlGenerator = $urlGenerator; - $this->recipeDb = $recipeDb; $this->recipeService = $recipeService; } @@ -62,7 +60,7 @@ function (array $recipe) use ($user): SearchResultEntry { $id = $recipe['recipe_id']; $subline = ''; - $category = $this->recipeDb->getCategoryOfRecipe($id, $user->getUID()); + $category = $recipe['category']; if ($category !== null) { // TRANSLATORS Will be shown in search results, listing the recipe category, e.g., 'in Salads' $subline = $this->l->t('in %s', [$category]); From 2d4841e032169892fd28eabb7054a44197facbba Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 13:49:59 +0200 Subject: [PATCH 07/11] Extract business logic from DB connection Signed-off-by: Christian Wolf --- lib/Db/RecipeDb.php | 7 +-- .../Filter/NormalizeRecipeFileFilter.php | 45 ++++++++++++++ lib/Service/DbCacheService.php | 10 ++- .../Filter/NormalizeRecipeFileFilterTest.php | 61 +++++++++++++++++++ 4 files changed, 115 insertions(+), 8 deletions(-) create mode 100644 lib/Helper/Filter/NormalizeRecipeFileFilter.php create mode 100644 tests/Unit/Helper/Filter/NormalizeRecipeFileFilterTest.php diff --git a/lib/Db/RecipeDb.php b/lib/Db/RecipeDb.php index 81fa8c6c4..7f02c0ea9 100755 --- a/lib/Db/RecipeDb.php +++ b/lib/Db/RecipeDb.php @@ -473,9 +473,6 @@ public function insertRecipes(array $recipes, string $userId) { $qb->setParameter('name', $recipe['name'], $this->types->STRING()); $dateCreated = $this->parseDate($recipe['dateCreated']); - if ($dateCreated === null) { - $dateCreated = $this->parseDate('now'); - } $qb->setParameter('dateCreated', $dateCreated, $this->types->DATE()); $dateModified = $this->parseDate($recipe['dateModified']); @@ -497,8 +494,8 @@ public function updateRecipes(array $recipes, string $userId) { $qb->set('name', $qb->createNamedParameter($recipe['name'], IQueryBuilder::PARAM_STR)); - $dateCreated = $this->parseDate($recipe['dateCreated'] ?? 'now'); - $dateModified = $this->parseDate($recipe['dateModified']) ?? $dateCreated; + $dateCreated = $this->parseDate($recipe['dateCreated']); + $dateModified = $this->parseDate($recipe['dateModified']); $qb->set('dateCreated', $qb->createNamedParameter($dateCreated, IQueryBuilder::PARAM_DATE)); $qb->set('dateModified', $qb->createNamedParameter($dateModified, IQueryBuilder::PARAM_DATE)); diff --git a/lib/Helper/Filter/NormalizeRecipeFileFilter.php b/lib/Helper/Filter/NormalizeRecipeFileFilter.php new file mode 100644 index 000000000..389b5ec39 --- /dev/null +++ b/lib/Helper/Filter/NormalizeRecipeFileFilter.php @@ -0,0 +1,45 @@ +filters = [ + $datesFilter, + ]; + } + + /** + * Normalize a recipe file according to the installed filters + * + * If the recipe needs updating, the file gets overwritten in the storage. + * + * @param array $json The content of the recipe + * @param File $recipeFile The file containing the recipe + * @param bool $updateFiles true, if the file on storage should be updated with the modified version + * @return array The corrected recipe object + */ + public function filter(array $json, File $recipeFile, bool $updateFiles = false): array { + $changed = false; + + foreach ($this->filters as $filter) { + /** @var AbstractRecipeFilter $filter */ + $changed |= $filter->apply($json, $recipeFile); + } + + if ($changed && $updateFiles) { + $recipeFile->putContent(json_encode($json)); + $recipeFile->touch(); + } + + return $json; + } +} diff --git a/lib/Service/DbCacheService.php b/lib/Service/DbCacheService.php index 868d3d72c..dd6d044ef 100644 --- a/lib/Service/DbCacheService.php +++ b/lib/Service/DbCacheService.php @@ -6,6 +6,7 @@ use OCP\Files\File; use OCP\AppFramework\Db\DoesNotExistException; use OCA\Cookbook\Exception\InvalidJSONFileException; +use OCA\Cookbook\Helper\Filter\NormalizeRecipeFileFilter; use OCA\Cookbook\Helper\UserConfigHelper; use OCP\IL10N; @@ -33,6 +34,9 @@ class DbCacheService { */ private $l; + /** @var NormalizeRecipeFileFilter */ + private $normalizeFileFilter; + private $jsonFiles; private $dbReceipeFiles; private $dbKeywords; @@ -47,12 +51,14 @@ public function __construct( RecipeDb $db, RecipeService $recipeService, UserConfigHelper $userConfigHelper, + NormalizeRecipeFileFilter $normalizeRecipeFileFilter, IL10N $l ) { $this->userId = $UserId; $this->db = $db; $this->recipeService = $recipeService; $this->userConfigHelper = $userConfigHelper; + $this->normalizeFileFilter = $normalizeRecipeFileFilter; $this->l = $l; } @@ -142,9 +148,7 @@ private function parseJSONFile(File $jsonFile): array { $id = (int) $jsonFile->getParent()->getId(); $json['id'] = $id; - if (!isset($json['dateModified'])) { - $json['dateModified'] = null; - } + $json = $this->normalizeFileFilter->filter($json, $jsonFile); return $json; } diff --git a/tests/Unit/Helper/Filter/NormalizeRecipeFileFilterTest.php b/tests/Unit/Helper/Filter/NormalizeRecipeFileFilterTest.php new file mode 100644 index 000000000..d7143434d --- /dev/null +++ b/tests/Unit/Helper/Filter/NormalizeRecipeFileFilterTest.php @@ -0,0 +1,61 @@ +datesFilter = $this->createMock(RecipeDatesFilter::class); + $this->dut = new NormalizeRecipeFileFilter($this->datesFilter); + } + + public function dp() { + yield [false, false, false]; + yield [false, true, false]; + yield [false, false, false]; + yield [true, true, true]; + } + + /** @dataProvider dp */ + public function testFilter($updateRequested, $changedDates, $shouldWrite) { + $recipe = ['name' => 'The recipe']; + + $recipeA = $recipe; + $recipeA['version'] = 'from RecipeDatesFilter'; + + /** @var MockObject|File $recipeFile */ + $recipeFile = $this->createMock(File::class); + + if ($shouldWrite) { + $recipeFile->expects($this->once())->method('putContent')->with(json_encode($recipeA)); + $recipeFile->expects($this->once())->method('touch'); + } else { + $recipeFile->expects($this->never())->method('putContent'); + $recipeFile->expects($this->never())->method('touch'); + } + + $this->datesFilter->method('apply')->with($recipe, $recipeFile) + ->willReturnCallback(function (&$json, $file) use ($changedDates, $recipeA) { + $json = $recipeA; + return $changedDates; + }); + + $ret = $this->dut->filter($recipe, $recipeFile, $updateRequested); + + $this->assertEquals($recipeA, $ret); + } +} From 76240787cd623b556a5e1c7fad408df971470567 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 13:54:55 +0200 Subject: [PATCH 08/11] Update changelog Signed-off-by: Christian Wolf --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fbdc8fb5..da55a617b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ ## [Unreleased] +### Fixed +- Prevent slow loading of recipes due to iteration over all files + [#1072](https://github.com/nextcloud/cookbook/pull/1072) @christianlupus + + ## 0.9.13 - 2022-07-02 ### Added From 565ca93e38c038b9876d256ec0a356b2384ea266 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 15:41:11 +0200 Subject: [PATCH 09/11] Correct column name to make it compatible with pgsql Signed-off-by: Christian Wolf --- lib/Db/RecipeDb.php | 39 ++++++++++++++----- .../Version000000Date20220703174647.php | 8 ++-- .../Version000000Date20220703174647Test.php | 14 +++---- 3 files changed, 41 insertions(+), 20 deletions(-) diff --git a/lib/Db/RecipeDb.php b/lib/Db/RecipeDb.php index 7f02c0ea9..a0d15bfab 100755 --- a/lib/Db/RecipeDb.php +++ b/lib/Db/RecipeDb.php @@ -59,6 +59,8 @@ public function findRecipeById(int $id) { $ret = []; $ret['name'] = $row['name']; $ret['id'] = $row['recipe_id']; + $ret['dateCreated'] = $row['date_created']; + $ret['dateModified'] = $row['date_modified']; return $ret; } @@ -92,7 +94,7 @@ public function deleteRecipeById(int $id) { public function findAllRecipes(string $user_id) { $qb = $this->db->getQueryBuilder(); - $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords', 'c.name AS category']) + $qb->select(['r.recipe_id', 'r.name', 'r.date_created', 'r.date_modified', 'k.name AS keywords', 'c.name AS category']) ->from(self::DB_TABLE_RECIPES, 'r') ->where('r.user_id = :user') ->orderBy('r.name'); @@ -114,6 +116,8 @@ public function findAllRecipes(string $user_id) { $result = $cursor->fetchAll(); $cursor->closeCursor(); + $result = $this->mapDbNames($result); + $result = $this->sortRecipes($result); // group recipes, convert keywords to comma-separated list @@ -122,6 +126,17 @@ public function findAllRecipes(string $user_id) { return $this->unique($recipesGroupedTags); } + private function mapDbNames($results) { + return array_map(function ($x) { + $x['dateCreated'] = $x['date_created']; + $x['dateModified'] = $x['date_modified']; + unset($x['date_created']); + unset($x['date_modified']); + + return $x; + }, $results); + } + public function unique(array $result) { // NOTE: This post processing shouldn't be necessary // When sharing recipes with other users, they are occasionally returned twice @@ -236,7 +251,7 @@ public function getRecipesByCategory(string $category, string $user_id) { // for the recipe, but those don't seem to work: // $qb->select(['r.recipe_id', 'r.name', 'GROUP_CONCAT(k.name) AS keywords']) // not working // $qb->select(['r.recipe_id', 'r.name', DB::raw('GROUP_CONCAT(k.name) AS keywords')]) // not working - $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.date_created', 'r.date_modified', 'k.name AS keywords']) ->from(self::DB_TABLE_CATEGORIES, 'c') ->where('c.name = :category') ->andWhere('c.user_id = :user') @@ -249,7 +264,7 @@ public function getRecipesByCategory(string $category, string $user_id) { $qb->groupBy(['r.name', 'r.recipe_id', 'k.name']); $qb->orderBy('r.name'); } else { - $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.date_created', 'r.date_modified', 'k.name AS keywords']) ->from(self::DB_TABLE_RECIPES, 'r') ->leftJoin('r', self::DB_TABLE_KEYWORDS, 'k', 'r.recipe_id = k.recipe_id') ->leftJoin( @@ -271,6 +286,8 @@ public function getRecipesByCategory(string $category, string $user_id) { $result = $cursor->fetchAll(); $cursor->closeCursor(); + $result = $this->mapDbNames($result); + // group recipes, convert keywords to comma-separated list $recipesGroupedTags = $this->groupKeywordInResult($result); @@ -286,7 +303,7 @@ public function getRecipesByKeywords(string $keywords, string $user_id) { $qb = $this->db->getQueryBuilder(); - $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'kk.name AS keywords']) + $qb->select(['r.recipe_id', 'r.name', 'r.date_created', 'r.date_modified', 'kk.name AS keywords']) ->from(self::DB_TABLE_KEYWORDS, 'k') ->where('k.name IN (:keywords)') ->andWhere('k.user_id = :user') @@ -303,6 +320,8 @@ public function getRecipesByKeywords(string $keywords, string $user_id) { $result = $cursor->fetchAll(); $cursor->closeCursor(); + $result = $this->mapDbNames($result); + // group recipes, convert keywords to comma-separated list $recipesGroupedTags = $this->groupKeywordInResult($result); @@ -321,7 +340,7 @@ public function findRecipes(array $keywords, string $user_id) { $qb = $this->db->getQueryBuilder(); - $qb->select(['r.recipe_id', 'r.name', 'r.dateCreated', 'r.dateModified', 'k.name AS keywords', 'c.name AS category']) + $qb->select(['r.recipe_id', 'r.name', 'r.date_created', 'r.date_modified', 'k.name AS keywords', 'c.name AS category']) ->from(self::DB_TABLE_RECIPES, 'r'); $qb->leftJoin('r', self::DB_TABLE_KEYWORDS, 'k', 'k.recipe_id = r.recipe_id'); @@ -355,6 +374,8 @@ public function findRecipes(array $keywords, string $user_id) { $result = $cursor->fetchAll(); $cursor->closeCursor(); + $result = $this->mapDbNames($result); + // group recipes, convert keywords to comma-separated list $recipesGroupedTags = $this->groupKeywordInResult($result); @@ -462,8 +483,8 @@ public function insertRecipes(array $recipes, string $userId) { 'recipe_id' => ':id', 'user_id' => ':userid', 'name' => ':name', - 'dateCreated' => ':dateCreated', - 'dateModified' => ':dateModified', + 'date_created' => ':dateCreated', + 'date_modified' => ':dateModified', ]); $qb->setParameter('userid', $userId); @@ -497,8 +518,8 @@ public function updateRecipes(array $recipes, string $userId) { $dateCreated = $this->parseDate($recipe['dateCreated']); $dateModified = $this->parseDate($recipe['dateModified']); - $qb->set('dateCreated', $qb->createNamedParameter($dateCreated, IQueryBuilder::PARAM_DATE)); - $qb->set('dateModified', $qb->createNamedParameter($dateModified, IQueryBuilder::PARAM_DATE)); + $qb->set('date_created', $qb->createNamedParameter($dateCreated, IQueryBuilder::PARAM_DATE)); + $qb->set('date_modified', $qb->createNamedParameter($dateModified, IQueryBuilder::PARAM_DATE)); $qb->setParameter('id', $recipe['id']); $qb->setParameter('uid', $userId); diff --git a/lib/Migration/Version000000Date20220703174647.php b/lib/Migration/Version000000Date20220703174647.php index 9eecd96b4..9cb16ce8e 100644 --- a/lib/Migration/Version000000Date20220703174647.php +++ b/lib/Migration/Version000000Date20220703174647.php @@ -33,14 +33,14 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt $table = $schema->getTable('cookbook_names'); - if (! $table->hasColumn('dateCreated')) { - $table->addColumn('dateCreated', 'datetime_immutable', [ + if (! $table->hasColumn('date_created')) { + $table->addColumn('date_created', 'datetime_immutable', [ 'notnull' => false, ]); } - if (!$table->hasColumn('dateModified')) { - $table->addColumn('dateModified', 'datetime_immutable', [ + if (!$table->hasColumn('date_modified')) { + $table->addColumn('date_modified', 'datetime_immutable', [ 'notnull' => false, ]); } diff --git a/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php b/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php index c8a48bb0d..0a6f679c1 100644 --- a/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php +++ b/tests/Migration/Setup/Migrations/Version000000Date20220703174647Test.php @@ -28,27 +28,27 @@ public function testRedundantEntriesInDB(/*$data, $updatedUsers*/) { $this->assertEquals(1, $qb->execute()); $table = $this->schema->getTable('cookbook_names'); - $this->assertFalse($table->hasColumn('dateCreated')); - $this->assertFalse($table->hasColumn('dateModified')); + $this->assertFalse($table->hasColumn('date_created')); + $this->assertFalse($table->hasColumn('date_modified')); // Run the migration under test $this->migrationService->migrate('000000Date20220703174647'); $this->renewSchema(); $table = $this->schema->getTable('cookbook_names'); - $this->assertTrue($table->hasColumn('dateCreated')); - $this->assertTrue($table->hasColumn('dateModified')); + $this->assertTrue($table->hasColumn('date_created')); + $this->assertTrue($table->hasColumn('date_modified')); $qb = $this->db->getQueryBuilder(); - $qb->select('dateCreated', 'dateModified')->from('cookbook_names'); + $qb->select('date_created', 'date_modified')->from('cookbook_names'); $res = $qb->execute(); $data = $res->fetchAll(); $this->assertEquals(1, count($data)); $row = $data[0]; $this->assertEquals(2, count($row)); - $this->assertNull($row['dateCreated']); - $this->assertNull($row['dateModified']); + $this->assertNull($row['date_created']); + $this->assertNull($row['date_modified']); //$this->assertEquals([null, null], $data[0]); From 390166f4ee35d2ca895dc0178c8ac294deedc458 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 16:56:07 +0200 Subject: [PATCH 10/11] Fix bug with parsing dated with time zone annottation Signed-off-by: Christian Wolf --- lib/Helper/Filter/DB/RecipeDatesFilter.php | 2 +- lib/Service/DbCacheService.php | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/Helper/Filter/DB/RecipeDatesFilter.php b/lib/Helper/Filter/DB/RecipeDatesFilter.php index d86cdfe98..a560b18fc 100644 --- a/lib/Helper/Filter/DB/RecipeDatesFilter.php +++ b/lib/Helper/Filter/DB/RecipeDatesFilter.php @@ -21,7 +21,7 @@ class RecipeDatesFilter implements AbstractRecipeFilter { private const DATE_MODIFIED = 'dateModified'; private const PATTERN_DATE = '\d{4}-\d{2}-\d{2}'; - private const PATTERN_TIME = '\d{1,2}:\d{2}:\d{2}(?:\.\d+)?'; + private const PATTERN_TIME = '\d{1,2}:\d{2}:\d{2}(?:\.\d+)?(?:[+-]\d{4}|[zZ]|[+-]\d\d:\d\d|[+-]\d\d| *UTC)?'; /** @var string */ private $patternDate; diff --git a/lib/Service/DbCacheService.php b/lib/Service/DbCacheService.php index dd6d044ef..9a6e7d5b1 100644 --- a/lib/Service/DbCacheService.php +++ b/lib/Service/DbCacheService.php @@ -47,13 +47,13 @@ class DbCacheService { private $updatedRecipes; public function __construct( - ?string $UserId, - RecipeDb $db, - RecipeService $recipeService, - UserConfigHelper $userConfigHelper, - NormalizeRecipeFileFilter $normalizeRecipeFileFilter, - IL10N $l - ) { + ?string $UserId, + RecipeDb $db, + RecipeService $recipeService, + UserConfigHelper $userConfigHelper, + NormalizeRecipeFileFilter $normalizeRecipeFileFilter, + IL10N $l + ) { $this->userId = $UserId; $this->db = $db; $this->recipeService = $recipeService; @@ -148,7 +148,7 @@ private function parseJSONFile(File $jsonFile): array { $id = (int) $jsonFile->getParent()->getId(); $json['id'] = $id; - $json = $this->normalizeFileFilter->filter($json, $jsonFile); + $json = $this->normalizeFileFilter->filter($json, $jsonFile, true); return $json; } @@ -204,7 +204,7 @@ private function compareReceipeLists() { if (array_key_exists($id, $this->dbReceipeFiles)) { // The file was at least in the database - if (! $this->isDbEntryUpToDate($id)) { + if (!$this->isDbEntryUpToDate($id)) { // An update is needed $this->updatedRecipes[] = $id; } @@ -290,7 +290,7 @@ private function updateCategories() { * @return bool */ private function hasJSONCategory(array $json): bool { - return ! is_null($this->getJSONCategory($json)); + return !is_null($this->getJSONCategory($json)); } /** @@ -336,7 +336,7 @@ private function updateKeywords() { return trim($v); }, $keywords); $keywords = array_filter($keywords, function ($v) { - return ! empty($v); + return !empty($v); }); $dbKeywords = $this->dbKeywords[$rid]; From 799b5a567f330a38b6482100182da2ad681247d7 Mon Sep 17 00:00:00 2001 From: Christian Wolf Date: Wed, 6 Jul 2022 17:13:09 +0200 Subject: [PATCH 11/11] Added more tests for different date and time formats Signed-off-by: Christian Wolf --- .../Filter/DB/RecipeDatesFilterTest.php | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php b/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php index 88d263018..516efd15b 100644 --- a/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php +++ b/tests/Unit/Helper/Filter/DB/RecipeDatesFilterTest.php @@ -58,6 +58,52 @@ public function testFilterFromJson($created, $modified, $expectedCreation, $expe $this->assertEquals($copy, $recipe, 'Other entries must not change.'); } + public function dpDateFormats() { + return [ + ['2022-07-05'], + ['2022-07-05T15:30:00'], + ['2022-07-05T15:30:00.123'], + ['2022-07-05T15:30:00z'], + ['2022-07-05T15:30:00Z'], + ['2022-07-05T15:30:00 UTC'], + ['2022-07-05T15:30:00+0100'], + ['2022-07-05T15:30:00-0100'], + ['2022-07-05T15:30:00+01:00'], + ['2022-07-05T15:30:00-01:00'], + ['2022-07-05T15:30:00+01'], + ['2022-07-05T15:30:00-01'], + ['2022-07-05T15:30:00.123+01:00'], + ]; + } + + /** + * @dataProvider dpDateFormats + * @param mixed $date + */ + public function testDateFormats($date) { + $recipe = [ + 'name' => 'my Recipe', + 'dateCreated' => $date, + 'dateModified' => $date + ]; + $copy = $recipe; + + $file = $this->createStub(File::class); + + $ret = $this->dut->apply($recipe, $file); + + $this->assertFalse($ret, 'Reporting of modification status'); + $this->assertEquals($date, $recipe['dateCreated'], 'Wrong creation date'); + $this->assertEquals($date, $recipe['dateModified'], 'Wrong modification date'); + + unset($recipe['dateCreated']); + unset($recipe['dateModified']); + unset($copy['dateCreated']); + unset($copy['dateModified']); + + $this->assertEquals($copy, $recipe, 'Other entries must not change.'); + } + public function dpFromFile() { yield ['2022-07-06T09:08:54+0000', false, false, 1657098534, 1657098535, 1657098536]; yield ['2022-07-06T09:08:55+0000', false, false, 0, 1657098535, 1657098536];