Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix the TeamClient.AddMembership(..) call #856

Merged
merged 5 commits into from
Aug 1, 2015

Conversation

davidalpert
Copy link
Contributor

While exploring some organizational automation with octokit.net I discovered that the TeamClient.AddMembership(..) call passes a null value into a parameter of ApiConnection.Put(..) that fails on an Octokit.Ensure.ArgumentNotNull(..) call.

The GitHub API 3.0 Team docs state that the correct way to call the Add Team Member endpoint is to essentially pass an empty body:

Note that you’ll need to set Content-Length to zero when calling out to this endpoint. For more information, see “HTTP verbs.”

My proposed fix (that I will submit shortly in this PR) is to have ApiConnection.Put(..) check for a null body and respond by setting the Content-Length header to zero as instructed, if that is possible, rather than throwing an ArgumentNullExceptionValue.

Todo

  • vet the design approach;
  • configure the underlying Connection object to add the Content-Length header when the body is null
  • removed the Content-Length header as that generated a System.InvalidOperationException: Misused header name.
  • confirmed worked for my use case, used octokit to add users to a team;

@davidalpert davidalpert force-pushed the add-membership-throws branch 2 times, most recently from dfbd084 to 31aab69 Compare July 31, 2015 18:40
@haacked
Copy link
Contributor

haacked commented Jul 31, 2015

Rather than accepting a null value, I wonder if we should have a special value. For example, we could have a static ApiConnection.EmptyBody property that's just an empty object and use that for cases when you're supposed to pass in an empty body. I try and avoid overloading null like this because it's hard to determine if you passed null as a mistake or intentionally.

Also, what happens if you just pass in new object() right now as the data argument. Does that do the right thing today?

@davidalpert davidalpert force-pushed the add-membership-throws branch 2 times, most recently from 99cc986 to c54dfa1 Compare August 1, 2015 05:15
… inverse dependency from Connection to ApiConnection
@davidalpert davidalpert changed the title [wip] fix the TeamClient.AddMembership(..) call fix the TeamClient.AddMembership(..) call Aug 1, 2015
@davidalpert
Copy link
Contributor Author

@haacked tried your suggestion of an Empty static instead of null; moved it to it's own class to avoid coupling Connection to depend on ApiConnection while ApiConnection depends on IConnection.

Take a look. All tests pass. Ran .\build FixProjects to add ResponseBody.cs to all projects. And it fixed my use case. 👍

@shiftkey
Copy link
Member

shiftkey commented Aug 1, 2015

@davidalpert thanks for finding this and fixing it!

thumbsupfamily

shiftkey added a commit that referenced this pull request Aug 1, 2015
fix the TeamClient.AddMembership(..) call
@shiftkey shiftkey merged commit 747f88d into octokit:master Aug 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants