Allow user to initialize or pass in an ETag value for Events client

[Norbo11]

Consider this scenario:

I want to be able to query the Events API for the events which happened while my web service was down, so that I can replay them. However, I want to do this efficiently, and only query for events in a given repo if new ones have been added. Before my service shuts down, it could save the last ETag returned by the Events API endpoint. When it comes back up, it could read this saved value and initialize the Octokit Events client so that it would make requests with the saved ETag.

Very easy thing to do and could potentially be quite useful!

[ryangribble]

Yeah this would definitely be a cool feature to add! According to the docs "most API functions" will support ETag and/or Last-Modified header... which can be specified in If-NoneMatch or If-ModifiedSince headers on requests, which will return 304 Not Modified response rather than actual data, if no new content was present since the specified ETag/timestamp.

The trick is, how to implement it gracefully!

We currently expose the ETag (and can also add the LastModified field) header values via the exiting GetLastApiInfo() method, so we are essentially covered on that side of things...

In terms of specifying the ETag or ModifiedSince values on requests in a broad/elegant way, we could think about adding support for these on our ApiOptions class which we currently use to specify pagination related settings. The problem there is that only GetAll methods supported pagination so they were the only ones that had overloads added that take an ApiOptions parameter. But I guess they are also the most important ones where you want to do ETag based efficient calls anyway. If it was a single entity "Get" request there really isnt much benefit in using this kind of functionality - it's only the "GetAll" requests where it would be most useful... do you agree with that reasoning?

Things to think about

• Should we support ETag and Last-Modified? What happens if one is specified and the API doesnt support it - hopefully this results in an Api error with clear indication of the problem, I'd be happy to bubble that up to the user

• Can we identify any candidates that are NOT GetAll type calls, that would be good to have this functionality on? If so we might want to look at having a base ApiOptions that only supports these fields, and change what is currently ApiOptions to something like ApiPaginationOptions that inherits the base one and add the pagination fields. That way we could use the base one on non GetAll functions

• How should we behave when the 304 response is received? Simply return empty collections?

[mitchdenny]

Hey folks - I'm working on a GitHub App that receives webhook events and ends up making calls back into GitHub to update checks. Its all going well but occasionally the PR status ends up in an inconsistent state when my code may have set in conflicting check run status/conclusion calls.

The problem I'm facing is that the incoming webhook events may come in on any of a host of machines that are processing events and so there is a natural race condition that occurs. Ideally this is where support for ETags would help because my API would be able to query the status of a check (noting the ETag) - evaluate a few conditions and then attempt to update the check providing the ETag. If the ETag had changed then I could get an error.

Basically what I'm after is some kind of concurrency, ETag or upset mechanism. It looks like the REST API provides this but it isn't exposed via Octokit.

[shiftkey]

@mitchdenny yeah, this is something we've been thinking about for a while but haven't really figured out #352. If there's a specific area of the codebase that you'd like to see this supported I'd be open to experimenting with how this might be supported as a spike.

I'd love for this to be more elegant than "just add another parameter, it'll be fiiiine" because between this and pagination and the actual parameters a caller can provide we're definitely hitting a maintenance wall here keeping track of what each endpoint actually supports without overwhelming callers.

[mitchdenny]

@shiftkey I only just spotted this response - thanks. I'll jump over to #352 to continue the conversation there. I'm happy to help here if we can figure out a way of doing this elegantly.

[github-actions]

👋 Hey Friends, this issue 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!