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

Search requests need to correctly encode offset for date values #2056

Closed
kceiw opened this issue Dec 26, 2019 · 1 comment · Fixed by #2091
Closed

Search requests need to correctly encode offset for date values #2056

kceiw opened this issue Dec 26, 2019 · 1 comment · Fixed by #2091
Assignees
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented

Comments

@kceiw
Copy link

kceiw commented Dec 26, 2019

Test version 0.36.0

The repro snippet

            var endDate = new DateTimeOffset(2012, 12, 20, 16, 0, 0, TimeSpan.FromMinutes(8 * 60)); // offset +08:00
            var startDate = endDate.AddDays(-30);
            var request = new SearchRepositoriesRequest()
            {
                Updated = DateRange.Between(startDate, endDate)
            };
            var githubClient = new GitHubClient(new ProductHeaderValue("MyAmazingApp"));
            var repos = githubClient.Search.SearchRepo(request).Result;
            Console.WriteLine($"ReproCase: Find {repos.TotalCount} repos");

That is becoming to a http request using this url (from Fiddler)
https://api.github.com/search/repositories?page=1&per_page=100&order=desc&q=pushed:2012-11-20T16:00:00+08:00..2012-12-20T16:00:00+08:00

Note that "+" isn't escaped in this case and it only finds zero repositories.

Escaping "+" to "%2B" in a browser gets me total_count 98777

https://api.github.com/search/repositories?page=1&per_page=100&order=desc&q=pushed:2012-11-20T16:00:00%2B08:00..2012-12-20T16:00:00%2B08:00

I attach a .NET core project to demonstrate it.
OctokitRepro.zip

@shiftkey
Copy link
Member

@kceiw this sounds like we should be either encoding here

if (Updated != null)
{
parameters.Add(string.Format(CultureInfo.InvariantCulture, "pushed:{0}", Updated));
}

Or more likely inside DateRange itself:

/// <summary>
/// helper class in generating the date range values for the date qualifier e.g.
/// https://help.github.com/articles/searching-repositories#created-and-last-updated
/// </summary>
[DebuggerDisplay("{DebuggerDisplay,nq}")]
public class DateRange
{

Marking this one as Up for Grabs because I think once we confirm the encoding change this is an easy fix to verify.

@shiftkey shiftkey changed the title Http request doesn't escape all the characters Search requests need to correctly encode offset for date values Jan 19, 2020
@shiftkey shiftkey self-assigned this Feb 10, 2020
@nickfloyd nickfloyd added Type: Bug Something isn't working as documented Status: Up for grabs Issues that are ready to be worked on by anyone and removed category: bug labels Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants