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

handle case insensitive headers when parsing for API rate limiting #2175

Merged
merged 7 commits into from
Apr 12, 2020

Conversation

shiftkey
Copy link
Member

@shiftkey shiftkey commented Apr 9, 2020

Related to #2161 but probably not the full fix, given we pass around a dictionary and lookup by key, rather than following the HTTP spec and supporting case insensitive headers.

I chatted with some others inside GitHub and they reminded me about RFC 2616, section 4.2:

Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive.

I'd love to get to a place where we can leverage HttpResponseHeaders but we are very tightly coupled to IResponse in the internals, and it's this component that manages the headers as a dictionary:

/// <summary>
/// Information about the API.
/// </summary>
IReadOnlyDictionary<string, string> Headers { get; }

The setup is here, which converts the HttpResponseHeaders into a dictionary:

return new Response(
responseMessage.StatusCode,
responseBody,
responseMessage.Headers.ToDictionary(h => h.Key, h => h.Value.First()),
contentType);

You'll also note that headers can have multiple values, so the eventual refactoring would need to support that.

  • identify other places where we TryGetValue without handling case-insensitive headers

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #2175 into master will increase coverage by 0.01%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master    #2175      +/-   ##
==========================================
+ Coverage   65.83%   65.85%   +0.01%     
==========================================
  Files         545      545              
  Lines       14234    14248      +14     
  Branches      834      836       +2     
==========================================
+ Hits         9371     9383      +12     
- Misses       4706     4707       +1     
- Partials      157      158       +1     
Impacted Files Coverage Δ
Octokit/Http/RateLimit.cs 93.61% <75.00%> (-3.89%) ⬇️
Octokit/Clients/Enterprise/EnterpriseProbe.cs 90.90% <100.00%> (ø)
Octokit/Exceptions/AbuseException.cs 70.58% <100.00%> (+1.83%) ⬆️
Octokit/Http/ApiInfoParser.cs 100.00% <100.00%> (ø)

@shiftkey shiftkey force-pushed the headers-case-insensitive branch from cbe1103 to e835013 Compare April 9, 2020 16:16
@shiftkey shiftkey marked this pull request as ready for review April 9, 2020 16:36
@shiftkey shiftkey merged commit 8d7bda9 into master Apr 12, 2020
@shiftkey shiftkey deleted the headers-case-insensitive branch April 12, 2020 16:04
@shiftkey
Copy link
Member Author

release_notes: fixed parsing of response headers in GitHubClient.GetLastApiInfo()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants