Skip to content

Conversation

@RZivaralam
Copy link

To Reproduce

This confirms that the exception is triggered in valid and expected situations, for example when:

multiple users perform the same search query at nearly the same time, or

the same search term is executed repeatedly within the same second.

In these cases:

the UPDATE statement does find the row (correct sequence_id)

but writes identical values (keywords and second-based tstamp)

MySQL / MariaDB correctly returns affectedRows = 0

the current logic incorrectly interprets this as “row not found”

@dkd-kaehm dkd-kaehm changed the base branch from release-12.0.x to main December 10, 2025 16:14
@dkd-kaehm dkd-kaehm changed the title [BUG] #4499 [BUGFIX] Use the return value of the UPDATE statement correctly in the LastSearchesRepository::update method Dec 10, 2025
…e `LastSearchesRepository::update` method

executeStatement() returns:
* false → SQL execution failed (real error)
* 0 → UPDATE executed successfully, but no data changed
* 1+ → UPDATE executed and changed data

Fixes: TYPO3-Solr#4499
@dkd-kaehm
Copy link
Collaborator

Are you sure about that statement?:

grafik grafik

There is no boolean return value on TYPO3 12 and 13 LTS Stacks.

@RZivaralam
Copy link
Author

RZivaralam commented Dec 14, 2025

Let me explain again what I tried to change and why.

Yes, you are right: executeStatement() returns an integer, not a boolean. I assumed that executeStatement() might return something like false or -1 if an UPDATE did not work correctly,
so I suggested checking for a failure condition.
However, the real problem is not the return type itself, but how the value is interpreted.

The current code treats affectedRows < 1 as an error and throws an exception. This is wrong.

A return value of 0 does not mean that the sequence_id does not exist. It simply means that the UPDATE was executed successfully, but nothing changed. This is a valid and very common situation.

This happens especially when:

  • multiple users trigger searches at the same time
  • the same search term is executed concurrently
  • the data (e.g., timestamp or keywords) remains the same
image

In these cases, affectedRows will be 0, and an exception will be thrown, even though nothing is wrong.

I understand that the intention of this check is to ensure that the sequence_id exists in the table. But this check is redundant.

I manually changed the database (renamed or altered the sequence_id column) to verify this:
If the sequence_id does not exist or the column is invalid, TYPO3 already throws a database error. This is correct and sufficient.
l renamed sequence_id to sequence_id1

image image

Adding an extra PHP-level check for this causes:

  • redundant validation
  • false positives
  • race conditions when multiple users update at the same time

So the issue is not that the return value is an integer.
The issue is that 0 is treated as an error, although it is a valid result.

executeStatement() returning an integer already guarantees that the SQL was executed successfully.
Column existence and type safety are enforced by the database.A return value of 0 is a valid state and must not be treated as an error.

In my opinion, the affectedRows < 1 check should be removed or relaxed; otherwise, concurrent searches will continue to trigger unnecessary exceptions.

This code should be deleted:
if ($affectedRows < 1) {
throw new InvalidArgumentException(vsprintf('By trying to update last searches row with values "%s" nothing was updated, make sure the given "sequence_id" exists in database.', [json_encode($lastSearchesRow)]), 1502717923);
}

@RZivaralam
Copy link
Author

You can also reproduce this issue quite easily.

Try searching for the same term multiple times in quick succession
(e.g. pressing Enter repeatedly on the same search query).

With concurrent or very fast repeated searches, the UPDATE may legitimately
return 0 affected rows, and the current affectedRows < 1 check will then
throw an exception.

This demonstrates that the issue is not theoretical, but can be reproduced
in real usage scenarios with normal user behavior.

Thanks for taking the time to look into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants