This repository was archived by the owner on May 29, 2026. It is now read-only.
Fix: schema-driven type coercion on magic-table read paths#1367
Merged
Conversation
OpenRegister's magic-table read paths returned object-property values
whose runtime PHP types did not match the JSON-Schema-declared property
type. Two specific defects in MagicStatisticsHandler::convertRowToObjectEntity:
- The unconditional `is_string($value) && isJsonString($value)` decode
block at lines ~593–600 ran on every property regardless of schema
type. Once the preceding cast normalised a numeric DB value to
string ("123"), this block decoded it right back to int 123. Same
block silently turned literal strings "true" into bool, "null" into
PHP null, and stripped quotes from escaped strings.
- Only `string` was coerced. `boolean`, `integer`, `number`, `array`,
and `object` properties returned whatever the driver produced — which
on MariaDB means TINYINT(1) booleans came back as int 0/1.
The search-side handler already had a complete schema-driven converter
(`MagicSearchHandler::convertValueByType`). Extracted that into a new
shared `lib/Service/Object/SchemaTypeConverter.php` and routed both
read paths through it:
- New SchemaTypeConverter — stateless service, single public
`convertValue(value, schemaType)`. Mirrors the proven search-side
semantics for all six JSON-Schema primitive types. The string branch
retains the historical compatibility window (decode strings starting
with `[` or `{` that parse) so schemas with mismatched type
declarations continue to work; scalar JSON literals are NOT decoded.
- MagicStatisticsHandler — replaces the partial coercion + over-eager
decode with a single converter call. DateTimeNormalizer format
handling stays where it is (per design D5). `isJsonString` helper
removed (no remaining call sites).
- MagicSearchHandler — replaces the inline `convertValueByType` call
with the converter. Six private converter methods deleted.
- MagicMapper handler instantiation now passes the converter via the
app container (auto-wired, zero deps).
Endpoint coverage matrix (every read returning an ObjectEntity now
flows through the unified converter):
GET /api/objects/<uuid> MagicStatisticsHandler ✓
GET /api/objects (list) MagicStatisticsHandler ✓
GET /api/search MagicSearchHandler ✓
POST /api/objects re-fetch via Statistics handler ✓
PUT /api/objects/<uuid> re-fetch via Statistics handler ✓
Boolean coercion intentionally rejects the HTML form literal `'on'` —
form normalisation belongs in the controller layer (design D7).
Inline private methods on MagicSearchHandler are deleted, not kept as
deprecation shims, since they had no external consumers (design D8).
Tests:
- New SchemaTypeConverterTest — 55 cases across 8 test methods covering
every schema type plus the regression suite (numeric-shaped string,
"true"/"null" literals, quoted-string JSON, MariaDB int 0/1 booleans,
array-shaped string compat, null preservation, unknown-type fallback).
- Existing MagicStatisticsHandlerTest and MagicSearchHandlerTest
constructors updated for the new dependency. Date-format ordering
regression in `testValidDateTimePropertyRendersAsIso8601` confirms
the converter→DateTimeNormalizer ordering still holds.
Quality: 69/69 unit tests pass, 49/49 magic-mapper tests pass. PHPCS,
PHPMD, and `php -l` clean on touched lib files.
Spec: openspec/changes/fix-magic-table-type-coercion/ — proposal,
design (8 decisions), specs/schema-driven-read-coercion/ (9
requirements), tasks.md, plan.json. Long-lived spec stub in
openspec/specs/schema-driven-read-coercion/.
Refs: #1366
41 tasks
WilcoLouwerse
approved these changes
Apr 30, 2026
WilcoLouwerse
left a comment
Contributor
There was a problem hiding this comment.
No blockers. Three concerns worth addressing: convertArrayOrObject silently returns PHP null for the string 'null', handler-level delegation tests added nothing to enforce the single-source-of-truth guarantee, and dead code in convertString. Also a PHPCS spacing regression in the test helper signatures that will fail composer check:strict.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 147/147 | |||
| npm | ✅ | ✅ 598/598 | |||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-04-30 14:48 UTC
Download the full PDF report from the workflow artifacts.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 the read-side type drift where API responses for OpenRegister and OpenConnector returned object-property values whose runtime PHP type didn't match the JSON-Schema-declared property type:
boolean-typed property came back asint 0/1(MariaDB TINYINT(1) → mysqlnd).string-typed property whose value looked like a JSON literal ("123","true","null",'"foo"') was silently re-decoded back into the original primitive — string-cast undone by an over-eageris_string && isJsonStringdecode block inMagicStatisticsHandler::convertRowToObjectEntity.Both magic-table read paths now delegate to a single shared converter.
Closes #1366.
Endpoint coverage
Every endpoint that reconstructs an
ObjectEntityfrom a magic-table row goes through the unified converter:ObjectEntityviaGET /api/objects/<uuid>MagicStatisticsHandler::convertRowToObjectEntityGET /api/objects(list)MagicStatisticsHandler::convertRowToObjectEntityGET /api/searchMagicSearchHandler::convertRowToObjectEntityPOST /api/objectsMagicStatisticsHandlerPUT /api/objects/<uuid>MagicStatisticsHandlerPOST/PUT responses build their bodies by re-fetching through
findInRegisterSchemaTable(see comment inMagicMapper::insertObjectEntity: "CRITICAL FIX: Re-fetch the inserted object from database to get complete metadata"). That re-fetch goes through the same converter, so create/update responses are now type-consistent with reads.What changed
lib/Service/Object/SchemaTypeConverter.php— stateless service with a single publicconvertValue(value, schemaType). Zero deps, auto-wired.lib/Db/MagicMapper/MagicStatisticsHandler.php— replaces the partial coercion + over-eager JSON-decode block with one converter call.DateTimeNormalizerformat handling stays where it is (per design D5). UnusedisJsonStringhelper removed.lib/Db/MagicMapper/MagicSearchHandler.php— replaces the inlineconvertValueByTypecall with a delegated converter call. Six private converter methods deleted (single source of truth from day one — per design D8).lib/Db/MagicMapper.php— handler instantiation updated to inject the converter via the app container.tests/Unit/Service/Object/SchemaTypeConverterTest.php— 55 cases across 8 test methods covering every JSON-Schema primitive type and the regression suite.tests/Unit/Db/MagicStatisticsHandlerTest.phpandtests/Unit/Db/MagicSearchHandlerTest.php— constructors updated for the new dependency.Out of scope (deferred to follow-up changes)
SaveObject/SaveObjects/MagicBulkHandler.minimum/maximumon JSON Schema integer properties (require-integer-bounds).Spec / design artifacts
Full OpenSpec change under
openspec/changes/fix-magic-table-type-coercion/:proposal.md— Why + What + Capabilities + Endpoint coverage matrixdesign.md— 8 decisions (incl. D7: reject'on', D8: delete inline methods immediately) + risks + migration planspecs/schema-driven-read-coercion/spec.md— 9 ADDED requirements with WHEN/THEN scenariostasks.md— implementation checklist (33/42 done before opening this PR; remaining are manual verification + release notes + archive)plan.json— tracking issue [OpenSpec] [openregister] fix-magic-table-type-coercion #1366Long-lived spec stub:
openspec/specs/schema-driven-read-coercion/spec.md.Test plan
php -lclean on all touched files.lib/files (sniff scope perphpcs.xml).lib/files (pre-existing complexity warnings onapplyObjectFilters/buildObjectFilterConditionsSqlare out of scope — methods I didn't touch).SchemaTypeConverterTest— 55/55 unit tests pass.MagicStatisticsHandlerTest+MagicSearchHandlerTest— 14/14 pass with the new converter wired in.tests/Unit/Service/MagicMapperTest.phpthat fails withConditionMatcherconstructor mismatch unrelated to this PR) — 49/49 pass.0/1in a boolean field, JSON-literal strings); callGET /api/objects/<uuid>,GET /api/objects,POST /api/objects,PUT /api/objects/<uuid>,GET /api/search?q=…and assert types in the JSON response.true/false.Heads-up for OpenConnector reviewers
Sync flows that previously received
0/1for booleans will now receive nativebool. Mapping templates that explicitly checked=== 1would break — Conduction-internal mappings have been audited (zero hits).Pre-existing issues NOT fixed by this PR
tests/Unit/Service/MagicMapperTest.php:224— constructor missing$conditionMatcherarg, all 29 tests in that file fail. Stack trace lands at line 365 ofMagicMapper.php(MagicRbacHandlerinstantiation), which I did not modify. Last touched by commit44894ed42. Worth a small follow-up fix.