search

contao / core

Contao 3 → see contao/contao for Contao 4
GNU Lesser General Public License v3.0
490 stars 213 forks source link

Duplicate entry for key 'url' INSERT INTO tl_search #8498

Closed BugBuster1701 closed 8 years ago

BugBuster1701 commented 8 years ago

Ähnlich dem #8460 . Contao 3.5.17 hat zwei Seitenbäume, für Sprache de und en bei gleicher Domain. Die Sprachkürzel in der URL sind nicht aktiviert! Home Alias bei "de" ist "index", bei "en" ist "home".

Ruft man nun mittels de und en Browser die Home Seiten ohne Alias auf, wird zweimal versucht die Url http://domain.xy/ einzutragen.

leofeyer commented 8 years ago

Also dürfen wir nie die leere URL indizieren, wenn addLanguageToUrl deaktiviert ist, richtig?

leofeyer commented 8 years ago

@contao/developers /cc

fritzmg commented 8 years ago

Oder nur die Fallback Sprache indizieren in so einem Fall.

ausi commented 8 years ago

Also dürfen wir nie die leere URL indizieren, wenn addLanguageToUrl deaktiviert ist, richtig?

IMO ja, denn wenn dieselbe URL zwei verschiedene Seiten/Inhalte liefern kann, darf nichts unter dieser URL indiziert werden. Ist die Seite mit dem Alias index dann eigentlich noch über eine andere URL abrufbar?

leofeyer commented 8 years ago

Vielleicht ist das auch ein Fehler im Routing? Denn eigentlich muss der Browser unter der leeren Domain immer dieselbe Seite anzeigen, nämlich das Index-Dokument.

fritzmg commented 8 years ago

Contao zeigt bei einem leeren Request aber einfach die erste Seite des jeweiligen Seitebaums an. Contao ermittelt also in diesem Fall zuerst den richtigen Seitenbaum anhand der Browser-Sprache und zeigt da die erste (published) Seite an - egal ob die den Alias index hat oder nicht.

leofeyer commented 8 years ago

Was aber falsch sein könnte, wie wir jetzt durch den hier diskutierten Fehler bemerken. Auch Google zeigt ja nicht unter ein und derselben Domain verschiedene Seiten an, oder?

KaiserCh commented 8 years ago

Google nicht, da die zig lokalisierte Domains haben, aber das -Attribut "lang" wird soweit ich weiß durchaus hinsichtlich duplicated content & Co in Betracht gezogen. Wenn die Startseiten beider Sprachen im Endeffekt den selben Content enthalten, nur eben lokalisiert, dürfte eine identische URL nicht vollkommen falsch sein, solange die deklarierte Sprache stimmt.

So gesehen wäre eine möglich Lösung für unser Problem hier, den Unique Key einfach um eine weitere Spalte für das Sprach-Kürzel zu ergänzen.

BugBuster1701 commented 8 years ago

Also dürfen wir nie die leere URL indizieren, wenn addLanguageToUrl deaktiviert ist, richtig?

Doch, es heißt ja nicht, dass es einen weiteren Seitenbaum gibt. Hat man nur einen, warum soll ich dann addLanguageToUrl aktivieren?

BugBuster1701 commented 8 years ago

Google mag ja nur eine Sprache aufnehmen, ist hier aber auch egal. Eine Webseite mit zwei Sprachen (+Seitenbäumen) möchte auch sprachabhängig suchen können.

Bräuchte man hier nicht einfach nur die Sprache (language) mit aufnehmen?

fritzmg commented 8 years ago

Google nicht, da die zig lokalisierte Domains haben, aber das -Attribut "lang" wird soweit ich weiß durchaus hinsichtlich duplicated content & Co in Betracht gezogen. Wenn die Startseiten beider Sprachen im Endeffekt den selben Content enthalten, nur eben lokalisiert, dürfte eine identische URL nicht vollkommen falsch sein, solange die deklarierte Sprache stimmt.

Trotzdem kann man dann aber nicht dediziert unter example.org/ einen bestimmten Content aufrufen. Dieser hängt vom Request ab (User-Agent).

KaiserCh commented 8 years ago

Ja, der User-Agent ändert den Content. Aber nichts anderes sagt der vary:User-Agent - Header, der z.B. auch von contao.org ausgesendet wird. Contao macht davon sogar regen Gebrauch, um dem body Klassen zuzuweisen.

