fix(mappers): don't compare the bigint id column against non-numeric strings in find() - #756
fix(mappers): don't compare the bigint id column against non-numeric strings in find()#756rubenvdlinde wants to merge 2 commits into
Conversation
…c strings in find()
`SourceMapper::find()` (and the matching MappingMapper / RuleMapper / JobMapper /
EndpointMapper) build a query that ORs `id = :param` into the uuid/slug lookup even
when the input is a non-numeric string (a uuid or slug). On Postgres that makes the
whole statement fail at plan time:
SQLSTATE[22P02]: invalid input syntax for type bigint: "xwiki"
so `find('<slug>')` is unusable on Postgres — you can only look an entity up by
numeric id. (MySQL silently casts the string to 0 and limps along.) The `id`
branch can never match a non-numeric string anyway, so drop it from the
non-numeric path — bringing these mappers in line with `SynchronizationMapper::find()`,
which already only searches uuid/slug for string input.
Surfaced by OpenRegister's pluggable integration registry (ConductionNL/openregister#1307):
external integration providers resolve their OpenConnector source by slug via
`SourceMapper::find($slug)` (`ExternalIntegrationRouter::loadSource()`), which 503s
on every Postgres install today.
Fixes #755
Quality Report — ConductionNL/openconnector @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ❌ | ||||
| eslint | ❌ | ||||
| stylelint | ❌ | ||||
| composer | ✅ | ✅ 148/148 | |||
| npm | ❌ | ❌ 1/573 denied | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
❌ Denied npm licenses
| Package | Version | License |
|---|---|---|
| @fortawesome/free-solid-svg-icons | 6.7.2 | (CC-BY-4.0 AND MIT) |
Quality workflow — 2026-05-12 13:00 UTC
Download the full PDF report from the workflow artifacts.
| @@ -47,12 +47,13 @@ public function find(int|string $id): Endpoint | |||
|
|
|||
| // If it's a string but can be converted to a numeric value without data loss, use as ID | |||
| if (is_string($id) && ctype_digit($id) === false) { | |||
There was a problem hiding this comment.
[CONCERN] Negative-integer strings and floats silently return not-found
The guard ctype_digit($id) === false routes strings like "-1", "1.5", or " 123" into the uuid/slug-only branch. Those values can never match a uuid or slug either, so the caller gets a DoesNotExistException rather than a Postgres error — an acceptable silent failure — but it may mask bugs in callers that accidentally pass a stringified negative ID. This applies identically to all five patched mappers. Consider using is_numeric($id) instead of ctype_digit($id) to also handle "-1" and "1.5" correctly, or add a comment noting this behaviour.
| @@ -47,12 +47,13 @@ public function find(int|string $id): Endpoint | |||
|
|
|||
| // If it's a string but can be converted to a numeric value without data loss, use as ID | |||
| if (is_string($id) && ctype_digit($id) === false) { | |||
There was a problem hiding this comment.
[CONCERN] ctype_digit('007') routes leading-zero strings to numeric branch — wrong match
ctype_digit('007') returns true, so the string '007' is forwarded to the numeric branch and cast to int 7 via PARAM_INT. If the intent is to match by uuid/slug for such values, this would be a mismatch. Also, ctype_digit('') returns false, so an empty string goes to the uuid/slug branch — safe after the fix but worth documenting. Consider is_numeric($id) && (int)$id > 0 as a tighter guard.
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Review
🟡 Concerns (2)
- Negative-integer strings and floats silently return not-found —
lib/Db/EndpointMapper.php:49 - ctype_digit('007') routes leading-zero strings to numeric branch — wrong match —
lib/Db/EndpointMapper.php:49
🟢 Minor (1)
- Comment consistency across all five patched mappers — verify all updated (
lib/Db/EndpointMapper.php:50)
Verify that the improved comment (explaining the bigint comparison error being avoided) is consistently applied across all five patched mappers: EndpointMapper, JobMapper, MappingMapper, RuleMapper, and SourceMapper. The diff view makes this hard to confirm at a glance.
Reviewed by WilcoLouwerse via automated batch review.
Quality Report — ConductionNL/openconnector @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 148/148 | |||
| npm | ✅ | ✅ 674/674 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-19 04:02 UTC
Download the full PDF report from the workflow artifacts.
|
Closing this PR because all 5 mapper files ( |
Closes #755.
Problem
SourceMapper::find(int|string $id)— and the identical pattern inMappingMapper,RuleMapper,JobMapper,EndpointMapper— builds a query that ORsid = :paraminto the uuid/slug lookup even when the input is a non-numeric string (a uuid or slug):On Postgres this makes the whole statement fail at plan time:
…so
find('<some-slug>')is unusable on a Postgres install — you can only look an entity up by numeric id. (MySQL silently casts the string to0and limps along, which is why it hasn't bitten before.)Fix
Drop the
idcomparison from the non-numeric branch in all five affected mappers — it can never match a non-numeric string anyway. This brings them in line withSynchronizationMapper::find(), which already only searchesuuid/slugfor string input. The numeric path (which casts toPARAM_INT) is untouched.Touched:
SourceMapper,MappingMapper,RuleMapper,JobMapper,EndpointMapper.Why now
OpenRegister's new pluggable integration registry (ConductionNL/openregister#1307, ADR-019) has external integration providers (e.g. the
xwiki"Articles" provider) that resolve their OpenConnector source by slug viaSourceMapper::find($slug)— seeOCA\OpenRegister\Service\Integration\ExternalIntegrationRouter::loadSource(). On a Postgres-backed Nextcloud that 503s withopenconnector-source-missingon every call, even when the source exists.Notes
tests/suite mocks services, not the DB layer), so this is verification-by-repro: on Postgres,\OC::$server->get(SourceMapper::class)->find('<existing-source-slug>')throws before this patch and resolves after.composer phpcs/psalmcouldn't run in my environment (composer.lock is incompatible with PHP 8.2 here —composer updatewould be needed, which is out of scope for this fix); the change is a one-line deletion per file matching the existingSynchronizationMapperstyle, so CI's quality jobs should pass clean.