Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions rust-bindings/src/expr/symbol_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,45 @@ impl PySerializedSymbolTable {
}
}

/// Build a ``SerializedSymbolTable`` from its JSON transport
/// text, as produced by ``to_json_str``.
///
/// This is the inverse of ``to_json_str`` and exists for callers
/// that carry the transport form across a process or service
/// boundary — a scheduler persisting the table produced by
/// ``create_job`` and later handing it to a worker, for instance.
/// Prefer ``from_symtab`` when the source is an in-memory
/// ``SymbolTable``.
///
/// Raises ``ValueError`` if the text is not valid JSON. Note that
/// the *contents* are validated lazily: a well-formed JSON
/// document whose entries are not valid symbol table entries is
/// accepted here and rejected by ``to_symtab``.
#[classmethod]
fn from_json_str(_cls: &Bound<'_, pyo3::types::PyType>, json: &str) -> PyResult<Self> {
let inner = openjd_expr::SerializedSymbolTable::from_json_str(json).map_err(|e| {
pyo3::exceptions::PyValueError::new_err(format!(
"Failed to parse SerializedSymbolTable JSON: {e}"
))
})?;
Ok(Self { inner })
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Design note on the lazy-validation choice: for the use case the docstring names — a scheduler persisting the transport text and later handing it to a worker — deferring content validation to to_symtab pushes the error to the worst possible place. The scheduler happily accepts and stores a bad payload, and the failure surfaces later on the worker, far from the input that caused it.

Validating structure at ingest (i.e. parsing into the real entry type rather than a loose JSON document) would make from_json_str a real trust boundary and give the error at the point where the caller still has the offending text in hand. If the laziness is deliberate — e.g. openjd_expr::SerializedSymbolTable::from_json_str intentionally keeps the raw document so unknown future entry kinds pass through untouched — it would help to say so in the docstring, since as written it reads as an accident rather than forward-compatibility.


/// Serialize to the JSON transport text: an array of
/// ``{"name", "type", "value"}`` objects in canonical
/// (lexicographic) path order.
///
/// Use this to move a table across a process or service boundary;
/// pair it with ``from_json_str`` to reconstruct. The result is
/// stable for a given table, so it is safe to store or compare.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The two guarantees documented here — an array of name/type/value objects "in canonical (lexicographic) path order", and "stable for a given table, so it is safe to store or compare" — only hold for instances built via from_symtab.

from_json_str is explicitly lazy (per its own docstring, and confirmed by test_from_json_str_defers_content_validation_to_to_symtab, which stores a non-array document successfully). So inner can hold arbitrary well-formed JSON, and to_json_str re-emits it as-is. Consequences:

  • to_json_str() on a from_json_str-derived instance is not necessarily an array, and not necessarily in canonical path order.
  • Byte-comparing two to_json_str() outputs is not equivalent to comparing table contents: a hand-built transport string with entries in non-lexicographic order, or with duplicate names, survives the round trip and compares unequal to the from_symtab form of the same table.

Since the docstring actively invites callers to store and compare the text, consider scoping the claim (e.g. canonical order and byte-stability hold for tables built via from_symtab; from_json_str preserves the input text as given), or normalizing on ingest in from_json_str so the invariant is unconditional.

fn to_json_str(&self) -> PyResult<String> {
serde_json::to_string(&self.inner).map_err(|e| {
pyo3::exceptions::PyValueError::new_err(format!(
"Failed to serialize SerializedSymbolTable: {e}"
))
})
}

/// Deserialize this serialized symbol table into a full
/// ``SymbolTable`` suitable for inspection or modification.
/// ``path_format`` controls how PATH-typed values are
Expand Down
30 changes: 30 additions & 0 deletions src/openjd/_openjd_rs.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -2032,6 +2032,36 @@ class SerializedSymbolTable:
use this classmethod to convert.
"""

@classmethod
def from_json_str(cls, json: builtins.str) -> SerializedSymbolTable:
r"""
Build a ``SerializedSymbolTable`` from its JSON transport
text, as produced by ``to_json_str``.

This is the inverse of ``to_json_str`` and exists for callers
that carry the transport form across a process or service
boundary — a scheduler persisting the table produced by
``create_job`` and later handing it to a worker, for instance.
Prefer ``from_symtab`` when the source is an in-memory
``SymbolTable``.

Raises ``ValueError`` if the text is not valid JSON. Note that
the *contents* are validated lazily: a well-formed JSON
document whose entries are not valid symbol table entries is
accepted here and rejected by ``to_symtab``.
"""

def to_json_str(self) -> builtins.str:
r"""
Serialize to the JSON transport text: an array of
``{"name", "type", "value"}`` objects in canonical
(lexicographic) path order.

Use this to move a table across a process or service boundary;
pair it with ``from_json_str`` to reconstruct. The result is
stable for a given table, so it is safe to store or compare.
"""

def to_symtab(self, *, path_format: typing.Optional[PathFormat] = None) -> SymbolTable:
r"""
Deserialize this serialized symbol table into a full
Expand Down
126 changes: 125 additions & 1 deletion test/openjd/expr/test_symbol_table.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.

import json
import pickle
from pathlib import Path
from typing import cast

import pytest

import sys

from openjd.expr import ExprValue, SymbolTable, TypeCode
from openjd.expr import ExprValue, SerializedSymbolTable, SymbolTable, TypeCode
from openjd.expr import PathFormat

HOST_PATH_FORMAT = PathFormat.WINDOWS if sys.platform == "win32" else PathFormat.POSIX
Expand Down Expand Up @@ -254,3 +256,125 @@ def test_contains_namespace(self) -> None:
assert "Param" in st
assert "Param.X" in st
assert "Other" not in st


class TestSerializedSymbolTableJson:
"""``to_json_str`` / ``from_json_str`` are the supported way to move a
serialized symbol table across a process or service boundary."""

def test_round_trip_preserves_values(self) -> None:
# GIVEN
symtab = SymbolTable(
{
"Job.Name": "my-job",
"Step.Name": "render",
"Param.Count": 42,
"Param.Scale": 1.5,
"Param.Debug": True,
}
)
serialized = SerializedSymbolTable.from_symtab(symtab)

# WHEN
json_text = serialized.to_json_str()
restored = SerializedSymbolTable.from_json_str(json_text).to_symtab()

# THEN
assert restored["Job.Name"] == ExprValue("my-job")
assert restored["Step.Name"] == ExprValue("render")
assert restored["Param.Count"] == ExprValue(42)
assert restored["Param.Scale"] == ExprValue(1.5)
assert restored["Param.Debug"] == ExprValue(True)

def test_transport_shape(self) -> None:
"""The transport form is an array of {name, type, value} objects in
canonical path order, with scalars carried as strings."""
# GIVEN
symtab = SymbolTable({"Job.Name": "my-job", "Param.Count": 42})

# WHEN
entries = json.loads(SerializedSymbolTable.from_symtab(symtab).to_json_str())

# THEN
assert entries == [
{"name": "Job.Name", "type": "string", "value": "my-job"},
{"name": "Param.Count", "type": "int", "value": "42"},
]

def test_to_json_str_is_stable(self) -> None:
# GIVEN
symtab = SymbolTable({"Param.B": 2, "Param.A": 1})

# WHEN
first = SerializedSymbolTable.from_symtab(symtab).to_json_str()
second = SerializedSymbolTable.from_symtab(symtab).to_json_str()

# THEN
assert first == second

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This asserts determinism (same input, same output twice in one process) but not the canonical-ordering property the docstring actually promises. Both from_symtab calls receive the identical dict, so the test would still pass if the transport order tracked insertion order rather than being sorted.

To pin down "canonical (lexicographic) path order" — and catch nondeterminism from the underlying HashMap iteration order, which is the real risk here — compare two tables built with opposite insertion orders:

first = SerializedSymbolTable.from_symtab(SymbolTable({"Param.B": 2, "Param.A": 1})).to_json_str()
second = SerializedSymbolTable.from_symtab(SymbolTable({"Param.A": 1, "Param.B": 2})).to_json_str()
assert first == second

(Python dict literals preserve insertion order, so these two do reach the Rust side in different orders.)


def test_empty_table_round_trips(self) -> None:
# WHEN
json_text = SerializedSymbolTable.from_symtab(SymbolTable()).to_json_str()

# THEN
assert json_text == "[]"
assert SerializedSymbolTable.from_json_str(json_text).to_symtab().symbols == set()

def test_from_json_str_accepts_hand_built_transport(self) -> None:
"""A caller that builds the transport form itself, rather than going
through ``from_symtab``, gets the same result."""
# GIVEN
hand_built = '[{"name": "Job.Name", "type": "string", "value": "hand-built"}]'

# WHEN
symtab = SerializedSymbolTable.from_json_str(hand_built).to_symtab()

# THEN
assert symtab["Job.Name"] == ExprValue("hand-built")

def test_from_json_str_rejects_malformed_json(self) -> None:
# WHEN / THEN
with pytest.raises(ValueError, match="Failed to parse SerializedSymbolTable JSON"):
SerializedSymbolTable.from_json_str("not json at all")

def test_from_json_str_defers_content_validation_to_to_symtab(self) -> None:
"""Well-formed JSON that is not a valid table is accepted by
``from_json_str`` and rejected by ``to_symtab``."""
# GIVEN
well_formed_but_wrong = '{"not": "an array"}'

# WHEN
serialized = SerializedSymbolTable.from_json_str(well_formed_but_wrong)

# THEN
with pytest.raises(ValueError, match="expected JSON array"):
serialized.to_symtab()

def test_json_round_trip_matches_pickle_round_trip(self) -> None:
"""The JSON form carries the same content as the pickle form, which
round-trips through the same transport text."""
# GIVEN
symtab = SymbolTable({"Job.Name": "my-job", "Param.Count": 42})
serialized = SerializedSymbolTable.from_symtab(symtab)

# WHEN
via_json = SerializedSymbolTable.from_json_str(serialized.to_json_str()).to_symtab()
via_pickle = pickle.loads(pickle.dumps(serialized)).to_symtab()

# THEN
assert via_json.symbols == via_pickle.symbols
for name in via_json.symbols:
assert via_json[name] == via_pickle[name]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This test is circular and cannot fail. __reduce__ (symbol_table.rs:342) serializes with the same serde_json::to_string(&self.inner) that to_json_str uses, and _reconstruct_serialized_symtab calls the same SerializedSymbolTable::from_json_str. So both sides of the comparison are literally the same code path — as the docstring itself notes ("round-trips through the same transport text"). It adds test surface without adding coverage.

If the intent is to guard against pickle and JSON diverging in the future, comparing the two text forms directly would be a tighter check:

assert pickle.loads(pickle.dumps(serialized)).to_json_str() == serialized.to_json_str()

Otherwise dropping it leaves nothing untested — test_round_trip_preserves_values already covers JSON content fidelity.


def test_path_values_round_trip_with_host_format(self) -> None:
# GIVEN
symtab = SymbolTable({"RawParam.Scene": "/proj/scene.blend"})
json_text = SerializedSymbolTable.from_symtab(symtab).to_json_str()

# WHEN
restored = SerializedSymbolTable.from_json_str(json_text).to_symtab(
path_format=HOST_PATH_FORMAT
)

# THEN
assert "RawParam.Scene" in restored

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This test does not exercise PATH-typed values, so it cannot catch a regression in path serialization.

SymbolTable({"RawParam.Scene": "/proj/scene.blend"}) goes through py_to_expr_value with no target type, which infers the native type — i.e. a plain STRING, not PATH. path_format is then irrelevant to the round trip. The existing test at line 43 shows the way to actually build a PATH value:

ExprValue(str(input_file), type="path", path_format=HOST_PATH_FORMAT)

Separately, the only assertion is membership ("RawParam.Scene" in restored), which holds for any surviving key. Asserting the restored value and its type code (e.g. restored["RawParam.Scene"].type == TypeCode.PATH plus the expected host-format string) would make the test meaningful — and would be the one place in this suite verifying that type: "path" survives the JSON transport with the requested path_format applied on the way back.

Loading