Skip to content

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

Open
piptouque wants to merge 2 commits into
openfun:mainfrom
piptouque:fix_allow_extensions_empty_str
Open

🐛(models) fix allow xapi Extensions with empty string values#631
piptouque wants to merge 2 commits into
openfun:mainfrom
piptouque:fix_allow_extensions_empty_str

Conversation

@piptouque

@piptouque piptouque commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

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

piptouque added 2 commits June 15, 2026 10:34
The 'extensions` dict is shared by contexts and results,
among others.
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, however, should be allowed according to spec:
https://github.com/adlnet/xAPI-Spec/blob/master/xAPI-Data.md#requirements-18
(An LRS MUST NOT reject a Statement based on the values
of the extensions map).

We must override base the model_config for our extensions.

@MYilFun00 MYilFun00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.pycoerce_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.pystr_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.

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.

2 participants