svix-webhooks icon indicating copy to clipboard operation
svix-webhooks copied to clipboard

Libs: make the payload parameter of create message generic, not "any" for all libs

Open tasn opened this issue 1 year ago • 4 comments

In rust: should probably just be a generic over json serializable if it's not already.

For everything else (that supports generic): just make it generic. This will let people have more strict type checking.

tasn avatar Feb 28 '23 22:02 tasn

Did a survey of what's what right now.

  • [x] C# (typed as Object)
  • [x] Go (typed as map[string]interface{} which is probably fine)
  • [x] Java (typed as Object aka POJO)
  • [ ] JavaScript (the ts type is currently any, :-1:)
  • [ ] Kotlin (typed as Any :-1:, need to check to see how generics play with Json lib or otherwise switch to HashMap<String, Any>)
  • [x] PHP seems to not offer APIs for create message :shrug:
  • [x] Python (typed as Dict[str, Any] which is probably okay)
  • [ ] Ruby - not sure what to do here. Looks like we could add some runtime checks to the initializer but...
  • [x] Rust (typed as serde_json::Value which is okay but it would be better if it insisted on an Object variant)

I'm basing this assessment on the fact I thought the payload was meant to be an object, not any valid json value. This is to say, I'd have expected it to be an error if I sent a create message request with:

{"eventType": "nonsense", "payload": false}

The object requirement not be the case according to our spec, but at least some of the code generators seem to be adhering to this.

A problem I see is the typing here is entirely in the hands of the OpenAPI codegen since this is a field on MessageIn. I need to learn more about how to influence what the generators spit out in each problem case.

svix-onelson avatar Mar 09 '23 00:03 svix-onelson

@svix-onelson, I actually meant something, else, but what I meant didn't make sense. Though looking at your report, and given that it's based on MessageIn, I don't think there's anything sane we can do at this point. So maybe let's take a pause on this ticket?

tasn avatar Mar 09 '23 00:03 tasn

@tasn sounds good.

I think we're actually in pretty decent shape as things are. If I were to cherry-pick one of these, it'd be JS. Re-typing as {[key: string]: any} would bring things back inline with most of the others in the list.

svix-onelson avatar Mar 09 '23 01:03 svix-onelson

Yeah, but changing the generation is a bit of a pita. I suspect that maybe updating the generator version, or maybe even fixing our spec (in case it's lacking) would be sufficient.

tasn avatar Mar 09 '23 12:03 tasn