`event` argument type

[arcnmx]

Along the same lines as #11 it would be nice to move away from event: Value as the input to a lambda function. A few options:

• Allowing anything : Deserialize would be great but would rely on inference, which may require an annotation of the type in the handler definition. Not a problem at all for fn handler(event: Whatever, context: Context), but maybe slightly annoying for |event: Whatever, context|? Not sure if that's a real concern or not, this seems like a nice ergonomic approach with a lot of flexibility.
  • Note that this means the handler itself can't catch any errors if the object doesn't conform to the expected structure.
• Value type could just be a newtype wrapper around PyObject that impls Deserializer so that handlers could just do: let event = Whatever::deserialize(event)?. Value-esque accessors to the PyObject's attributes could be supplied for anyone who wants to use it without serde and just access things by string keys.

[softprops]

+1

[softprops]

I wonder if this could be accomplished as is without a breaking change since deserialize is already implemented for value https://docs.serde.rs/serde_json/enum.Value.html

[softprops]

Using something like https://docs.serde.rs/serde_json/fn.from_value.html

[arcnmx]

True, Value does impl Deserializer already, and that is how handlers using serde need to work currently. The main advantage of the newtype approach would be that the implementation details are hidden, so that the PyObject could be passed directly without explosing the Value interim type (which is unnecessary extra allocations/copying/processing/etc) unless the handler wants it.

Since Value also impls Deserialize, existing handlers taking the Value type would work in the first option as well, however would still be a breaking change due to inference changes requiring type annotations for closures.

[softprops]

Feel like this is a worth while breaking change. The only cases where in haven't called serde_json::from_value(...) are cases where I don't need to reference the event, typically scheduled cloudwatch event crons.

The advantage of the current interface is that it gives the application control over error handling when deserializing fails.

For me, I would be willing to fold on that for the convenience of having crowbar do the deserialization for me. This also allows for future optimization where crowbar could skip the intermediary serde_json::Value type and deserialize directly from pyobject to my deserialize type with serde.

[arcnmx]

Agreed, and if you did need to do manual error handling, you could always have your handler use serde_json::Value for the event anyway and it would be equivalent to the current interface. Doing this however would not be able to make use of the mentioned future optimization (until specialization is a thing at least), so may be slightly more inefficient than letting crowbar handle the conversion. I believe the tradeoff for using a serde type directly is worth it though, and the more common use-case, and at least even without the optimization will be no less efficient than what's currently in place.

[softprops]

+1