Преглед изворни кода

Fix tagging an already-tagged entry on PostgreSQL (#9136)

* Complete two source TODOs: negative UTC offset date parsing and PostgreSQL tag insert

_noDelimit() stripped '-' and ':' from the whole ISO 8601 string, including a
trailing UTC offset, so a negative offset such as "-05:00" silently lost its
sign and corrupted the parsed date. Extract a trailing offset (or "Z") before
stripping delimiters and re-append it unchanged.

FreshRSS_TagDAOPGSQL::sqlIgnore() returned an empty string as a placeholder,
so re-tagging an already-tagged entry produced a real SQL error on PostgreSQL
instead of being a silent no-op like on MySQL/SQLite. Replace the sqlIgnore()
keyword-injection approach with the sqlIgnoreConflict(string $sql): string
pattern already used by EntryDAO, and implement it for PostgreSQL with
"ON CONFLICT DO NOTHING".

Fixes #7923.

Co-Authored-By: Claude <noreply@anthropic.com>

* Drop the negative-UTC-offset date fix, already covered by #9071

#9071 already fixes this exact bug in lib_date.php, so keeping this
PR focused on the PostgreSQL tag-insert fix only.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Gerard Alvear <gerard.alvear@logiqd.me>
Co-authored-by: Claude <noreply@anthropic.com>
Gerard Alvear Porras пре 5 дана
родитељ
комит
9b40f04f32
5 измењених фајлова са 43 додато и 13 уклоњено
  1. 1 0
      CHANGELOG.md
  2. 8 9
      app/Models/TagDAO.php
  3. 2 2
      app/Models/TagDAOPGSQL.php
  4. 2 2
      app/Models/TagDAOSQLite.php
  5. 30 0
      tests/app/Models/TagDAOTest.php

+ 1 - 0
CHANGELOG.md

@@ -21,6 +21,7 @@ See also [the FreshRSS releases](https://github.com/FreshRSS/FreshRSS/releases).
 	* New per-feed option to show or hide enclosures (attachments) [#4999](https://github.com/FreshRSS/FreshRSS/issues/4999)
 * Bug fixing
 	* Fix lost elements while parsing search query [#8884](https://github.com/FreshRSS/FreshRSS/pull/8884)
+	* Fix tagging an already-tagged entry on PostgreSQL raising a SQL error instead of being a no-op [#7923](https://github.com/FreshRSS/FreshRSS/issues/7923)
 * CLI
 	* New `cli/reconfigure-user.php` to read/write per-user config attributes [#8873](https://github.com/FreshRSS/FreshRSS/pull/8873)
 * API

+ 8 - 9
app/Models/TagDAO.php

@@ -3,8 +3,8 @@ declare(strict_types=1);
 
 class FreshRSS_TagDAO extends Minz_ModelPdo {
 
-	public function sqlIgnore(): string {
-		return 'IGNORE';
+	public static function sqlIgnoreConflict(string $sql): string {
+		return str_replace('INSERT INTO ', 'INSERT IGNORE INTO ', $sql);
 	}
 
 	public function sqlResetSequence(): bool {
@@ -303,10 +303,9 @@ class FreshRSS_TagDAO extends Minz_ModelPdo {
 
 	public function tagEntry(int $id_tag, string $id_entry, bool $checked = true): bool {
 		if ($checked) {
-			$ignore = $this->sqlIgnore();
-			$sql = <<<SQL
-				INSERT {$ignore} INTO `_entrytag`(id_tag, id_entry) VALUES(:id_tag, :id_entry)
-				SQL;
+			$sql = static::sqlIgnoreConflict(<<<'SQL'
+				INSERT INTO `_entrytag`(id_tag, id_entry) VALUES(:id_tag, :id_entry)
+				SQL);
 		} else {
 			$sql = <<<'SQL'
 				DELETE FROM `_entrytag` WHERE id_tag=:id_tag AND id_entry=:id_entry
@@ -331,9 +330,8 @@ class FreshRSS_TagDAO extends Minz_ModelPdo {
 	 */
 	public function tagEntries(iterable $addLabels): int|false {
 		$hasValues = false;
-		$ignore = $this->sqlIgnore();
-		$sql = <<<SQL
-			INSERT {$ignore} INTO `_entrytag`(id_tag, id_entry) VALUES
+		$sql = <<<'SQL'
+			INSERT INTO `_entrytag`(id_tag, id_entry) VALUES
 			SQL;
 		foreach ($addLabels as $addLabel) {
 			$id_tag = (int)($addLabel['id_tag'] ?? 0);
@@ -347,6 +345,7 @@ class FreshRSS_TagDAO extends Minz_ModelPdo {
 		if (!$hasValues) {
 			return false;
 		}
+		$sql = static::sqlIgnoreConflict($sql);
 
 		$affected = $this->pdo->exec($sql);
 		if ($affected !== false) {

+ 2 - 2
app/Models/TagDAOPGSQL.php

@@ -4,8 +4,8 @@ declare(strict_types=1);
 class FreshRSS_TagDAOPGSQL extends FreshRSS_TagDAO {
 
 	#[\Override]
-	public function sqlIgnore(): string {
-		return '';	//TODO
+	public static function sqlIgnoreConflict(string $sql): string {
+		return rtrim($sql, ' ;') . ' ON CONFLICT DO NOTHING';
 	}
 
 	#[\Override]

+ 2 - 2
app/Models/TagDAOSQLite.php

@@ -4,8 +4,8 @@ declare(strict_types=1);
 class FreshRSS_TagDAOSQLite extends FreshRSS_TagDAO {
 
 	#[\Override]
-	public function sqlIgnore(): string {
-		return 'OR IGNORE';
+	public static function sqlIgnoreConflict(string $sql): string {
+		return str_replace('INSERT INTO ', 'INSERT OR IGNORE INTO ', $sql);
 	}
 
 	#[\Override]

+ 30 - 0
tests/app/Models/TagDAOTest.php

@@ -0,0 +1,30 @@
+<?php
+declare(strict_types=1);
+
+/**
+ * Tests for the database-specific `INSERT ... ON DUPLICATE` variants used when tagging entries.
+ */
+final class TagDAOTest extends \PHPUnit\Framework\TestCase {
+	private const SQL = 'INSERT INTO `_entrytag`(id_tag, id_entry) VALUES(:id_tag, :id_entry)';
+
+	public function test_sqlIgnoreConflict_forMySQL(): void {
+		self::assertSame(
+			'INSERT IGNORE INTO `_entrytag`(id_tag, id_entry) VALUES(:id_tag, :id_entry)',
+			FreshRSS_TagDAO::sqlIgnoreConflict(self::SQL),
+		);
+	}
+
+	public function test_sqlIgnoreConflict_forSQLite(): void {
+		self::assertSame(
+			'INSERT OR IGNORE INTO `_entrytag`(id_tag, id_entry) VALUES(:id_tag, :id_entry)',
+			FreshRSS_TagDAOSQLite::sqlIgnoreConflict(self::SQL),
+		);
+	}
+
+	public function test_sqlIgnoreConflict_forPostgreSQL(): void {
+		self::assertSame(
+			'INSERT INTO `_entrytag`(id_tag, id_entry) VALUES(:id_tag, :id_entry) ON CONFLICT DO NOTHING',
+			FreshRSS_TagDAOPGSQL::sqlIgnoreConflict(self::SQL),
+		);
+	}
+}