[2.x] fix: notification dedup query never matches non-null data on MySQL - #4645
Merged
Conversation
Adds three integration tests for the model scope and the user-visible
syncer behaviour:
1. matchingBlueprint() finds existing rows when blueprint data is null
(sanity check — the path that does work today)
2. matchingBlueprint() finds existing rows when blueprint data is non-null
— currently fails on MySQL because the scope compares the JSON column
directly to the json_encode'd string, but MySQL canonicalises stored
JSON ({"replyNumber": 58}) and the encoded input is compact
({"replyNumber":58}), so string equality never matches
3. NotificationSyncer::sync() called repeatedly with the same blueprint
does not create duplicate rows — the user-visible symptom on every
edit of a post that triggered postMentioned, newPost, postMoved, etc.
See #4643. Pushing the tests
without the fix so the CI matrix demonstrates which DB engines hit the
bug.
The scope used `->where('data', $data)` which on MySQL compares a JSON
column directly to a string. MySQL stores JSON canonicalised — `{"a": 1}`
even when the input was `{"a":1}`, which `json_encode()` produces — so
string equality never matches and every NotificationSyncer::sync() call
on a blueprint with non-null data treated the recipient as new and
re-emailed them.
Branch by driver:
- PostgreSQL: cast column to text (`data::text = ?`) — unchanged
- MySQL/MariaDB: cast parameter to JSON (`data = CAST(? AS JSON)`) so
the comparison round-trips through MySQL's canonical form
- SQLite: stores JSON as opaque text, plain `=` works as before
Pull the null-data path out front since `= NULL` doesn't match anything
on any engine and there's no driver-specific handling needed.
Fixes #4643.
MariaDB rejects CAST(... AS JSON) syntax — its JSON column is just an alias for LONGTEXT and there's no SQL-standard JSON cast. The previous fix incorrectly applied that cast to MariaDB too, breaking the matrix on MariaDB and MariaDB-prefix. Branch by driver explicitly: - PostgreSQL: data::text = ? (cast column to text) - MySQL: data = CAST(? AS JSON) (cast parameter to JSON) - MariaDB / SQLite: plain string equality (no canonicalisation)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4643. `Notification::scopeMatchingBlueprint()` on MySQL never matches existing rows when the blueprint's `data` is non-null, silently disabling dedup for every notification type whose blueprint returns non-null `getData()` — `postMentioned`, `newPost`, `postMoved`, plus extension types like `byobuPrivateDiscussionReplied`. Every edit of a post that triggered such a notification creates a new row and re-sends the email.
Root cause
Before:
```php
return $query->where($attributes)
->whenPgSql(function ($query) use ($data) {
return $query->whereRaw('data::text = ?', [$data]);
}, function ($query) use ($data) {
return $query->where('data', $data);
});
```
The PostgreSQL branch correctly casts the column to text. The else branch (MySQL / MariaDB / SQLite) compares the JSON column directly to a string. `getBlueprintAttributes()` produces `$data` via `json_encode($data)` — compact form like `{"replyNumber":58}`. MySQL stores its JSON column canonicalised — with a space after the colon: `{"replyNumber": 58}`. Direct string comparison never matches.
Fix
Branch by driver explicitly:
The null-data path is pulled out front since `= NULL` doesn't match anything on any engine.
Tests
Three integration tests in `framework/core/tests/integration/notification/MatchingBlueprintTest.php`:
The first commit added these tests without the fix, so the CI matrix demonstrated which engines were affected (MySQL fail; PostgreSQL/MariaDB/SQLite pass — confirming the diagnosis). The second commit applied the fix; full matrix now green.
Scale
From the issue reporter's production instance:
```
type dup_groups total_dup_rows
postMentioned 665 1,477
newPost 2 4
```
`postMentioned` is the dominant offender precisely because it has non-null data and the dedup query was broken for it.
Relationship to #4622
#4622 describes a queue-timing race in `NotificationSyncer::sync()` for null-data blueprints (where the dedup query does work, so duplicates only arise from a narrow window). For non-null-data blueprints, this bug means duplicates were guaranteed on every edit regardless of timing — much higher impact, much simpler fix. Both are needed; this is the one users hit most often.