Optional[Raw] results in an unexpected validation error at json.decode time about msgspec

[jcrist]

Thanks for opening this. Here's a complete reproducer:

import msgspec
from typing import Optional


class Wrapper(msgspec.Struct):
    result: Optional[msgspec.Raw]


msg = b'{"result": {"foo": "bar"}}'

print(msgspec.json.decode(msg, type=Wrapper))
#> Traceback (most recent call last):
#>   File "/home/jcristharif/Code/msgspec/bug.py", line 11, in <module>
#>     print(msgspec.json.decode(msg, type=Wrapper))
#> msgspec.ValidationError: Expected `null`, got `object` - at `$.result`

[jcrist]

Can you say more about your use case for the Optional[Raw] annotation? How do you plan on consuming this field post decode (or producing this field for encoding)?

---

The current Raw decoding logic assumed that Raw nodes won't ever be included in a union like Raw | None (same as Optional[Raw]) . What you're seeing here is a bug in this limitation actually being enforced.

That said, there's a few things we could do here:

1. Error if Raw is ever present in a Union. The type described above would error saying you can't include Raw inside a union like Optional[Raw].
2. Support Raw inside unions, but if Raw is ever present then we always decode it as a Raw value, even if it matches other types in the union. There's some precedence for this - this is how we handle Any today. In your case above mypy would let you pass a None or Raw to result, but when decoding you'd always end up with a Raw, even if the JSON value is null (in that case you'd get Raw(b'null'). The union support would then be most useful for cases where you might want to support passing in-memory objects when programatically constructing these, but on the decode-side always decode them as Raw.
3. Raw is not supported in unions except for with None. So result: Raw or result: Optional[Raw] both work, but result: Raw | int would error as an unsupported type. This restriction matches what we already do for custom types. In this case null would decode as None, but any other value would decode as a Raw value.
4. A mix of 2 and 3. In this case any union with Raw would be supported, but only Raw and None would actually be decoded, everything else would silently be ignored (like in 2).

Given these options, I'm leaning towards either 1 (easiest, best matches the main use case for Raw), or 3 (slightly harder, but may still be useful and matches a convention we had for other types). 2 also seems fine. I think option 4 would be confusing.

[jdef]

My particular use case is processing line-delimited json, each line may contain an object that has a "result" property, or an "error" property. grpc-gateway follows this protocol for streaming responses. the value of "result" and "error" are encoded via protojson (vs regular json) so I want to avoid decoding them using the stock json parser. If I get a vote, it's for "Raw | None"

…

On Wed, Mar 20, 2024 at 8:58 PM Jim Crist-Harif ***@***.***> wrote: Can you say more about your use case for the Optional[Raw] annotation? How do you plan on consuming this field post decode (or producing this field for encoding)? ------------------------------ The current Raw decoding logic assumed that Raw nodes won't ever be included in a union like Raw | None (same as Optional[Raw]) . What you're seeing here is a bug in this limitation actually being enforced. That said, there's a few things we could do here: 1. Error if Raw is ever present in a Union. The type described above would error saying you can't include Raw inside a union like Optional[Raw]. 2. Support Raw inside unions, but if Raw is ever present then we always decode it as a Raw value, even if it matches other types in the union. There's some precedence for this - this is how we handle Any today. In your case above mypy would let you pass a None or Raw to result, but when decoding you'd always end up with a Raw, even if the JSON value is null (in that case you'd get Raw(b'null'). The union support would then be most useful for cases where you might want to support passing in-memory objects when programatically constructing these, but on the decode-side always decode them as Raw. 3. Raw is not supported in unions *except* for with None. So result: Raw or result: Optional[Raw] both work, but result: Raw | int would error as an unsupported type. This restriction matches what we already do for custom types. In this case null would decode as None, but any other value would decode as a Raw value. 4. A mix of 2 and 3. In this case any union with Raw would be supported, but only Raw and None would actually be decoded, everything else would silently be ignored (like in 2). Given these options, I'm leaning towards either 1 (easiest, best matches the main use case for Raw), or 3 (slightly harder, but may still be useful and matches a convention we had for other types). 2 also seems fine. I think option 4 would be confusing. — Reply to this email directly, view it on GitHub <#659 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLE5BTUSU3DBRIRDZT3YZIWERAVCNFSM6AAAAABFAPNLNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJQHE4TGOBUGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- James DeFelice

[jcrist]

If I get a vote, it's for "Raw | None"

Sorry, 2, 3, or 4 could support that type, the question is what happens on decode.

# Given the following type definition
import msgspec

class Wrapper(msgspec.Struct):
    result: msgspec.Raw | None

# And the message
msg = b'{"result": null}'

# The decode call
msgspec.json.decode(msg, type=Wrapper)

# would result in the following values:
# 1. Would error as an unsupported type
# 2. Would decode as `Wrapper(result=Raw(b'null'))`
# 3. Would decode as `Wrapper(result=None)`
# 4. Would decode as `Wrapper(result=None)`

---

As an aside, your use case would probably be best supported by not using Raw | None, but rather having result default to an empty Raw() instance. The following works today:

import msgspec
from typing import Optional


class Wrapper(msgspec.Struct):
    # result is always a Raw type, will use an empty Raw if not present
    result: msgspec.Raw = msgspec.Raw()
    # you didn't say what the structure of `error` was, so guessing string
    error: str = ""


msgs = [
    b'{"result": {"x": 1}}',            # result present
    b'{"result": null}',                # result explicitly null
    b'{"error": "an error message"}',   # result missing, error present
]

for msg in msgs:
    obj = msgspec.json.decode(msg, type=Wrapper)
    # An empty `msgspec.Result()` is false-y, so you can branch on if
    # this field is Truthy to detect the presence of a result.
    if obj.result:
        print(f"result: {bytes(obj.result).decode()!r}")
    else:
        print(f"error: {obj.error!r}")

#> result: '{"x": 1}'
#> result: 'null'
#> error: 'an error message'

[jdef]

The behavior of (3) is what I was expecting, and would IMO be least surprising.

Raw is not supported in unions *except* for with None. So result: Raw or result:

Optional[Raw] both work, but result: Raw | int would error as an unsupported type. This restriction matches what we already do for custom types. In this case null would decode as None, but any other value would decode as a Raw value.

…

On Wed, Mar 20, 2024 at 9:18 PM Jim Crist-Harif ***@***.***> wrote: If I get a vote, it's for "Raw | None" Sorry, 2, 3, or 4 could support that type, the question is what happens on decode. # Given the following type definitionimport msgspec class Wrapper(msgspec.Struct): result: msgspec.Raw | None # And the messagemsg = b'{"result": null}' # The decode callmsgspec.json.decode(msg, type=Wrapper) # would result in the following values:# 1. Would error as an unsupported type# 2. Would decode as `Wrapper(result=Raw(b'null'))`# 3. Would decode as `Wrapper(result=None)`# 4. Would decode as `Wrapper(result=None)` ------------------------------ As an aside, your use case would probably be best supported by *not* using Raw | None, but rather having result default to an empty Raw() instance. The following works today: import msgspecfrom typing import Optional class Wrapper(msgspec.Struct): # result is always a Raw type, will use an empty Raw if not present result: msgspec.Raw = msgspec.Raw() # you didn't say what the structure of `error` was, so guessing string error: str = "" msgs = [ b'{"result": {"x": 1}}', # result present b'{"result": null}', # result explicitly null b'{"error": "an error message"}', # result missing, error present ] for msg in msgs: obj = msgspec.json.decode(msg, type=Wrapper) # An empty `msgspec.Result()` is false-y, so you can branch on if # this field is Truthy to detect the presence of a result. if obj.result: print(f"result: {bytes(obj.result).decode()!r}") else: print(f"error: {obj.error!r}") #> result: '{"x": 1}'#> result: 'null'#> error: 'an error message' — Reply to this email directly, view it on GitHub <#659 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLA2NNFTKF56BZPLB73YZIYONAVCNFSM6AAAAABFAPNLNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJRGAYTEMZUGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- James DeFelice