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

Can't search C# code #1459

Closed
aetos382 opened this Issue Sep 8, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@aetos382

aetos382 commented Sep 8, 2016

When I specify Language.CSharp to SearchCodeRequest.Language, SearchCode request is failed.
Because '#' character contains in request URL and subsequent part is not passed to the server.

The request URL contains the results of Language.ToParameter method.
And the value of ParameterAttribute of Language.CSharp is "C#".

I think should not use Language.ToParameter method in SearchCodeRequest.MergedQualifiers, because it is not used in SearchRepositoriesRequest.

@shiftkey

This comment has been minimized.

Show comment
Hide comment
@shiftkey

shiftkey Sep 9, 2016

Member

Can you provide a failing test here, so we can verify the issue?

Member

shiftkey commented Sep 9, 2016

Can you provide a failing test here, so we can verify the issue?

@aetos382

This comment has been minimized.

Show comment
Hide comment
@aetos382

aetos382 Sep 9, 2016

Please test this.

var client = new GitHubClient(new ProductHeaderValue("SearchCodeSample"));

var searchCodeRequest = new SearchCodeRequest
{
    Language = Language.CSharp
};

searchCodeRequest.Repos.Add("octokit/octokit.net");

// generated URL: "https://api.github.com/search/code?page=1&per_page=100&order=desc&q=language:C#+repo:octokit/octokit.net"
// expected URL : "https://api.github.com/search/code?page=1&per_page=100&order=desc&q=language:CSharp+repo:octokit/octokit.net"
var result = client.Search.SearchCode(searchCodeRequest).Result;

Please find the entire code.

aetos382 commented Sep 9, 2016

Please test this.

var client = new GitHubClient(new ProductHeaderValue("SearchCodeSample"));

var searchCodeRequest = new SearchCodeRequest
{
    Language = Language.CSharp
};

searchCodeRequest.Repos.Add("octokit/octokit.net");

// generated URL: "https://api.github.com/search/code?page=1&per_page=100&order=desc&q=language:C#+repo:octokit/octokit.net"
// expected URL : "https://api.github.com/search/code?page=1&per_page=100&order=desc&q=language:CSharp+repo:octokit/octokit.net"
var result = client.Search.SearchCode(searchCodeRequest).Result;

Please find the entire code.

@dampir

This comment has been minimized.

Show comment
Hide comment
@dampir

dampir Sep 12, 2016

Contributor

@aetos382 good catch! The fix of this issue looks very simple and if you need this fix as ASAP just change attribute value on Language enum.

So, this one SearchRepositoriesRequest.cs#L449

 [Parameter(Value = "C#")]
 CSharp,

should be changed on:

 [Parameter(Value = "CSharp")]
 CSharp,

Anyway, I made some investigation why integration tests in Octokit didn't catch this issue. It's quite interesting thing! Octokit have integration test for SearchCodeRequest SearchClientTests.cs#L436:

    [IntegrationTest]
    public async Task SearchForExcludedLanguage()
    {
        var language = Language.CSharp;

        // Search for issues by include filter
        var request = new SearchIssuesRequest("octokit");
        request.Language = language;

        var issues = await _gitHubClient.Search.SearchIssues(request);

        // Ensure we found issues
        Assert.NotEmpty(issues.Items);

        // Search for issues by exclude filter
        var excludeRequest = new SearchIssuesRequest("octokit");
        excludeRequest.Exclusions = new SearchIssuesRequestExclusions
        {
            Language = language
        };

        var otherIssues = await _gitHubClient.Search.SearchIssues(excludeRequest);

        // Ensure we found issues
        Assert.NotEmpty(otherIssues.Items);

        // Ensure no items from the first search are in the results for the second
        Assert.DoesNotContain(issues.Items, x1 => otherIssues.Items.Any(x2 => x2.Id == x1.Id));
    }

As you can see to create SearchIssuesRequest class the constructor with one parameter is used:

var request = new SearchIssuesRequest("octokit");

The GitHub Search Code Api has some restrictions and one of them is:

You must always include at least one search term when searching source code. For example, searching for language:go is not valid, while amazing language:go is.

