Allow users to access unsupported Activty event payloads as Json strings

[ryangribble]

Background

As part of the Events API, Activity responses are retrieved that contain a Payload field which varies according to the event in question. In Octokit, we implement the Payload field of the Activity response model as a base type of ActivityPayload but specific inherited payload classes such as IssueCommentPayload, PullRequestEventPayload etc are assigned to this field via some deserializer hackery MAGIC 😀

Problem

When GitHub API implements new event payloads, the payload cannot be obtained via Octokit until a response model class for the specific payload is added (and the magic switch statement is updated).

It actually turns out that GitHub API has currently 37 event payloads, and only 8 of these are implemented in Octokit.net! 🤔

What does this PR do?

Whilst we should implement all of these outstanding payload types (and any future ones!) (see #1646), this PR aims to add a generic "workaround" to Octokit so that in addition to the specific Payload response model classes, there is a PayloadJson field containing the raw json of the event payload. This field can be used by consumers to obtain the payload of all events - regardless of whether they are yet implemented in Octokit or not.

A picture tells 1000 words so here is some debugger output showing what the Payload and PayloadJson fields look like for an implemented and unimplemented event type:

IssueCommentEvent (implemented in Octokit)

The Payload is of explicit type IssueCommentPayload (and PayloadJson is also available)

DeleteEvent (not currently implemented in Octokit)

The Payload is of base type ActivityPayload and doesn't provide anything useful, but the PayloadJson is now available and can be used by clients to at least understand this event prior to a Model for it being added to Octokit.net

Implementation Notes

Implementing this required some tweaks to the deserializer so that we can have more than one response field that maps to the same API value (eg in this case the API field payload maps to both ActivityPayload Payload and string PayloadJson). To limit the impact, I added a new field to the [Parameter] attribute we use, that explicitly marks the parameter declaration AllowDuplicates. In the internals of the deserializer, it then appends a unique value to the api field (so it can be stored in the dictionary for the SetterCache along with other instances of the same api field). In the case of PayloadJson it would end up in the SetterCache as payload_octokit_payload_json. Then when populating models from the api response it removes this appended text from the dictionary key, so it can find the correct API field (payload). It's a bit hacky but IMO no more so than the existing "magic" that was switching out the classes derived from ActivityPayload base class in the first place.

A 2nd deserializer tweak was needed so that an entire JsonObject could be written to a string property (by calling .ToString() on it) - this is used so that the payload API field (a JsonObject structure) can be mapped to our string PayloadJson field.

I also fixed up the existing Event payload integration tests which were previuosly skipped due to them looking for events that are too long ago. I tweaked them to use our pagination support to keep searching until they find an event of the desired type. We also now have an integration test for each of the 8 payload types octokit currently supports. NOTE that there is a chance that these tests can fail if we hit the pagination limit of the events API before we encounter an event of the desired type but this is acceptable for the time being IMO

[ryangribble]

Actually I just realised that there are unit tests here covering each of the payload deserialization (using hardcoded json responses) so im not sure if we need the integration tests or not. On the one hand they are useful since there's no telling whether the upstream API responses may look different to the hardcoded test fixtures but the rarer events do pose a problem in finding them within the 300 events/90day limits on the GitHub events API

[ryangribble]

@shiftkey @khellang would appreciate your thoughts on this from an implementation POV

@Norbo11 would this workaround be useful to you, given you had raised #1635 covering the DeleteEvent payload being unsupported currently?

[khellang]

Would it be more convenient to provide a type wrapping the payload JSON? So you could add a method like DeserializeAs<T>(), which would deserialize the JSON into the specified type using the built-in serializer and settings?

[M-Zuber]

Would it be more convenient to provide a type wrapping the payload JSON? So you could add a method like DeserializeAs(), which would deserialize the JSON into the specified type using the built-in serializer and settings?

I am not following what this gains?

[khellang]

I am not following what this gains?

It gives you a really convenient way to deserialize the JSON into the type you provide, using the custom (GitHub-specific) JSON (de-)serialization settings.

[ryangribble]

It gives you a really convenient way to deserialize the JSON into the type you provide, using the custom (GitHub-specific) JSON (de-)serialization settings.

Do you think there is a use case for consumers providing their own type? Given how specific the deserializer rules are (eg mapping ruby/snake case to c# camel case, using our [Property] attribute on members or enums to determine the API field mappings etc... The class has to be "just right" and if someone was going to go to that effort it would be preferable they send a PR to add the model officially so everyone can use it?

That said I do still like the idea not so that people can BYO response class but more so because it could provide a bit more discoverability and compile time guidance than we currently have.

Eg currently you just "have to know" that you can cast the Payload to another type via activity.Payload as PulRequestEventPayload but there is no evidence you can do this, and no indication on what type you should use.

Whereas if we implemented a method like DeserializeAs<T>() where T: ActivityPayload users would hopefully disover the fact it can be deserialized AND get a compile time check that they are at least specifying a class that derives from ActivityPayload.

Im not sure if ActivityPayload itself should be a wrapper class, similar to the StringEnum you implemented @khellang that contains the json string and can provide it as raw json or deserialized to a specific payload type, or whether we would just have a DeserializePayloadAs<T> method on the Activity object itself.

[khellang]

Do you think there is a use case for consumers providing their own type?

Yes, the use case is that there might be no official type for the payload yet, and you're blocked until next release. Isn't this what we're trying to fix?

if someone was going to go to that effort it would be preferable they send a PR to add the model officially so everyone can use it?

Yes, that would be preferable. But this will let them do the work and unblock themselves first, then submit a PR 😄

currently you just "have to know" that you can cast the Payload to another type via activity.Payload as PulRequestEventPayload but there is no evidence you can do this, and no indication on what type you should use.

What if it's a new payload type that doesn't have a type yet?

Im not sure if ActivityPayload itself should be a wrapper class, similar to the StringEnum you implemented @khellang that contains the json string and can provide it as raw json or deserialized to a specific payload type, or whether we would just have a DeserializePayloadAs<T> method on the Activity object itself.

In my opinion, this problem is the exact same problem as the StringEnum problem. We don't control the payload types and new ones can be introduced at any moment. When that happens, users that want to handle that payload needs to wait for a new release (or, I guess, do the casting).

Is this generic enough for a Payload<T>? I'm om the phone, so don't have 100% control on the door of this thing...

[Norbo11]

@ryangribble Yes, this exactly what I need - then I can use something like JsonConvert in order to deserialize the object into my own DeleteEventPayload class which contains all the fields which I need to access.

But what would be even better is not having to create this class at all, have Octokit deserialize into some kind of dynamic object instead, so that I can access its fields however I like.

[ryangribble]

We could return a dynamic object but I'm not a huge fan of them (and you could do that for yourself anyway from the json string if you want). We also could return our JsonObject class which I believe comes from "simple json" which we use internally as the basis of the deserializer....

I figured the string json was the most flexible though

[thedillonb]

As a consumer of this library, literally anything than what is currently being done now would be preferable. Not having access to the raw JSON payload for all of the unimplemented activity types means that I need to resort to executing a raw request and parse into my own types - which is really unfortunate. The addition of this change would be really helpful.

[ryangribble]

Thanks for the feedback @thedillonb, as you can see above we had a few options regarding how to actually implement this, however I agree that anything is better than how things are now!

I'll aim to pick this back up and at least get something in

[haacked]

There are cases where a user can set a payload such as NewDeployment.Payload which today takes a string. But Deployment.Payload is an IDictionary<string, string> which is almost certainly wrong since it can't handle nested JSON objects.

Perhaps a solution that handles all these cases is important. If I were using JSON.NET I'd make those properties a JObject. Octokit.net uses SimpleJson which I'm not sure what the equivalent is. But I think a strongly typed JSON Expando object type property would make sense.

The nice thing about JObject is you can call JObject.ToObject<T>() so if you have a strongly typed representation of the event, you could still use it.

Is it worth considering moving to System.Text.Json? Or maybe look at SimpleJson to see if it supports a JObject style construct. SimpleJson was chosen originally so that Octokt.net would have zero external dependencies. I wonder if that's so important right now.

[github-actions]

👋 Hey Friends, this pull request has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!