Nach genauerer Konsultation der Google-Guidelines lag ich mit meiner ersten Einschätzung aber falsch. Google achtet nicht auf <html lang="...", sondern ermittelt die Sprache einer Seite anhand des Contents. SEO-technisch wäre es also falsch, unter "domain.tld/" mehrere verschiedene Sprachen auszuliefern, auch wenn der vary:User-Agent - Header sowas durchaus zu lässt.

Auch wenn n zusätzliches Feld im Unique-Key das ursprüngliche Problem beseitigen würde, wäre eine umfassendere Lösung besser. Wie wäre es, wenn bei obigem Szenario (Multi-Lang, kein Lang-Parameter in der URL) beim Aufruf der Domain in einer anderen als der Fallback-Sprache ein Redirect auf domain.tld/sprachstart-alias.html erfolgt? Müsste dann aber n 302 sein, damit Browser den Redirect nicht cachen & Suchmaschinen ihn nicht fest indizieren.

asaage commented 8 years ago

Ich hatte mir schon des öfteren gewünscht den Lang-Parameter bei jeder Sprache AUßER der Fallback-Sprache automatisch in die url hängen zu können - das wäre wohl auch eine Möglichkeit dieses Problem anzugehen.

leofeyer commented 8 years ago

SEO-technisch wäre es also falsch, unter "domain.tld/" mehrere verschiedene Sprachen auszuliefern, auch wenn der vary:User-Agent - Header sowas durchaus zu lässt.

So sehe ich das auch.

Wie wäre es, wenn bei obigem Szenario (Multi-Lang, kein Lang-Parameter in der URL) beim Aufruf der Domain in einer anderen als der Fallback-Sprache ein Redirect auf domain.tld/sprachstart-alias.html erfolgt?

Entweder so (leere Domain == Fallback-Sprache) oder anhand des Index-Dokuments (leere Domain == Seite mit Alias "index"). Letzteres wäre näher am normalen Verhalten eines Webservers.

KaiserCh commented 8 years ago

Das mit dem Alias "index" setzt aber voraus, dass jeder Webmaster bei der Startseite seiner Fallback-Sprache eben "index" setzt, bzw. überhaupt "index" irgendwo verwendet. Auch wenn es sinnvoll und empfohlen ist, die Startseite index zu nennen, damit domain.tld/index.html = domain.tld/, ist es eben keine Pflicht. Somit wäre so eine Änderung ein BC-Break.

Leere Domain = Fallback ist evtl. SEO-technisch und semantisch nicht DIE ultimative Lösung, aber sie gewährleistet Abwärtskompatibilität. Oder natürlich "index, wenn index vorhanden, sonst Fallback".

fritzmg commented 8 years ago

On that note, there are currently 3 URLs for the startpage in Contao:

The changelanguage extension creates URLs to the root page for example (see https://github.com/terminal42/contao-changelanguage/issues/81) - but that URL simply displays the contents of the start page.

leofeyer commented 8 years ago

That is not correct. If you open the root page URL, you are being redirected.

fritzmg commented 8 years ago

True, I remembered that wrong.

KaiserCh commented 8 years ago

This still leaves us 2 options, domain.tld/ and domain.tld/alias.html (if alias != index). From a SEO point of view, this is total garbage if not compensated with a canonical-tag. For the Contao search, I assume that it will handle "domain.tld/" and "domain.tld/alias-of-start.html" as 2 different pages, even though they are the same. But I'm not sure of this aspect. If my assumption is correct, this needs fixing, too.

leofeyer commented 8 years ago

As discussed in Mumble on October 6th, we should 302 redirect to the respective root page in this case. Only if there is a page with the alias index and the page language matches, the page can be delivered under the empty domain.

leofeyer commented 8 years ago

Fixed in f42f2a41b13a768fccb033f109c9e3eca8b86e47.

The fix also fixes the "Do not redirect empty URLs" feature, which obviously nobody is using, because nobody ever complained that it was broken. 😄

asaage commented 8 years ago

"Do not redirect empty URLs"

i never got what it's used for 😄

fritzmg commented 8 years ago

Leider besteht das Problem immer noch, wahrscheinlich im Zusammenhang mit codefog/contao-cookiebar (siehe https://github.com/codefog/contao-cookiebar/issues/21) - allerdings ist es sehr schwer direkt zu reproduzieren, es tritt immer erst nach einer gewissen Zeit oder unter gewissen Umständen auf. Diese Umstände habe ich noch nicht herausgefunden.

Das einzige was ich herausgefunden habe ist, dass sich (vor cookiebar Version 1.1.2) die checksum einer page manchmal ändern konnte, wenn eben ein neuer User die Seite betritt, der das COOKIEBAR… cookie noch nicht gesetzt hat. Dadurch änderte sich der Content und dadurch die Checksum (vor Version 1.1.2 wurde die cookiebar nicht vom indexer ausgenommen). Das alleine macht, normalerweise, kein Problem - \Seach::indexPage aktualisiert dann einfach nur die checksum.

Im Code von der cookiebar Extension habe ich sonst nichts außergewöhnliches gefunden. Ich werde in einer betroffenen Installation mal ein ausgiebiges Logging einbauen um vielleicht näheres zu finden. Aber vielleicht hat jemand anders noch eine Idee?

fritzmg commented 8 years ago

Eine MySQL Frage hätte ich noch: laut der Fehlermeldung

[30-Nov-2016 06:51:51 CET] PHP Fatal error: Uncaught exception 'Exception' with message 'Query error: Duplicate entry 'f8a8cb99b33e996eaff1cc514d33f378-334' for key 'checksum_pid' (UPDATE tl_search SET … WHERE id='3')' thrown in …/system/modules/core/library/Contao/Database/Statement.php on line 295
#0 …/system/modules/core/library/Contao/Database/Statement.php(264): Contao\Database\Statement->query()
#1 …/system/modules/core/library/Contao/Search.php(210): Contao\Database\Statement->execute('3')
#2 …/system/modules/core/classes/FrontendTemplate.php(330): Contao\Search::indexPage(Array)
#3 …/system/modules/core/classes/FrontendTemplate.php(124): Contao\FrontendTemplate->addToSearchIndex()
#4 …/system/modules/core/pages/PageRegular.php(190): Contao\FrontendTemplate->output(true)
#5 …/system/modules/core/controllers/FrontendIndex.php(285): Contao\PageRegular->generate(Object(Contao\PageModel), true)
#6 …/index.php(20): Contao\FrontendIndex->run()
#7 {main}

passiert die Fehlermeldung hier: Search.php#L210, also bei

$objDatabase->prepare("UPDATE tl_search %s WHERE id=?")
            ->set($arrSet)
            ->execute($objIndex->id);

Aber wie kann das sein? Ich meine wie kann ein UPDATE, dass auf eine id begrenzt ist, einen Duplicate entry … for key 'checksum_pid' verursachen? Das müsste ja bedeuten, dass die aktualisierte checksum und die aktualisierte pid bereits für eine andere id existiert, oder nicht?

fritzmg commented 8 years ago

Ok, ich denke ich habe das Problem gefunden: https://github.com/codefog/contao-cookiebar/issues/21#issuecomment-263834643

Problem ist im Endeffekt, dass durch die Cookiebar Extension es zu mehren Einträgen in tl_search kommen kann unter der selben pid und der selben url - allerdings mit unterschiedlichen GET Parametern und durch die Cookiebar Extension entweder unterschiedlichen oder gleichen checksums (weil einmal die Cookiebar im Inhalt ausgegeben wird und ein andermal nicht).

Im Zusammenhang mit der Cookiebar Extension lässt sich das Problem einfach lösen: auf Version 1.1.2 aktualisieren (das fügt <!-- indexer::stop -->…<!-- indexer::continue --> in das cookiebar template ein) und danach den Suchindex löschen lassen.

Dieses Problem könnte aber auch in anderen Zusammenhängen auftreten, also immer dann, wenn auch durch Cookies, anstatt nur durch GET Parameter, anderer Inhalt erzeugt wird, welcher vom Search Indexer erfasst wird (also nicht mit <!-- indexer::stop --> ausgenommen ist).

KaiserCh commented 8 years ago

Hm, müsste man dann also in Contao quasi eine Cookie-Liste führen, die den Content ändern (können) und beim Suchindex auch die Existenz dieser Cookies mit in Betracht ziehen? Da hast du exponentielles Wachstum des Suchindex. Da würde ich das Problem doch lieber auf die Verantwortung der Extension-Schreiber verlagern. Durch Cookies variierter Content sollte nicht im Suchindex auftauchen bzw. nur die Version ohne Cookie.

Der Fehler geht aber sogar noch etwas weiter. Contao sendet 2 Werte im Vary-Header: Accept-Encoding & User-Agent. Sobald ein Cookie den Content ändert, müsste eigentlich auch noch der Vary-Header angepasst werden. Sonst gibts Ärger mit vorgelagerten Caches. Hab vor längerer Zeit mal mit Contao+Varnish herum gepfuscht, das ist verdammt krampfig.

fritzmg commented 8 years ago

Oder das Indexing wird in Zukunft gar nicht mehr bei einem regulären Frontend Aufruf gemacht, sondern nur mehr über den neuen Indexer.

KaiserCh commented 8 years ago

Das löst das Problem nicht für v3.5. Außerdem hatte ich schon ein paar Fälle mit Custom Mods, bei denen nicht jede mögliche valide Kombination von URL Parametern auch im Backend-Indexer erfasst wird.

Für einen neuen Indexer könnte man natürlich sagen: Trage jede aufgerufene URL inkl. Parameter einmalig in ne Cron-Liste ein, der Cronjob macht dann das Indexing ohne Kekse. Dann hat man ein Ähnliches Verhalten wie jetzt hinsichtlich URLs, aber das Keksproblem entfällt.

fritzmg commented 8 years ago

Das löst das Problem nicht für v3.5

Ja, ich meinte für Contao 3.5 muss eben im core und bei Extensions darauf geachtet werden, dass es keine indexierbaren, per Cookie gesteuerten Inhalte geben kann.

Ansonsten habe ich auch keine gute Idee wie man das anderweitig lösen könnte. Evt. müsste man, before man tl_search aktualisiert, nochmal überpüfen ob für die pid und checksum es bereits einen anderen Eintrag gibt und dann dementsprechend agieren?

KaiserCh commented 8 years ago

Also lautet die Antwort als kompakter Hack: $objDatabase->prepare("UPDATE IGNORE tl_search %s WHERE id=?") ....

Toflar commented 8 years ago

Für die Implementation in Contao 4 können wir Cookies natürlich nicht einfach ignorieren, aber Einführung für entsprechende Vary-Header hab ich mir auf die Fahne geschrieben. Das wird also verbessert werden. Das Problem ist, dass Cookie nicht als Vary geeignet ist, weil beliebige Info daran hängt. Die Liste ist also auch nich praktikabel (z.B. das PHP-Session Cookie wäre das selbe aber abhängig von der Session kann man tausende Dinge am Inhalt ändern). Für Contao 3.5 würde ich wohl tatsächlich den Eintrag ignorieren, wenn er bereits existiert.

leofeyer commented 7 years ago

@contao/developers Soll ich also wirklich das UPDATE in ein UPDATE IGNORE ändern? Die fehlenden Indexer-Kommentare in der Cookiebar-Erweiterung würde ich eher als ein Bug in der Erweiterung betrachten, der ja auch bereits gefixt wurde.

Würde ein UPDATE IGNORE nicht eher andere Bugs verschleiern, weil eben keine Exception mehr kommt?

Toflar commented 7 years ago

Das stimmt so nicht ganz. Der Inhalt könnte beliebig verändert werden aufgrund eines Cookies. Es ist aber richtig, dass dieser Inhalt dann meistens auch nicht indexiert werden muss. Z.B. ein Warenkorb etc. pp. Aber der Suchindex sollte sich daran auch nicht stören, weil es absolut valid ist, auf derselben URL verschiedenen Inhalt zu haben. Genau dafür ist ja eben der Vary header da :-) Insofern ist das ganz klar ein Fehlverhalten von Contao im Moment.

discordier commented 7 years ago

@Toflar Ich sehe da durchaus ein Problem wenn in diesem Falle dann dummerweise der Cookie-based Content im Index landet und der "richtige" nicht mehr reinkommt und ich nichtmal einen Fehler bekomme.

Dass es sich hierbei um ein Fehlverhalten handelt ist, denke ich, unbestritten. Dennoch sollten wir nicht ein Fehlverhalten durch ein anderes tauschen.

Toflar commented 7 years ago

Da bin ich ja bei dir, ich hab keine Aussage getroffen, ausser dass es jetzt falsch ist.

discordier commented 7 years ago

Dann habe ich dich falsch verstanden, ich dachte dass du pro UPDATE IGNORE seist.

Ich bin aus o.g. Grund dagegen.

Toflar commented 7 years ago

Imho sollten wir in der 3.5 das Indizieren ignorieren wenn credentialed requests reinkommen. Also sprich Authorization oder Cookie in den HTTP headers vorkommen. WDYT?

Toflar commented 7 years ago

Ah, dann hätten wir keine personalisierten Suchergebnisse mehr.

discordier commented 7 years ago

Exakt, we are doomed. :smiley_cat: Das ist btw. auch eins der Probleme die ich in meinem crawler in C4 habe, ich kann nicht personalisieren.

Toflar commented 7 years ago

Es geht halt einfach nicht. Es ist wieder eins dieser Probleme die sich einfach nicht lösen lassen in 3.5. In der 4 können wir anständige Vary header setzen und dann passt das, dann geht es auch für deinen Crawler.

leofeyer commented 7 years ago

Aber Google berücksichtigt den Vary-Header auch nicht beim Indizieren. Das hat @KaiserCh oben schon erläutert.

discordier commented 7 years ago

Die praesentieren aber auch nicht jedem Besucher einen anderen Inhalt sondern jedem denselben?

Toflar commented 7 years ago

Das ist ja auch irrelevant. Es geht darum was unsere Suche macht und im Moment geht sie davon aus, dass unter der selben URL immer derselbe Content existiert was ein Fehler ist, denn dem ist einfach nicht so. Die URL ist nie unique. Sie ist unique zusammen mit allen Vary Headern und mit denen kann weder unser Contao 3.5 HTTP-Cache noch unsere Suche umgehen. Uns fehlt nebst der Unterstützung für die Vary Header auch das senden dieser Header, nämlich für

Und die hängen am Cookie, man muss sie also erst noch mit Vorab-Requests aufsplitten. Das ist ein riesiger Aufwand. Es ist deshalb m.M.n. zum jetzigen Zeitpunkt unmöglich dieses Problem zu fixen. Die Suche muss davon ausgehen, dass auf der selben URL verschiedener Inhalt ist.

KaiserCh commented 7 years ago

Zumindest für alle Content-Änderungen, die nicht vom Session-Cookie, sondern x-beliebigen weiteren abhängen (wie z.B. bei der Cookiebar-Extension) gäbe es eine schnelle und nicht all zu hässliche Lösung:

Hilft natürlich nix für Dinge aus der Session, z.B. wenn eine Extension eine sortier- oder filterbare Liste darstellt, die Filter-Einstellungen in der Session lagert und die Liste nicht vom Indexer ausschließt. Aber irgend was ist ja immer...

fritzmg commented 7 years ago

Is there any plan to accommodate this in a bugfix version in Contao 3? While we identified the problem with extensions likes visitors and cookiebar there are still sporadic reports on the community forum about this - while having the most recent version of the aforementioned extensions.

A Google search still reveals a lot of websites that suffer from this problem - though most of them haven't been updated to the most recent Contao LTS version, otherwise the error would not be visible in the front end.

Toflar commented 7 years ago

Imho this needs to be reopened, yes. The only question is whether we should go for ON DUPLICATE KEY UPDATE or ignore :)