So, how this test can be correct in case of this bug with '#' symbol? Answer is simple: this test is correct but not for C#. Instead of sending search request for C# it is sending request on C language!

    [IntegrationTest]
    public async Task SearchForExcludedLanguage()
    {
        var language = Language.CSharp;

        // Search for issues by include filter
        var request = new SearchIssuesRequest("octokit");
        request.Language = language;

        // generated URL: "https://api.github.com/search/?page=1&per_page=100&order=desc&q=GitHub+language:C"
        // Search issues on C language instead of C#
        var issues = await _gitHubClient.Search.SearchIssues(request);

        // Ensure we found issues
        Assert.NotEmpty(issues.Items);

        // Search for issues by exclude filter
        var excludeRequest = new SearchIssuesRequest("octokit");
        excludeRequest.Exclusions = new SearchIssuesRequestExclusions
        {
            Language = language
        };

        var otherIssues = await _gitHubClient.Search.SearchIssues(excludeRequest);

        // Ensure we found issues
        Assert.NotEmpty(otherIssues.Items);

        // Ensure no items from the first search are in the results for the second
        Assert.DoesNotContain(issues.Items, x1 => otherIssues.Items.Any(x2 => x2.Id == x1.Id));
    }

@aetos382 in order to use SearchIssuesRequest in right way you have to use the next constructor:

        /// <summary>
        /// Search using a specify keyword
        /// </summary>
        /// <param name="term">The term to filter on</param>
        public SearchIssuesRequest(string term) : base(term)
        {
            Repos = new RepositoryCollection();
        }

and do not use SearchIssuesRequest parameterless constructor that created for correct Json deserialization.

I'm going to create PR to fix this bug.

Contributor

dampir commented Sep 12, 2016

@aetos382 good catch! The fix of this issue looks very simple and if you need this fix as ASAP just change attribute value on Language enum.

So, this one SearchRepositoriesRequest.cs#L449

 [Parameter(Value = "C#")]
 CSharp,

should be changed on:

 [Parameter(Value = "CSharp")]
 CSharp,

Anyway, I made some investigation why integration tests in Octokit didn't catch this issue. It's quite interesting thing! Octokit have integration test for SearchCodeRequest SearchClientTests.cs#L436:

    [IntegrationTest]
    public async Task SearchForExcludedLanguage()
    {
        var language = Language.CSharp;

        // Search for issues by include filter
        var request = new SearchIssuesRequest("octokit");
        request.Language = language;

        var issues = await _gitHubClient.Search.SearchIssues(request);

        // Ensure we found issues
        Assert.NotEmpty(issues.Items);

        // Search for issues by exclude filter
        var excludeRequest = new SearchIssuesRequest("octokit");
        excludeRequest.Exclusions = new SearchIssuesRequestExclusions
        {
            Language = language
        };

        var otherIssues = await _gitHubClient.Search.SearchIssues(excludeRequest);

        // Ensure we found issues
        Assert.NotEmpty(otherIssues.Items);

        // Ensure no items from the first search are in the results for the second
        Assert.DoesNotContain(issues.Items, x1 => otherIssues.Items.Any(x2 => x2.Id == x1.Id));
    }

As you can see to create SearchIssuesRequest class the constructor with one parameter is used:

var request = new SearchIssuesRequest("octokit");

The GitHub Search Code Api has some restrictions and one of them is:

You must always include at least one search term when searching source code. For example, searching for language:go is not valid, while amazing language:go is.

So, how this test can be correct in case of this bug with '#' symbol? Answer is simple: this test is correct but not for C#. Instead of sending search request for C# it is sending request on C language!

    [IntegrationTest]
    public async Task SearchForExcludedLanguage()
    {
        var language = Language.CSharp;

        // Search for issues by include filter
        var request = new SearchIssuesRequest("octokit");
        request.Language = language;

        // generated URL: "https://api.github.com/search/?page=1&per_page=100&order=desc&q=GitHub+language:C"
        // Search issues on C language instead of C#
        var issues = await _gitHubClient.Search.SearchIssues(request);

        // Ensure we found issues
        Assert.NotEmpty(issues.Items);

        // Search for issues by exclude filter
        var excludeRequest = new SearchIssuesRequest("octokit");
        excludeRequest.Exclusions = new SearchIssuesRequestExclusions
        {
            Language = language
        };

        var otherIssues = await _gitHubClient.Search.SearchIssues(excludeRequest);

        // Ensure we found issues
        Assert.NotEmpty(otherIssues.Items);

        // Ensure no items from the first search are in the results for the second
        Assert.DoesNotContain(issues.Items, x1 => otherIssues.Items.Any(x2 => x2.Id == x1.Id));
    }

@aetos382 in order to use SearchIssuesRequest in right way you have to use the next constructor:

        /// <summary>
        /// Search using a specify keyword
        /// </summary>
        /// <param name="term">The term to filter on</param>
        public SearchIssuesRequest(string term) : base(term)
        {
            Repos = new RepositoryCollection();
        }

and do not use SearchIssuesRequest parameterless constructor that created for correct Json deserialization.

I'm going to create PR to fix this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment