Skip to content

Fix embedded schema to distinguish between flags and enums#297

Open
naegelejd wants to merge 2 commits into
mainfrom
naegelejd/fix-296
Open

Fix embedded schema to distinguish between flags and enums#297
naegelejd wants to merge 2 commits into
mainfrom
naegelejd/fix-296

Conversation

@naegelejd

Copy link
Copy Markdown
Contributor

Fixes the bug described in #296 .

Open Question

How do we want to support compatibilty with files generated by previous versions of Yardl?

With this patch, any pre-existing stream on disk will now fail the string-equality schema check inside BinaryProtocolReader.__init__ and its C++/Matlab counterparts, because:

  1. The embedded schema baked into the file uses the old flat {"name":"DaysOfWeek","values":[...]} form.
  2. The reader's expected_schema is re-emitted with this fix in the new wrapped form.
  3. Comparison is == on the raw JSON string — no normalization, no structural compare.

Options

  1. Ship as-is and treat it as a schema-format bump: Streams written by earlier yardl releases won't be readable by generators built from this commit. Reasonable because Yardl is still pre v1.0.0 and the issue is explicitly about correcting the schema. Nothing in the wire envelope changes.
  2. Fully non-breaking: make the reader tolerate both old and new schema shapes (e.g. normalize both sides to a canonical form, or JSON-decode + compare structurally). This is the only path that actually protects existing on-disk streams. Bigger change, touches every language SDK.

Thoughts

I'm already working on a general-purpose dynamic Yardl reader feature, which requires dynamically parsing the embedded JSON schema string. Perhaps we should consider a two-step schema validation mechanism:

  • Step 1: String equality comparison on the embedded JSON schema string (currently implemented).
  • Step 2: If Step 1 fails, fall back to a structural comparison:
    1. Parse both embedded schema strings
    2. Canonicalize them, accounting for this particular fix
    3. Compare canonicalized forms

Example:

_TYPE_WRAPPERS = frozenset({"enum", "flags", "record", "alias"})

def _canonicalize_schema(schema: str) -> str:
    try:
        obj = json.loads(schema)
    except json.JSONDecodeError:
        return schema
    types = obj.get("types") if isinstance(obj, dict) else None
    if isinstance(types, list):
        for i, t in enumerate(types):
            if isinstance(t, dict) and len(t) == 1:
                key, value = next(iter(t.items()))
                if key in _TYPE_WRAPPERS:
                    types[i] = value
    return json.dumps(obj, sort_keys=True, separators=(",", ":"))

and the Reader's schema check changes to:

if expected_schema and self._schema != expected_schema:
    if _canonicalize_schema(self._schema) != _canonicalize_schema(expected_schema):
        raise RuntimeError("Invalid schema")

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.

1 participant