Missing primary key in table `honeypot_user`

I have created an MR , please merge. Thanks

If the database structure is going to change, it must be done in the current 2.1.x branch first, and only then backported to D7. Doing it the opposite way will prevent upgrades and will risk having this fix disappear when D7 goes out of support.

Also, any database schema changes must have update tests. We now have a working example of such a test (see 📌 Drupal Core (8.8.4) Update Forces Honeypot to Recall Hook_Update_N Fixed ) which will serve as an example of how to write a test like this.

Drupal 10.1.0-alpha1 status report now reports this issue, which will make it a lot more visible. Also it has performance implications.

Transaction isolation level
READ-COMMITTED
For this to work correctly, all tables must have a primary key. The following table(s) do not have a primary key: honeypot_user. See the setting MySQL transaction isolation level page for more information.

So I'll change this to a task and gain priority.

Steps to be taken are described in #23, so let's see what can be done soon!

Just waiting for a patch here - I'm not personally working on this one.

Honeypot only does one query, which looks like:

      $query = $this->connection->select('honeypot_user', 'hu')
        ->condition('uid', $uid)
        ->condition('timestamp', $this->timeService->getRequestTime() - $expire_time, '>');

(See the code in HoneypotService.php).

I've never been fond of adding a DB column just to act as a key and nothing else. If we add an ID column, we will never be querying on ID and it will just serve to take up space in the DB for no reason other than to have a declared primary key.

The above selection determines a unique record through the combination of uid and timestamp. If this combination is declared as a primary key, then we shouldn't need a dedicated column to hold an arbitrary sequential number.

What we still need is a patch for 2.1.x that contains:

1. A new schema in honeypot_schema() that declares a primary key. This will be used for new installations of this module.
2. A hook_update_8102() to modify the schema when updating existing installations of this module.
3. A new update test method - testHookUpdate8102() - added to the existing tests/src/Functional/Update/HoneypotUpdateTest.php. This new test method will necessarily be very similar to testHookUpdate8101() so it should be easy to write even if you don't have much experience with writing test cases.

Done, please review!

I hope simply checking whether a primary key mearly exists before and after the update, will suffice for the update hook test, as I couldn't quickly find another method explicitly checking the primary keys names after update.

Looks good. My preference would be to have two separate test methods for the two separate update hooks, rather than combine them into one test method.

The test failure is a real failure caused by this patch, and needs to be fixed before we can proceed:
Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '0-1684161684' for key 'PRIMARY': INSERT INTO "test80290015honeypot_user" ("uid", "hostname", "timestamp") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => 0 [:db_insert_placeholder_1] => 127.0.0.1 [:db_insert_placeholder_2] => 1684161684 ) in Drupal\mysql\Driver\Database\mysql\ExceptionHandler->handleExecutionException() (line 43 of core/modules/mysql/src/Driver/Database/mysql/ExceptionHandler.php).
See https://dispatcher.drupalci.org/job/drupal8_contrib_patches/156879/artif...

So the problem comes from this code in the module:

    $this->connection->insert('honeypot_user')
      ->fields([
        'uid' => $uid,
        'hostname' => $this->requestStack->getCurrentRequest()->getClientIp(),
        'timestamp' => $this->timeService->getRequestTime(),
      ])
      ->execute();

This logs failed form submissions, and the biggest problem here is for anonymous users, who will all have uid=0. So I guess the set {uid, timestamp} is not sufficiently unique to use as a primary key. I guess we have to add that new column just for the serial key after all.

@TR, thanks for the feedback! I'll be right on it.

Alright, everything should be adjusted now! Please review!

Thx @Grevil! LGTM!

I triggered tests for pgsql, mariadb and sqlite to see if it causes trouble there.

Just one point:
'description' => 'Unique record ID.',
Should we explain why this record was created? In the description or a comment above the schema change, eventually linking here?
As it's never used as reference, it should perhaps be explained?

@TR should decide.

Conflicts with this commit 📌 Drupal Core (8.8.4) Update Forces Honeypot to Recall Hook_Update_N Fixed

@SocialNicheGuru: Why do you say that? What conflicts?

Update 8101 adds user and hostname on the dev version with the patch above that was committed.

When I added this patch, it failed because update 8102 also tries to add the user to hostname schema.
I kept getting the error that the user already exists.

The update never completed.

