🐛(models) fix allow xapi Extensions with empty string values

[piptouque]

Purpose

When defined with a bare Dict[...] as a field in a pydantic class, extensions inherit from the config which disallows empty strings in dict values.

This should be allowed according to spec.

An LRS MUST NOT reject a Statement based on the values
of the extensions map.

Proposal

We override the model_config for our extensions to allow for empty strings.

• Refactored ExtensionMap
• Override model_config for ExtensionMap
• Added tests
• Updated CHANGELOG.md

[MYilFun00]

Request changes

Thanks for the ExtensionMap refactor — the overall approach is clean, and the empty-string fix addresses a real issue (BaseModelWithConfig's str_min_length=1 was rejecting valid extension values).

However, during my local review, I found a couple of blocking issues in the ExtensionMap configuration that should be addressed before this PR can be merged.

🔴 Blocking

1. src/ralph/models/xapi/base/common.py — coerce_numbers_to_str=True should be removed

Current implementation:

class ExtensionMap(RootModel[Dict[IRI, Union[str, int, bool, list, dict, None]]]):
    model_config = ConfigDict(str_min_length=0, coerce_numbers_to_str=True)

The fix only needs to allow empty strings; str_min_length=0 is sufficient.

coerce_numbers_to_str=True is unnecessary because the model already explicitly accepts Union[str, int, bool, ...]. Enabling this option also introduces a data-fidelity risk for an LRS by silently coercing numeric values to strings.

Suggested fix:

class ExtensionMap(RootModel[Dict[IRI, Union[str, int, bool, list, dict, None]]]):
    model_config = ConfigDict(str_min_length=0)

---

2. src/ralph/models/xapi/base/common.py — str_min_length=0 should only apply to extension values

Current implementation:

model_config = ConfigDict(str_min_length=0, ...)

This configuration applies to every string in the model, including IRI keys.

Although empty IRI keys are currently rejected by the IRI validator, the relaxed string constraint should be scoped only to extension values rather than being applied globally.

Suggested fix:

from typing import Annotated
from pydantic import StringConstraints

ExtensionValue = Union[
    Annotated[str, StringConstraints(min_length=0)],
    int,
    bool,
    list,
    dict,
    None,
]

class ExtensionMap(RootModel[Dict[IRI, ExtensionValue]]):
    """Pydantic custom data type for XAPI extensions."""

---

🟡 Non-blocking

• Remove the duplicated test case in the parametrized tests (None appears twice, lines 145–146).

• Replace the try / except / pytest.fail() pattern with direct model instantiation.

• Consider ordering the union as Union[str, bool, int, ...] to avoid the bool/int ambiguity.

• Add a few negative test cases, for example:

  • empty IRI key ({"": 42});
  • unsupported extension value type.

---

✅ Local verification

Verified locally:

• On main, an empty string ("") is rejected because BaseModelWithConfig enforces str_min_length=1.
• With this implementation, empty string extension values are accepted as expected.
• The extension test suite passes successfully (12 passed).

Once the blocking issues above are addressed, please rebase this branch onto the current main (which already includes the CI fixes and PRs #630 and #632) before requesting another review.

[piptouque]

Hi @MYilFun00 , thank you for reviewing this pull request. I've rebased it on current main and address most of the changes you requested / suggested.

Can you elaborate on this remark? I'm not sure how to make a test fail in a cleaner way.

* Replace the `try` / `except` / `pytest.fail()` pattern with direct model instantiation.

[MYilFun00]

Hi @piptouque, thanks for the rebase and for addressing the points so quickly.

I re-tested the branch locally at b6ca46f (rebased on main 62d7d4d). Confirmed fixed:

• coerce_numbers_to_str=True removed ✅
• min_length=0 now scoped to extension values via Annotated[str, StringConstraints(...)] instead of the whole model ✅
• union reordered bool before int ✅
• duplicated None parametrize case removed ✅
• negative test for the empty IRI key ({"": 43}) added ✅

---

On your question about try / except / pytest.fail()

First — my apologies, that nit was poorly aimed. The neighbouring
..._language_map_with_valid_data test in the very same file already uses this
exact pattern, and there are ~24 pytest.fail() occurrences across the suite,
so you were correctly following the local convention. Please treat this as
optional; it is not something I want to hold the PR on.

To answer the actual question: you don't need pytest.fail() to make the test
fail. A test fails as soon as any exception escapes it, so for a
"valid input must be accepted" test, the plain call is already the assertion:

def test_models_xapi_base_common_field_extensions_with_valid_data(values):
    """Test that a valid Extensions field does not raise a `ValidationError`."""

    class DummyExtensionsModel(BaseModel):
        """A dummy pydantic model with an Extensions field."""

        extensions: ExtensionMap

    DummyExtensionsModel(**values)

The reason I prefer this is the failure output. With the try / except
wrapper, pytest prints the ValidationError, then
During handling of the above exception, another exception occurred:, then the
same message again inside Failed: — the useful content is duplicated and the
original traceback is buried:

test_demo.py:11: in test_avec_try_except_pytest_fail
    Dummy(**BAD)
E   pydantic_core._pydantic_core.ValidationError: 1 validation error for Dummy
E   value
E     Input should be a valid integer, unable to parse string as an integer ...

During handling of the above exception, another exception occurred:

test_demo.py:13: in test_avec_try_except_pytest_fail
    pytest.fail(f"Valid data should not raise exceptions: {err}")
E   Failed: Valid data should not raise exceptions: 1 validation error for Dummy
E   value
E     Input should be a valid integer, unable to parse string as an integer ...

Direct instantiation gives the same information once:

test_demo.py:16: in test_instanciation_directe
    Dummy(**BAD)
E   pydantic_core._pydantic_core.ValidationError: 1 validation error for Dummy
E   value
E     Input should be a valid integer, unable to parse string as an integer ...

Secondary point: except ValidationError only wraps that one exception type, so
a TypeError in the same call is reported as an error rather than a Failed —
the handling is inconsistent for no benefit. pytest.fail() earns its place when
there is no exception to rely on (asserting a callback ran, a timeout branch,
an if that should be unreachable), which is not the case here.

The pytest.raises in the invalid-data test is correct as it stands.

---

🔴 One new blocking issue: CI is red (lint + all four test-python jobs)

This one is on me, not on you. You implemented my round-1 suggestion verbatim,
and the snippet I handed you was missing float:

# what I suggested in round 1 — the bug is right here
ExtensionValue = Union[
    Annotated[str, StringConstraints(min_length=0)],
    int,
    bool,      # no float
    list,
    dict,
    None,
]

Dropping coerce_numbers_to_str=True was the right call, but it removed the
crutch that was hiding that omission, and I failed to check what the config was
actually holding up. Reproduced locally on b6ca46f.

float is missing from ExtensionValue

ExtensionValue accepts str | bool | int | list | dict | None but not float.
On main this was invisible because BaseModelWithConfig.coerce_numbers_to_str
silently turned floats into strings. Now that ExtensionMap is its own
RootModel and no longer inherits that config, float extension values are
rejected outright:

result.extensions.`https://w3id.org/xapi/video/extensions/time`
  Input should be a valid string [input_value=1.3035532628854358, input_type=float]

This breaks 10 tests, all the video/LMS profiles that carry time, length or
progress:

tests/models/xapi/base/test_statements.py::..._all_defined_xapi_models[VideoPaused]
  (+ VideoInitialized, VideoPlayed, VideoSeeked, VideoCompleted, VideoTerminated,
     VideoEnableClosedCaptioning, VideoScreenChangeInteraction,
     VideoVolumeChangeInteraction, LMSDownloadedAudio)
→ 10 failed, 848 passed

Worth noting the old behaviour was not good either: 1.3035532628854358 was
stored as the string "1.3035532628854358", which is exactly the fidelity loss
we wanted to remove. The xAPI spec allows any JSON value in an extensions map, so
float simply belongs in the union.

Suggested fix — your current code with one line added:

ExtensionValue = Union[
    Annotated[str, StringConstraints(min_length=0)],
    bool,
    int,
    float,  # <- the only change
    list,
    dict,
    None,
]

Verified locally — with this change the whole model suite is green
(858 passed) and every JSON type round-trips without coercion:

value	main	this PR	PR + float
1.5	⚠️ '1.5' (str)	❌ rejected	✅ 1.5 (float)
42	✅ int	✅ int	✅ int
True	✅ bool	✅ bool	✅ bool
""	❌ rejected	✅ ''	✅ ''
None / [] / {}	✅	✅	✅

A test case for a float extension value would be a good addition to the
parametrized valid-data list, since nothing currently guards this.

ci/circleci: lint

ruff==0.6.5 (the pinned version) reports four errors in
src/ralph/models/xapi/base/common.py:

:3:1   I001  Import block is un-sorted or un-formatted   (Annotated before Dict)
:6:22  F401  `pydantic.ConfigDict` imported but unused   (leftover from the removed model_config)
:6:89  E501  Line too long (94 > 88)
:49:89 E501  Line too long (100 > 88)

ruff format --check also wants to reformat the file — the two missing blank
lines around class ExtensionMap (PEP8 wants two). Splitting the union over
several lines as above resolves both E501s at once, and ConfigDict can now be
dropped from the import list.

After applying the float addition plus these import/formatting fixes, I get
ruff check clean, ruff format --check clean and 858 passed locally.

Happy to re-review as soon as CI is green.