Repository navigation
fix(python): accept only booleans for is_time, as the schema does - #557
Conversation
The schema types `Dimension.is_time` as `boolean`, but the field was a plain `Optional[bool]`, so pydantic's lax mode coerced "yes", "true", "True", "no", "off", "1", 1, 0, 1.0 and 0.0. The package accepted all ten, none of which the schema allows, and serialized them back as `true` or `false`. `StrictBool` narrows the field to the two values the schema permits. YAML input is unaffected, because PyYAML resolves `yes` and `on` to real booleans before the model sees them, and every `is_time` value in the repository is already a boolean literal or absent.
There was a problem hiding this comment.
🟡 Changes recommended
Explicit null remains accepted despite being invalid under the schema.
1 open finding
What changed in this PR
Aligns Python validation of is_time with the OSSIE boolean schema.
Changes:
- Uses
StrictBoolto prevent coercion. - Adds acceptance and rejection tests.
| File | Description |
|---|---|
python/src/ossie/models.py |
Enforces strict boolean validation. |
python/tests/test_models.py |
Tests strict is_time inputs and omission. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # `bool` would let pydantic's lax mode accept "yes", "1", 1 or 1.0 and store a | ||
| # real boolean, so input the schema forbids is silently rewritten into output it | ||
| # allows, and the mistake is never reported. | ||
| is_time: Optional[StrictBool] = None |
There was a problem hiding this comment.
Correct on the mechanics, and the laundering is the same shape: {"is_time": null} is schema-invalid, the model accepts it, and exclude_none=True drops the key so the output validates.
I'd rather not fix it in this PR, because it isn't specific to is_time. ossie-schema.json permits null for no property anywhere, and all 27 Optional[...] fields in the SDK accept an explicit null and serialize it away — OssieField.model_validate({..., "label": None, "description": None, "datatype": None}) succeeds today and emits a document with those keys simply absent.
The two problems differ in kind. Lax coercion is per-field and per-type, and StrictBool fixes it; this PR applies that to the only "type": "boolean" in the schema. Explicit-null acceptance follows from Optional[X] = None being how pydantic spells "optional", so it is uniform across the models by construction. Patching it on is_time alone would leave the other 26 unchanged and introduce a one-off idiom the next reader would reasonably question.
Doing it properly means deciding whether the SDK distinguishes an absent key from an explicit null across all 10 models — a behaviour change for any caller currently passing label=None. Nothing in this repository does (I checked every Ossie*(...) construction), but that is a decision worth its own PR and review rather than a rider on this one. I'll open it as a follow-up unless a reviewer would rather keep absent and null interchangeable.
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
LGTM! Switching OssieDimension.is_time to Optional[StrictBool] stops pydantic from coercing "yes", 1 and 1.0 into a real boolean 😄
That matches the strictness fixes in #551 and #552.
Two comments:
- models that currently set
is_timeto a string or number, such as"true"or1, will now fail validation. That's the point of the change, but I wanted to mention 😄 Optional[StrictBool]still accepts an explicitnull, while the test docstring saysnullis not schema-legal. The field could be updated (nit).
Summary
ossie-schema.jsontypes$defs.Dimension.properties.is_timeas"boolean", but thefield was a plain
Optional[bool], so pydantic's lax mode accepted ten values theschema forbids:
"yes" "true" "True" "no" "off" "1" 1 0 1.0 0.0
The harm is not bad output — it's a lost diagnostic. Coercion stores a real boolean, so
the invalid input is rewritten into valid output and the mistake disappears:
in='yes' -> stored True -> serialized {"is_time": true} -> schema accepts output: True
schema accepts the INPUT 'yes': False
A document written with
is_time: "no"is wrong per the spec, and the SDK is the onlyplace that could say so. It instead turns it into
falseand a later schema check passes.StrictBoolnarrows the field to the two values the schema permits.is_timeis the only"type": "boolean"in the schema and the onlyboolfield in theSDK, so this is the whole of the change.
Related Issues
Checklist
Specification
core-spec/and follow the existing structureOntology
ontology/are consistent with spec changesConverters
converters/is updated to reflect spec or ontology changes.github/labeler.ymlis updated with the new pathValidation
validation/are updated if the spec changedDocumentation
docs/is updated to reflect any user-facing changesCONTRIBUTING.mdis updated if the contribution process changedExamples
examples/are added or updated for any new spec constructs or converter supportTests
pytest/ CI green)Compliance
NOTICEand/orLICENSEhave been updated per ASF policy