octokit.net icon indicating copy to clipboard operation
octokit.net copied to clipboard

Published 20 hours ago • verified publisher icon octokit

Auditing API response fields

Open ryangribble opened this issue 8 years ago • 15 comments

We've had a number of issues raised where users have found certain fields provided by the GitHub API arent implemented in Octokit.net (mostly due to historic reasons). We have also found on more than one occasion that fields are included in Json payload responses that aren't necessarily shown in the official GitHub API docs.

I thought it would be interesting to add some trace logging to highlight any fields which are received in API responses that we don't have a matching Octokit field for, and ran as many integration tests as I could.

This is the full unaltered list... Many items in the list should likely remain un-implemented, but there could be some useful/interesting or simply overlooked fields in here. I thought I would post the results and anyone interested could highlight fields they think should be implemented in Octokit.net

Octokit.ActivityPayload

  • action
  • description
  • master_branch
  • member
  • pages
  • pusher_type
  • ref
  • ref_type
  • release

Octokit.ApiError

  • error

Octokit.ApiErrorDetail

  • value

Octokit.ApplicationAuthorization

  • app
  • updated_at
  • user

Octokit.Author

  • gravatar_id

Octokit.Authorization

  • app
  • token
  • updated_at

Octokit.AuthorizedManagementKey

  • pretty-print

Octokit.Blob

  • url

Octokit.BlobReference

  • url

Octokit.Branch

  • _links
  • protected

Octokit.CombinedCommitStatus

  • commit_url
  • url

Octokit.Commit

  • distinct
  • html_url

Octokit.Committer

  • avatar_url
  • events_url
  • followers_url
  • following_url
  • gists_url
  • gravatar_id
  • html_url
  • id
  • login
  • organizations_url
  • received_events_url
  • repos_url
  • site_admin
  • starred_url
  • subscriptions_url
  • type
  • url

Octokit.Deployment

  • environment
  • ref
  • repository_url
  • task

Octokit.DeploymentStatus

  • deployment_url
  • repository_url

Octokit.EventInfo

  • commit_url

Octokit.Gist

  • commits_url
  • forks_url
  • truncated
  • user

Octokit.GistFile

  • truncated

Octokit.GistFork

  • comments
  • comments_url
  • commits_url
  • description
  • files
  • forks_url
  • git_pull_url
  • git_push_url
  • html_url
  • id
  • owner
  • public
  • updated_at

Octokit.GitReference

  • author
  • comments_url
  • commit
  • committer
  • html_url
  • parents

Octokit.Issue

  • labels_url
  • repository_url
  • score

Octokit.IssueComment

  • issue_url

Octokit.License

  • conditions
  • limitations
  • permissions

Octokit.LicenseMetadata

  • featured

Octokit.Migration

  • archive_url
  • owner

Octokit.Milestone

  • html_url
  • id
  • labels_url
  • updated_at

Octokit.Organization

  • billing_email
  • description
  • events_url
  • gravatar_id
  • hooks_url
  • issues_url
  • members_url
  • public_members_url
  • repos_url
  • updated_at

Octokit.PublicKey

  • created_at
  • read_only
  • verified

Octokit.PullRequest

  • _links
  • comments_url
  • commits_url
  • id
  • merge_commit_sha
  • mergeable_state
  • merged
  • review_comment_url
  • review_comments
  • review_comments_url

Octokit.PullRequestReviewComment

  • _links

Octokit.PushEventPayload

  • before
  • distinct_size
  • push_id

Octokit.ReadmeResponse

  • _links
  • download_url
  • git_url
  • path
  • sha
  • size
  • type

Octokit.Repository

  • archive_url
  • assignees_url
  • blobs_url
  • branches_url
  • collaborators_url
  • comments_url
  • commits_url
  • compare_url
  • contents_url
  • contributors_url
  • deployments_url
  • downloads_url
  • events_url
  • forks
  • forks_url
  • git_commits_url
  • git_refs_url
  • git_tags_url
  • has_pages
  • hooks_url
  • issue_comment_url
  • issue_events_url
  • issues_url
  • keys_url
  • labels_url
  • languages_url
  • merges_url
  • milestones_url
  • network_count
  • notifications_url
  • open_issues
  • organization
  • public
  • pulls_url
  • releases_url
  • score
  • size
  • stargazers_url
  • statuses_url
  • subscribers_count
  • subscribers_url
  • subscription_url
  • tags_url
  • teams_url
  • trees_url
  • watchers
  • watchers_count

Octokit.RepositoryContent

  • _links

Octokit.RepositoryContentInfo

  • _links

Octokit.RepositoryContributor

  • gravatar_id

Octokit.RepositoryHook

  • last_response
  • type

Octokit.SearchCode

  • score

