"Integrity constraint violation: 1062 Duplicate entry" error when updating a node

This looks like it might be a redirect module issue?

We can just move this straight to Redirect.

I guess it affects the D8 branch?

Will need steps to reproduce, looks like somehow you manage to create redirects with the same hash?

holder_5] => about/staff/guarini-joe\n [:db_insert_placeholder_6] => a:0:{}\n [:db_insert_placeholder_7] => internal:/node/48\n [

this is the from => to redirect it is trying to create. are you changing the name of those staff entries? Can you check your redirect table for the record with hash 1zvDyLS_nqAUEDQSKpZXidjJJeLUMXrvAdoCL6x9qRw and post that here? Is it the same path as above or different?

I was encountering the same issue.
Here's what I was able to track down:
When you update a node, redirect_path_update() is called to generate the redirect.

This function will attempt to call RedirectRepository->findMatchingRedirect() to check for an existing redirect first.
However, leading slashes are removed from the source path when a new redirect is created.
Example:

public function setSource($path, array $query = array()) {
  $this->redirect_source->set(0, ['path' => ltrim($path, '/'), 'query' => $query]);
}

This was NOT being done when calling findMatchingRedirect(), which was causing the hashes to not match and thus allowing a some cases to slip through and throw the duplicate entry errors.

I'm attaching a patch with a fix.

Patch works perfectly fine.

Steps to reproduce (for my case)
* Taxonomy terms are created through drush command (through external API)
* When the title/name for the term was changed, it was creating redirects
* When the same changed for second time, it started giving error
* Additional note: it may be related to multi-lingual system too

Changing to needs review for patch to be checked through automated tests.

Test passed, updating to RTBC

The existing tests passed, yes, but we need to have a test that exposes this bug or some future refactoring might re-introduce this.

I just ran into the issue while trying to import some redirects from a D7 site through drush migration. It's because a redirect already exists in the database with the same hash.

The header of the generateHash method says that a hash is created based off the source_path, source_query and language:

/**
   * Generates a unique hash for identification purposes.
   *
   * @param string $source_path
   *   Source path of the redirect.
   * @param array $source_query
   *   Source query as an array.
   * @param string $language
   *   Redirect language.
   *
   * @return string
   *   Base 64 hash.
   */

I checked in the database and there was already a record with the same source_path, source_query and language as the redirect my migration was trying to import. So it's probably a good thing this error is thrown in this case, because you may not want your existing queries overwritten.

Any patch and test would need to take that into account.

The patch in #9 doesn't fix it for me. I still get the following message:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'BSyomGAjF_qtCPwYv-eQ0bATX2_7Sxet7BsrITTz7y0' for key 'hash'

@achap possible to share the steps/scenario for your case?

@nikunjkotecha

Sure. I am running a migration that imports redirects from a D7 export csv to D8 redirect table. 98% of them import fine but my D8 site already has some existing redirects in it before the migration occurred. The hash is generated from source_path, source_query and language, so if any of my redirect imports have the same source_path, source_query and language as a redirect had when it was originally created via the UI the integrity constraint violation will occur.

Interestingly, some of the existing redirects have the same hash as the migrated redirects even though the source_path is different in the D8 database, which suggests that the D8 redirects had their source paths updated after the hash was created and a new hash was not created.

Patch works for me, thanks!

This patch (#9) is not working for me. Anybody please help.

So I actually experienced the issue in the same way as the other posters and the patch in #9 works fine. Thanks for that. I added a simple Kernel Test that will expose this bug.

I think my issue in #14 might be because of my own migration code. In any case I don't think it should block this patch.

Just to be sure the test exposes the bug let's run only the test against the current code base

Perfect.

I think #21 can be commited

Thanks. Leading / or not is unfortunately a mess, I think this change makes sense and ensures it is consistent.

Minor comments on the test, then this can be committed.

1. +++ b/tests/src/Kernel/RedirectAPITest.php
   @@ -148,6 +148,64 @@ class RedirectAPITest extends KernelTestBase {
   +  /**
   +   * @see https://www.drupal.org/project/redirect/issues/2887866
   +   */
   +  public function testDuplicateRedirectEntry() {
   

   This is not a valid docblock description, should be a sentence describing what it does.

   Referencing the d.o issue is not needed, that will be visible through the commit message/git blame.

2. +++ b/tests/src/Kernel/RedirectAPITest.php
   @@ -148,6 +148,64 @@ class RedirectAPITest extends KernelTestBase {
   +    $this->assertNotNull(
   +      $redirect_repository->findMatchingRedirect(
   +        $original_existing_redirect_path['alias'],
   +        array(),
   +        $original_existing_redirect_path['langcode']
   +      )
   +    );
   

   I'd prefer to split this to two lines, one that calls fingMatchingRedirect() on a single line and stores it into a variable and then the assert.

Here you go. How's that?

That was fast!

I did a bit of cleanup, new array syntax and simplified the test code quite a bit. I assume the large arrays were copied from what hook_path_update() receives which I guess made sense to you but I actually found quite confusing to understand.

If I just hardcode the data then the is much shorter and IMHO easier to understand, any objections?

PS: don't forget to provide an interdiff, which makes it easier for reviewers to see what changed. Trivial with git, either commit the original patch and diff against different commits or just apply to head (e.g. with git apply -3) then it is the git diff HEAD is the full patch and git diff is the changes/interdiff.

Yes I copied from hook_update_path. That looks much better now, no objections from me!

Great, committed. Will do a new release soon.

Sorry for re-opening this issue. I was having this problem again with redirect 1.4

I realized that the redirect_path_update() hook wasn't deleting the redirect in database with the hash the new redirect will have. I don't know if this is the correct solution, i'm a little lost with the logic. I don't know why the redirect in database has the hash generated from the "node/xxx" string instead of the path alias string... but this was happening to me.

I can provide a dump of the $path variable at line 106:

$path = 
Array
(
    [source] => /node/19251
    [alias] => /apoyo/apoyo-empresarial/empresas-participantes/samsung-3
    [langcode] => es
    [pid] => 1
    [original] => Array
        (
            [source] => /node/19251
            [alias] => /apoyo/apoyo-empresarial/empresas-participantes/samsung-2
            [langcode] => es
        )

)

An evaluation of "Redirect::generateHash("node/19251",[],"es")":
uiw673iLnkADyupNDaMwl7O2Rd08ej_dqkD3lTDCSp4

And the row at the database:
hash: uiw673iLnkADyupNDaMwl7O2Rd08ej_dqkD3lTDCSp4
redirect_source__path: apoyo/apoyo-empresarial/empresas-participantes/samsung-2
redirect_redirect__uri: internal:/node/19251

I also provide a patch to manually generate the hash from $path['source'] and deleting by hash from the database

Hope that helps, and that someone could get to a better solution or at least an understanding of the problem.

Experienced a similar issue with updating/inserting redirects in Drupal 7.
Attached patch prevents this from happening in following use case :
- Pathauto is configured to only allow 1 alias (rest is diverted to be managed in redirects)
- Historical redirects (originating from a batch URL alias to redirect migration) are reused via node/edit UI or remote services layer
- these redirects are by implementation first disabled within redirects (update status to 0)
- this update fails because of clashing hash primary key existance

It might help someone experiencing the same.

Here's an updated version of #34