betterproto: support Struct and Value

[woodruffw]

Summary

Fixes #332. See also #549.

Opening as a draft until I add tests.

Checklist

• If code changes were made then they have been tested.
  • I have updated the documentation to reflect the changes.
• This PR fixes an issue.
• This PR adds something new (e.g. new method or parameters).
  • This change has an associated test.
• This PR is a breaking change (e.g. methods or parameters removed/renamed)
• This PR is not a code change (e.g. documentation, README, ...)

[woodruffw]

NB: I haven't made any documentation changes, since I believe this behavior was intended (in terms of Protobuf's specified behavior) but just buggy before. However, if there's somewhere I should note this (like the CHANGELOG?) let me know and I'm more than happy to add it 🙂

Similarly, in terms of semver, I believe this is considered a nonbreaking change (since it's a bugfix).

[Gobot1234]

Thanks for this! Honestly I think just putting this behaviour on the type itself would be the best solution. The generated file has been pretty stable for a long time so I don't really have a problem just tacking this on at this point.

[woodruffw]

Thanks for this! Honestly I think just putting this behaviour on the type itself would be the best solution. The generated file has been pretty stable for a long time so I don't really have a problem just tacking this on at this point.

No problem! Making sure I understand: by "on the type itself," you mean specialize to_dict() and from_dict() on Struct itself?

If so, makes sense -- I can do that in a moment.

[Gobot1234]

Thanks for this! Honestly I think just putting this behaviour on the type itself would be the best solution. The generated file has been pretty stable for a long time so I don't really have a problem just tacking this on at this point.

No problem! Making sure I understand: by "on the type itself," you mean specialize to_dict() and from_dict() on Struct itself?

If so, makes sense -- I can do that in a moment.

Yep exactly

[woodruffw]

Alright, moved the specialization to Struct itself!

[woodruffw]

Gentle ping; please let me know if there's anything else I can do here!

[Gobot1234]

Test failures seem unrelated

[woodruffw]

Test failures seem unrelated

Yeah, I think so -- looks like the failure is in Message._betterproto, which then fails to generate cls._betterproto_meta. But I can't see anything in these changes that would cause that.

[woodruffw]

Triaging a bit more: _get_field_default_gen has an issubclass check that fails because t is not a type object.

This suggests that something about type hints is hinky, since t is resolved from them. Maybe a deferred/non-deferred annotations thing, causing t to be a str instead?

[Gobot1234]

I'd personally stick it in quotes manually rather than using the typing version because it'll be easier to remove after #540 lands

[woodruffw]

(Also, if I can make an additional request: a new beta release after this lands would be greatly appreciated!)

[Gobot1234]

Can't do a beta release till grpc/grpc#35329 is merged and #540

[Gobot1234]

Thanks

[atomicmac]

@woodruffw @Gobot1234 Is there anything special that needs to be done for serialization/deserialization of the Structs? From what I'm seeing, instantiating a Struct as exemplified in the tests and then attempting to send it over the wire fails because the Struct's dump method is eventually called, which falls back to the Message.dump method, which treats the struct as a dict, which expects all keys to be of type 'string' and all values to be of type 'message' - if the value is an intrinsic type like str, the catch-all bytes(value) call fails in _preprocess_single because of TypeError('string argument without an encoding')

[Gobot1234]

Oh it seems like it might need special casing for dump yeah

[woodruffw]

Yep, thanks for catching that @atomicmac, and sorry for the gap there. I can look into a fix tomorrow.

[woodruffw]

I've started work on this in #559, but I'm a little stumped on how to actually specialize dump() here -- from a quick look Struct should be transparently serialized as Map<string, Value> in the binary encoding, but I'm not sure what the most idiomatic/straightforward way to do that is.

[atomicmac]

Thanks for looking @Gobot1234 & @woodruffw ! Yes @woodruffw I was looking at implementing the specialized dump() and came to the same crossroads. It would be nice to not have to wrap every value in the struct, but then autoconverting to protobuf types becomes involved if you want to keep transmitted sizes as small as possible.

[woodruffw]

Yes @woodruffw I was looking at implementing the specialized dump() and came to the same crossroads. It would be nice to not have to wrap every value in the struct, but then autoconverting to protobuf types becomes involved if you want to keep transmitted sizes as small as possible.

Glad to hear it's not just me 😅 -- I've been thinking about this a bit, and I still don't have a great idea here.