Octokit.Team

  • description
  • members_url
  • privacy
  • repositories_url
  • slug

Octokit.User

  • display_login
  • events_url
  • followers_url
  • following_url
  • gists_url
  • gravatar_id
  • gravatar_id
  • organizations_url
  • received_events_url
  • repos_url
  • score
  • starred_url
  • subscriptions_url
  • updated_at

ryangribble avatar Jun 19 '16 11:06 ryangribble

+1

AlenPelin avatar Jun 28 '16 03:06 AlenPelin

Is there any progress on getting some of these fields added? I'm currently having to work around the non-existent "before" field in PushEventPayload, for example.

Norbo11 avatar Jul 25 '17 10:07 Norbo11

@Norbo11 no progress per se, this wasn't a list of things to implement as many of them aren't desired/required (due to being API url fields or previously deprecated fields etc).

It's great you've found one that should be added! would you consider sending a PR to include it?

Thanks

ryangribble avatar Jul 25 '17 12:07 ryangribble

Sure, I can do that at some point this week. Also, I was wondering why, when accessing the Events API, the "deleted" field is not included in PushEvent payloads. I'm not talking about Octokit in particular, I mean the actual Events API payload, which I assumed would return the same payload as the event delivered through i.e. a web hook?

Norbo11 avatar Jul 25 '17 12:07 Norbo11

I'm not sure what the actual reason is for that upstream API behaviour/design but I do see multiple references in the API docs saying that the push event is specifically different payload between the webhook and the events API

https://developer.github.com/webhooks/#payloads https://developer.github.com/v3/activity/events/types/#pushevent

I guess that confirms its expected behaviour but if you wanted your know WHY you could perhaps try contacting github support and asking?

ryangribble avatar Jul 25 '17 12:07 ryangribble

Okay, thanks a lot, I'll see what I can find.

Norbo11 avatar Jul 25 '17 13:07 Norbo11

We were looking at using the distinct field on commits specifically (to cut down on processing). There's a bit of weirdness there in that it's on the webhook commit response, but not on the GET /commit response, and as best I can tell OctoKit uses the same model for both. Would that be a nullable bool (seems easier to add from a 'I could make a PR for this' standpoint)? Or would it need to be split out into two models, one for commits from the API & one for commits from a webhook payload?

BaconSoap avatar Oct 19 '17 14:10 BaconSoap

How different are the response payloads? Would be great if you could post an example commit json response from both cases so we can see how many fields aren't returned.

You're right in that generally we use the same model for the same thing (eg a PullRequest) eventhough some API responses (eg a GetAll type request) don't include all of the fields that are returned from a single object Get request.

It sounds reasonable to make this field nullable (and perhaps any others that are in the same boat) rather than splitting out a new object model, unless there are heaps of discrepancies

ryangribble avatar Oct 19 '17 21:10 ryangribble

I compared what I get from a saved webhook (push event vs from the api) and added the results/diff in a gist.

Key points:

  • webhook uses Id, commit uses Sha
  • webhook has lists of files affected, api does not (and requires generating a diff)
  • webhook his a distinct field, commit doesn't (which makes sense given distinct is only valid at that point in time)
  • api has info about the tree/parents of the commit, and the webhook doesn't

(It turns out that I had actually been using a reimplemented model locally which was how I've been parsing pushes in the first place).

I think the worst difference is that it uses different property names to refer to the same thing - like id vs sha.

BaconSoap avatar Oct 21 '17 14:10 BaconSoap

Maybe we should consider adding an attribute to properties that contain mappings of potential names for the deserializer. For example,

[OtherNames("sha")] 
Public string ID {get} ;

Although this will only work if the type is the same

M-Zuber avatar Oct 21 '17 20:10 M-Zuber

so , how do we do these missing fields? any plans?

niltor avatar Mar 04 '19 08:03 niltor

Hi @niltor,

There aren't any plans to add every extra field en-masse because some of them could be being deprecated and so on...

But certainly if anyone comes across a field that is provided by the API but not defined in octokit.net it can be added to the response models via a pull request. Is there a particular missing field you've encountered?

ryangribble avatar Mar 04 '19 09:03 ryangribble

@ryangribble ok,got it.

niltor avatar Mar 04 '19 09:03 niltor

Deployment.Environment would be useful to have. My use case is the user can rename an enviroment and I want to reflect it in the API.

Cyberboss avatar Aug 07 '20 18:08 Cyberboss

Deployment.Environment would be useful to have

@Cyberboss see https://github.com/octokit/octokit.net/pull/2560

hansmbakker avatar Sep 06 '22 10:09 hansmbakker

Closing this issue, given many of these resources are now present in the current SDK. Please open new issues for other needs.

nickfloyd avatar Nov 03 '22 18:11 nickfloyd