Optional[Raw] results in an unexpected validation error at json.decode time #659

Activity

1. jcrist commented on Mar 21, 2024

   

   jcrist

   on Mar 21, 2024

   Member

   More actions

   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`
2. jcrist commented on Mar 21, 2024

   

   jcrist

   on Mar 21, 2024

   Member

   More actions

   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.
3. jdef commented on Mar 21, 2024

   

   jdef

   on Mar 21, 2024 via email

   Author

   More actions

   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
4. jcrist commented on Mar 21, 2024

   

   jcrist

   on Mar 21, 2024

   Member

   More actions

   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'
5. jdef commented on Mar 21, 2024

   

   jdef

   on Mar 21, 2024 via email

   Author

   More actions

   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
6. mmatl commented on Jul 31, 2026

   

   mmatl

   on Jul 31, 2026

   More actions

   This is still an issue -- allowing for Optional[Raw] to decode null to None and anything else to Raw would be ideal.
7. sobolevn commented on Nov 9, 2026

   

   sobolevn

   on Nov 9, 2026

   Member

   More actions

   I agree with the empty Raw() approach if the field is optional. Using | None as the special singleton for Raw is not a good idea. Because this breaks the parsing behavior.

   👍React with 👍1Reacted by Janek Nouvertné
8. sobolevn

   closed this as not plannedon Nov 9, 2026