Try the patch on the honeypot dev version to see if you get the same.

The current patch under consideration is the Merge Request 9 from comment #33, on the 2.1.x branch of Honeypot.

Update 8101 adds user and hostname on the dev version with the patch above that was committed.

8101 adds the hostname column to the honeypot_user table, but only if the hostname column is missing.

When I added this patch, it failed because update 8102 also tries to add the user to hostname schema.

8102 adds an id column to the honeypot_user table and a primary key to the honeypot_user table. It doesn't touch the hostname column.

I don't see a problem here. As you can see from the tests in #33, the current MR does apply to the HEAD of the branch (2.1.x dev), and there are no errors running the tests. The tests include a new test of the update from 8101 to 8102, so that update test should have failed if there were a conflict.

The latest patch from MR9 works fine against the latest Dev release of Honeypot, and with D10.1-rc1

wfm too

I upgraded to Drupal 10.1.0 and error is showing about missing primary key in the table 'honyeput_user'.

I'm using Honyepot version 2.1.2.

Please fix.

@redseujac: with all respect, that's exactly what the community members are doing here. If you want to help this to land, you can contribute by testing the patch.

Cheers,
Markus

(or paying someone experienced, if you're not a developer). Thanks :)

@masipila @Anybody: well, the issue is solved for me. I just uninstalled and removed the module. Thank you all :)

@redseujac, you could also simply apply this MR's diff: https://git.drupalcode.org/project/honeypot/-/merge_requests/9.diff, as a patch inside your composer.json and the issue is fixed.

@Grevil

I applied this patch to -dev version earlier and looks like it worked just fine. However, can't be 100% sure, as Drupal 10.1 breaks some things in Bootstrap theme that I use.

^ update

Rechecked patch from #33 and mentioned at #48 with Drupal 10.1. Definitely works.
Thanks.

Tested patch from #33 on D 10.1 and works perfectly.

Thanks for your effort and time.

Grevil commented in #48:

@redseujac, you could also simply apply this MR's diff: https://git.drupalcode.org/project/honeypot/-/merge_requests/9.diff, as a patch inside your composer.json and the issue is fixed.

I have followed your advice and the issue is fixed indeed. Thanks a lot!

Composer gives me the error message that the patch cannot be applied. I am using Honeypot Version 2.1.2 and Drupal Version 10.1.

I don't know if there is another problem or if i am just too stupid ....

Could someone please append the patch to this issue? Thanks! :)

@gmarcel patch is always against dev. Core version doesn't matter.

Try with honeypot 2.1.x-dev until it's merged.
Still I think the maintainer will merge it soon as of the feedback.

After upgrading to Drupal Version 10.1.0 I get the following error:

Transaction isolation level READ-COMMITTED
For this to work correctly, all tables must have a primary key. The following table(s) do not have a primary key: honeypot_user. See the setting MySQL transaction isolation level page for more information.

My configuration:
php 8.27
MariaDB 10.11.4
nginx 1.24.0
Ubuntu 20.04.6 LTS

I could get rid of this error by adding a primary key to the table honeypot_usr in the related DB but I am not sure whether this is a good solution. However it should become fixed as not every user can/would like to modify via sql-commands its DB:

I changed some of the comments in the code. Interdiff attached. I will be merging MR 9 after tests complete.

Merged. Thanks to all who worked on this.

@TR
New -dev release works well for me. Thanks.

Can we please have a tagged release for this?

I second asking for a tagged release. We are unable to run the dev version due to security policies so are just waiting for an update to be made to make this stop acting up. I would also settle for a patch on the current released version so we can put this issue to rest until a new release is made.

+1 for a tagged release. Thanks!

I do agree. Please release a tagged release.

For all those waiting for the merge, here's a patch. I made it based on the differences between 2.1.2 release and dev branch and included the updates laid out above.

Hello! I am experiencing the same problem.

Drupal Version: 10.1.1
Web Server: LiteSpeed
PHP Version:8.1.18
Database Version: 10.6.12-MariaDB-cll-lve
System:MariaDB

I am not a developer, but I would like to apply the patch. I have installed all the modules using composer. Should I add it in the composer.json file of the module or in the general composer.json file?

Thank you in advance!

Constant

@constantsearcher

