Extracting code: single type per file

[xavierdecoster]

The below types will be affected by semver2-related work (addition of semVerLevel optional parameter).
This separate PR is just another minor refactoring in preparation of semver2-related PRs (avoid pollution + easier diffs).

[skofman1]

🌷 One type per file... what a beauty

[scottbommarito]

I see the potential for a lot of shared code here. I think if we're going to fix this code we might as well improve it all around!

[scottbommarito]

IPackageIdsQuery and IPackageVersionsQuery are nearly identical, the only difference being the name of the parameter. Personally I would merge them into a single interface, IPackageQuery. It will also allow you to share code between the autocomplete queries and the SQL queries more easily.

[xavierdecoster]

See #3644 (comment)

[scottbommarito]

Most of the code here is shared between AutocompleteServicePackageIdsQuery and AutocompleteServicePackageVersionsQuery--the construction of the query string is the only part that seems to differ at all. I would suggest making a single base class that has all of the logic that calls the search service and parses its response. You can then call into this shared functionality with the query string you've constructed.

[xavierdecoster]

Although the code looks similar, the functionality is quite different. In addition, the interfaces are markers for their specific use cases and configured separately in the DI container. See https://github.com/NuGet/NuGetGallery/blob/master/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs#L324.

Would consider such change out-of-scope for this PR.

[scottbommarito]

I can agree that consolidating the two interfaces into IPackageQuery is outside the scope of this PR, but consolidating the code between the two AutocompleteService query classes is minor and I think fits within it well.

public class AutoCompletePackageQuery {
    private readonly ServiceDiscoveryClient _serviceDiscoveryClient;
    private readonly string _autocompleteServiceResourceType;
    private readonly RetryingHttpClientWrapper _httpClient;

    public AutoCompletePackageQuery(IAppConfiguration configuration)
    {
        _serviceDiscoveryClient = new ServiceDiscoveryClient(configuration.ServiceDiscoveryUri);
        _autocompleteServiceResourceType = configuration.AutocompleteServiceResourceType;
        _httpClient = new RetryingHttpClientWrapper(new HttpClient());
    }

    public async Task<IEnumerable<string>> RunQuery(string queryString, bool? includePrerelease) {
    {
        queryString += $"&prerelease={includePrerelease.HasValue ? includePrerelease.Value : false}";
        var endpoints = await _serviceDiscoveryClient.GetEndpointsForResourceType(_autocompleteServiceResourceType);
        endpoints = endpoints.Select(e => new Uri(e + "?" + queryString)).AsEnumerable();
        
        var result = await _httpClient.GetStringAsync(endpoints);
        var resultObject = JObject.Parse(result);

        return resultObject["data"].Select(entry => entry.ToString());
    }
}

so that you can do

public class AutocompleteServicePackageIdsQuery : AutoCompletePackageQuery, IPackageIdsQuery
{
    public async Task<IEnumerable<string>> Execute(string partialId, bool? includePrerelease)
    {
        partialId = partialId ?? string.Empty;
        return RunQuery("take=30&q=" + Uri.EscapeUriString(partialId), includePrerelease);
    }
}

and

public class AutocompleteServicePackageVersionsQuery : AutoCompletePackageQuery, IPackageIdsQuery
{
    public async Task<IEnumerable<string>> Execute(string id, bool? includePrerelease)
    {
        if (string.IsNullOrWhiteSpace(id))
        {
            throw new ArgumentNullException(nameof(id));
        }
        return RunQuery("id=" + Uri.EscapeUriString(id), includePrerelease);
    }
}

[joelverhagen]

There seems to be some value in consolidating the code per @scottbommarito's suggestion. Not sure if there is test coverage so please limit refactoring to tested code.

[xavierdecoster]

@scottbommarito implemented your suggestion, can you please have another look at it?

[scottbommarito]

Looks awesome!

[skofman1]

null check

[skofman1]

same