Skip to content

model: replace assert-based validation with raise ValueError (works under python -O) - #516

Merged
erikbosch merged 2 commits into
COVESA:masterfrom
SoundMatt:fix/model-validators-asserts-to-raise
May 12, 2026
Merged

model: replace assert-based validation with raise ValueError (works under python -O)#516
erikbosch merged 2 commits into
COVESA:masterfrom
SoundMatt:fix/model-validators-asserts-to-raise

Conversation

@SoundMatt

Copy link
Copy Markdown
Contributor

Problem

The validators in src/vss_tools/model.py use assert COND, "msg"
to enforce ~32 invariants on VSSData, VSSDataDatatype, and
VSSUnit fields. Pydantic's own documentation explicitly warns
against this pattern: Python's -O optimisation flag disables
asserts, allowing invalid data to pass through.

Anyone running vss-tools under python -O (some packaging
pipelines, performance-tuned environments) gets all VSS spec
validation silently bypassed. The parser accepts invalid specs as
valid, and downstream exporters then act on garbage data.

Reproduction

python -O -c "
from vss_tools.model import VSSDataDatatype
# This *should* raise ValidationError ('foo' is not a datatype)
VSSDataDatatype(type='attribute', description='x', datatype='foo')
"
# Before this PR: silently succeeds (wrong).
# After this PR:  raises ValidationError (correct).

Fix

Mechanically replace each:

assert COND, "msg"

with:

if not COND:
    raise ValueError("msg")

Pydantic wraps a ValueError raised inside a validator as a
ValidationError automatically, so externally-observable behaviour
is identical in normal mode. The new form is preserved under
python -O.

Test

tests/test_model.py already parametrises 50+ invalid-input cases
that expect pydantic.ValidationError. All continue to pass after
this PR in normal mode. They would have failed under python -O
before this change; now they pass in both modes.

I considered adding an explicit python -O job to CI to catch
future regressions of this anti-pattern, but kept it out of this PR
to stay focused. Happy to follow up if maintainers want.

Notes

  • 32 assert statements replaced across VSSData, VSSUnit, and
    VSSDataDatatype validators.
  • A few asserts had not (X and Y) form; those became if X and Y
    for natural inversion. Equivalent semantics, idiomatic Python.
  • One pre-existing falsy-check bug (if not self.default: in
    check_default_min_max) is intentionally left untouched — it's
    a separate validation bug that deserves a focused PR (signal
    with default=0, min=10 silently passes today).

VSSData / VSSDataDatatype / VSSUnit pydantic validators in model.py
used `assert COND, "msg"` to enforce ~32 invariants. Python's `-O`
optimisation flag strips assert statements at compile time, so any
deployment running `python -O` (some packaging pipelines, performance-
tuned environments) silently skipped all VSS spec validation. The
parser would then accept invalid specs as valid.

This is a documented anti-pattern in pydantic's own docs:
https://docs.pydantic.dev/latest/concepts/validators/

Replace each `assert COND, "msg"` with the equivalent
`if not COND: raise ValueError("msg")` form. Pydantic wraps the
ValueError into a ValidationError automatically, so externally-
observable behaviour is identical in normal mode. The difference:
the new form is preserved under `python -O`, restoring validation
in optimised builds.

No test changes needed: the existing parametric suite in
tests/test_model.py already exercises 50+ invalid-input cases and
expects ValidationError. Those tests would have failed under
`python -O` before this change; now they pass in both modes.

Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>

@sschleemilch sschleemilch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a valid one

Six tests in tests/vspec/test_description_error/test_description_error.py
and tests/vspec/test_overlay/test_overlay.py asserted on the literal
string `'type': 'assertion_error'` in the captured log output.

Pydantic reports the error type field differently depending on what the
validator raises:

- `assert COND, "msg"` raises AssertionError, which pydantic surfaces
  as `'type': 'assertion_error'`.
- `if not COND: raise ValueError("msg")` raises ValueError, which
  pydantic surfaces as `'type': 'value_error'`.

The previous commit replaced asserts with raises across model.py;
update the test expectations accordingly.

No production behaviour change — the affected tests still verify that
invalid input is rejected with a ValidationError carrying the same
message; only the type-tag string differs.

Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
@SoundMatt

Copy link
Copy Markdown
Contributor Author

CI caught a regression: six tests in test_description_error/ and
test_overlay/ asserted on the literal string 'type': 'assertion_error'
in pydantic's error output, which becomes 'type': 'value_error' after
the assert -> raise refactor. Pushed a follow-up commit updating those
test expectations. Same coverage, same intent — only the type-tag string
differs.

@erikbosch

Copy link
Copy Markdown
Collaborator

MoM:

  • Presented at meeting
  • ok to merge

@erikbosch
erikbosch merged commit 48cfe11 into COVESA:master May 12, 2026
5 checks passed
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.

3 participants