Yes you can apply the patch directly to your composer.json file. Here's an article that outlines how to add a patch to composer. https://vazcell.com/blog/how-apply-patch-drupal-9-composer

Will version 2.1.3 fix this issue for those of us who have difficulty applying manual patches?

Tagged release please? The last honeypot release was Oct 2022.

I reached out to TR → via Slack with a kind request for 2.1.3.

You know what? An issue like this that has been open for more than 5 years does not scream "emergency, new release needed" to me. That just seems very selfish. As in, you got the fix that is important to you, so who cares about all the other issues? You could have helped out anytime over the past five years and this issue would have been fixed long ago. Kudos to @Grevil who actually took the time to do the work.

(And by "you" I mean the people asking ME to do additional work even though THEY haven't contributed anything. Some of you HAVE contributed, and I appreciate that so don't take this personally, but for the most part I seem to be the only one contributing to this module over the past two years ...)

What *I* would like is for some people to help out here in the issue queue. I don't think it's too much to ask that, if you are dependent on this module, you at least try out some of the proposed patches and give some feedback.

The current -dev release only has three commits, including this one. No one - not a single person - has tested the first of the three commits (see 📌 Drupal Core (8.8.4) Update Forces Honeypot to Recall Hook_Update_N Fixed ). I wrote the patch for that and committed it after two years, but only after writing tests and doing my own testing. The tests I wrote were critical for solving this current issue as well. Still, not a single community member has tested that previous patch. And because it involves an update function, just like this issue, it potentially breaks 70,000 sites that use this module. So if I make a new release, everyone using this module will be subject to several update functions. I don't currently have time to deal with that.

So yes, I will eventually make a new release, when I am ready and have time to deal with the inevitable problems created by that new release. But YOUR emergency is not my problem. You can use the -dev release if you want, you can add the patches to your composer.json if you want, but a new release is NOT blocking your progress. It's an inconvenience to you at best. A new release doesn't help anyone but the few here - there are still other issues that are important to other people that still need to be addressed.

Is it really too much to ask that people contribute to this module BEFORE issues like this becomes a problem? I'm doing my part by volunteering to maintain this module even though I don't personally use it. If you (whoever you are) are getting paid to develop or maintain a site that uses this module, the very least you can do it devote a small amount of your time to commenting on open issues and/or reviewing pending patches. If even 1/10 of 1% of the people who use this module did this, then this would have been solved long ago.

With all my respect for the maintainers, but it seems to me that's not much honey left in the pot...

What's the worth of a module without users or when users are not allowed to report issues and humbly beg to fix them?

Not every user is supposed to be a developer, right? and not every user is supposed to be or become a maintainer? Is it the users' fault that a problem has not been solved after 5 years? I surely don't think so.

That being said, I have disabled and uninstalled again the empty honeypot.

@TR

First off, we should all be thankful to you and the other maintainers who have worked to maintain this module over the past ~12 years. Personally, this is the most effective option I’ve found for combatting spam on contact forms without adding tons of frustration for users (like most of the captcha options).

I think this issue in particular went unnoticed by most for so long since Honeypot generally worked fine. The Drupal 10.1 update started flagging this an issue on the status report page, so naturally this is leading people to become more aware of the problem and seek out a fix (myself included).

I totally understand where you’re coming from. Users often have unreasonable expectations for open source projects, and expect/demand prompt support without offering much in return. IMHO, it doesn’t help that Drupal has lost some of the momentum it had in the 7.x days due to the poor transition to 8.x+ which likely has led to some to diverting their focus elsewhere.

With that said, I think there are many who would be willing to support a handful of project such as this that their sites rely on, but don’t have the development skills or time to make meaningful contributions. Instead, they may be willing to offer some sort of monetary support, either one-time or on recurring basis. I don’t know where this falls in the spirit of the Drupal project or organization, but compared to something like Wordpress which has countless paid/or subscription type modules I don’t think it’s too crazy of an idea.

I’m currently running the dev branch on the latest 10.1.1 release of Drupal, and everything is working great. Sure, it would be nice to have a tagged release at some point, but most should be able to use 2.1.x-dev for now or add the patch from @catapipper in post #64 to get things updated to avoid the error flag in Drupal.

Anyways, thanks again for your work here. :)

jabeler commented in #72:

With that said, I think there are many who would be willing to support a handful of projects such as this that their sites rely on, but don’t have the development skills or time to make meaningful contributions. Instead, they may be willing to offer some sort of monetary support, either one-time or on recurring basis. I don’t know where this falls in the spirit of the Drupal project or organization, but compared to something like Wordpress which has countless paid/or subscription type modules I don’t think it’s too crazy of an idea.

see Webform module's Open Collective: https://opencollective.com/webform#section-contributors

I would say it's a good idea for maintainers to add funding URLs to at least composer.json, as well as the admin user interface so non-developers can find it. But organizing fundraising and disbursement requires some work too, so in the short term probably more helpful for non-developers to simply ask your developer (or hire a developer if you don't have one) to test and review patches in the issue queue, which circles back to what @TR was saying would be helpful.

mfb commented in #74

(or hire a developer if you don't have one)

I'm afraid that's an illusion: a common user will not be eager to hire a developer to test module patches or so.

As someone who has hired developers to work on open source, I can say it's a thing that happens. Generally not enough though, sigh

Automatically closed - issue fixed for 2 weeks with no activity.

I've only just found out about this issue via Drupal's status report while upgrading from 9.3.9 to 10.1.1 so I have applied the the patch from #64. Everything appears to be in order: updates were run, the column has been added and the module functions fine.
Thanks for your work!

Just wanted to confirm that I have been using dev version for a while now and all looks good :)

The patch in #64 works as hoped, can we get this released?

I created a 'Plan' issue to create a new release. That issue is referencing this issue.

Thanks for everyone's work on this!

I have not really used drupal since the early 7.x years, so there are a lot of new features, and this whole patching thing is one. So this is going to be a very, very stupid question to most... if not all.

I'm running drupal 10.1.4, and really just would like to know how do we actually apply the patch?

I've googled "apply patch drupal" but there are no concise instructions that I have found.

I really appreciate the response.

Thank you in advance,

Jason

Version 2.1.3 of honeypot fixed the problem. Thanks!

Thanks for the release.

Many thanks indeed, highly appreciated!

Ambient.Impact → closed merge request !18

So I ran into an unexpected problem with this update: if you already had the primary key, the update would just fail, which is bad if you'd already been using the existing patch in production. I added a check that the key doesn't already exist and only attempts to add it if it doesn't. Also added a test, though this is the first update hook test I've done so feel free to edit as needed.

For anyone coming here after upgrading to the latest version of the module, and seeing this is fixed, I had to uninstall and re-install the module to get the message to go away (and then re-import the config).

Not sure if there was a technical issue with getting a hook_update to work?

Same issue here (core version 9.5.10 PHP 8.1 MySQL 8)
hook update did not end and we got this MySQL error :

[error] SQLSTATE[HY000]: General error: 4111 Please drop primary key column to be able to drop generated invisible primary key.: ALTER TABLE "honeypot_user" ADD "id" INT NOT NULL auto_increment COMMENT 'Unique record ID.', DROP PRIMARY KEY, ADD PRIMARY KEY ("id");

This is a patch for the actual dev version of honeypot module, taking into account the issue mentioned just above, when primary key exists and cannot be deleted

And that is a patch for 2.1.3 stable release

A cleaner version of the patch in #92

@federiko_ You must have MySQL 8 config sql_generate_invisible_primary_key (GIPK) enabled? It defaults to off.

In that case, are you sure dropping the primary key works? The GIPK documentation implies you cannot drop a primary key the way you are trying to do.

My take on this is that drupal core doesn't yet support GIPK being enabled. It would need some code in the addColumn() method to drop the invisible column in addition to the primary key if GIPK is enabled.

Yes my bad @mfb I published those patches too quickly, I'm sorry! It worked in my dev environnment but when deploying my code to real world (MySQL 8 with sql_generate_invisible_primary_key enabled) above patches (#92 and #93) didn't work.

[error]  SQLSTATE[42000]: Syntax error or access violation: 1235 This version of MySQL doesn't yet support 'existing primary key drop without adding a new primary key. In @@sql_generate_invisible_primary_key=ON mode table should have a primary key. Please add a new primary key to be able to drop existing primary key.': ALTER TABLE "honeypot_user" DROP PRIMARY KEY; Array
> (
> )
>  
>  [error]  Update failed: honeypot_update_8102 

I created 📌 Support MySQL GIPK mode Needs review for working on GIPK support.