Support optionally_keyed_by("foo", dict, use_msgspec=True)#918
Support optionally_keyed_by("foo", dict, use_msgspec=True)#918ahal wants to merge 4 commits intotaskcluster:mainfrom
optionally_keyed_by("foo", dict, use_msgspec=True)#918Conversation
This tests a broader scope of code and will allow us to catch regressions in refactors that change the bigger picture.
This is currently an expected fail due to msgspec limitations.
This value isn't used anywhere, we just want to make sure msgspec doesn't raise an exception. Simplify things a bit.
ahal
force-pushed
the
ahal/push-wqqyrtqxppyt
branch
2 times, most recently
from
8fea0a3 to
1226ac0
Compare
ahal
force-pushed
the
ahal/push-wqqyrtqxppyt
branch
from
1226ac0 to
6399170
Compare
jcristau
left a comment
jcristau
left a comment
There was a problem hiding this comment.
This is too meta for my brain I'm afraid I don't understand the problem or fix :(
Is there a msgspec issue about this, or is this something that can't/won't be fixed there?
|
From: https://jcristharif.com/msgspec/supported-types.html#union-optional
So yeah, probably won't be fixed in msgspec. ProblemSo initially our plan was to convert Which worked great until we ran into a schema that used SolutionI'm not surprised this patch breaks your brain, as Abhishek and I have spent literal days trying to solve it. At a high level, the idea is that instead of validating the above type expression all at once, we break it down into two parts. First we validate the outer This way we can avoid the scenario that resulted in Lmk if that makes sense or if there's any parts you want better comments. |
| with pytest.raises(msgspec.ValidationError): | ||
| TestSchema.validate({"field": {"by-bar": "a"}}) | ||
|
|
||
| # Inner dict values are Any, so mixed types are accepted | ||
| result = TestSchema.validate({"field": {"by-foo": {"a": 1, "c": "d"}}}) | ||
| assert result.field == {"by-foo": {"a": 1, "c": "d"}} | ||
| with pytest.raises(msgspec.ValidationError): | ||
| TestSchema.validate({"field": {"by-bar": {1: "b"}}}) |
There was a problem hiding this comment.
This seems to be changing what's being tested, and unrelated to the commit message?
| if use_msgspec: | ||
| # msgspec implementation - return type hints | ||
| # msgspec implementation - use Annotated[Any, OptionallyKeyedBy] | ||
| _type = arguments[-1] |
There was a problem hiding this comment.
drive-by-nit: might want to make this not raise IndexError if there's no arguments
| # First validate the outer keyed-by dict | ||
| bykeys = UnionTypes(*[Literal[f"by-{field}"] for field in keyed_by.fields]) | ||
| msgspec.convert(obj, dict[bykeys, dict[str, Any]]) | ||
|
|
||
| # Next validate each inner value against the wrapped type | ||
| for pattern, value in keyed_by_dict.items(): | ||
| msgspec.convert(pattern, str) | ||
| msgspec.convert(value, keyed_by.wrapped_type) | ||
|
|
There was a problem hiding this comment.
Some questions here:
- why do we force the keys to be
str? - this doesn't seem to handle things like
optionally_keyed_by("foo", "bar", str)and nested "by-foo" + "by-bar"? By contrast the voluptuous implementation's validator function does recurse.
| TestSchema.validate({"field": {"by-bar": {"a": "b"}}}) | ||
|
|
||
|
|
||
| def test_optionally_keyed_by_mulitple_keys(): |
2 participants
This works around msgspec's limitation where unions can't contain more than one dict.