leofeyer commented 7 years ago

Correct. I have already asked this question on December 15th, 2016, but never got an answer.

Toflar commented 7 years ago

Both ways are possibly incorrect, so what. Just pick one :D I think I'd just ignore it.

KaiserCh commented 7 years ago

@Toflar What do you mean regarding ON DUPLICATED KEY UPDATE? The problem here exists because of an UPDATE statement. UPDATE does not have a syntax like UPDATE ON DUPLICATE KEY UPDATE. The only option regarding duplicates is IGNORE. Only INSERT statements support an optional UPDATE.

What would happen if we set it to ON DUPLICATE KEY IGNORE and change regular content of the page, e.g. add/modify a content element? Wouldn't in this case the index still contain the old content until someone issues a full reindex from backend? If this is the case, IGNORE is no option at all. But the main problem still persists. I think I've seen one of those Update-Duplicate-Errors this week on one of our projects, and it should be a current Contao installation, at least as current as the mail-related security update.

I think it's wrong to simply say "Check your extensions". While old extensions might be the reason this problem exists, these extensions were not resulting in this kind of error for older v3.5-releases. Since the core-updates on LTS-release should not result in backward compatibility issues, it is wrong to expect every extension to adapt to the modified search behaviour with checksum_pid. If an extension was working with 3.5.0, it should work with 3.5.999.

Oh, and a Google-search for "contao Query error: Duplicate entry" reveals pure horror...

Toflar commented 7 years ago

Yeah obviously I mean the update statement.

What would happen if we set it to ON DUPLICATE KEY IGNORE and change regular content of the page, e.g. add/modify a content element? Wouldn't in this case the index still contain the old content until someone issues a full reindex from backend? If this is the case, IGNORE is no option at all.

Yeah, as I said. It's a problem we cannot fix. If your content element provides different output for the same URL based on something like the session, the content changes. You cannot determine which change was correct. We can always update the entry instead of ignoring it but anyway it leads to inconsistent search results no matter what way we choose. It cannot be fixed properly without huge changes that are not feasible for Contao 3.5 and require major efforts in Contao 4.