Permalink
Browse files

* Fixing search in feed via Lucene

* Remove id boost to prevent field from being incorrectly normalized
  • Loading branch information...
pranavkm committed Aug 28, 2012
1 parent 1cbda45 commit 87344bd2f57a9c297e72c598ff23a1225567287b
@@ -1,6 +1,6 @@
-using System;
-using System.Collections.Generic;
+using System.Collections.Generic;
using System.Linq;
+using System.Web;
using Moq;
using Xunit;
using Xunit.Extensions;
@@ -9,6 +9,41 @@ namespace NuGetGallery.Services
{
public class FeedServiceFacts
{
+ [Theory]
+ [InlineData("http://nuget.org", "http://nuget.org/")]
+ [InlineData("http://nuget.org/", "http://nuget.org/")]
+ public void SiteRootAddsTrailingSlashes(string siteRoot, string expected)
+ {
+ // Arrange
+ var config = new Mock<IConfiguration>();
+ config.Setup(s => s.GetSiteRoot(false)).Returns(siteRoot);
+ var feed = new V2Feed(entities: null, repo: null, configuration: config.Object, searchSvc: null);
+ feed.HttpContext = GetContext();
+
+ // Act
+ var actual = feed.SiteRoot;
+
+ // Assert
+ Assert.Equal(expected, actual);
+ }
+
+ [Fact]
+ public void SiteRootUsesCurrentRequestToDetermineSiteRoot()
+ {
+ // Arrange
+ var config = new Mock<IConfiguration>();
+ config.Setup(s => s.GetSiteRoot(true)).Returns("https://nuget.org").Verifiable();
+ var feed = new V2Feed(entities: null, repo: null, configuration: config.Object, searchSvc: null);
+ feed.HttpContext = GetContext(isSecure: true);
+
+ // Act
+ var actual = feed.SiteRoot;
+
+ // Assert
+ Assert.Equal("https://nuget.org/", actual);
+ config.Verify();
+ }
+
[Fact]
public void V1FeedSearchDoesNotReturnPrereleasePackages()
{
@@ -305,9 +340,12 @@ public class TestableV1Feed : V1Feed
{
}
- protected override bool UseHttps()
+ protected internal override HttpContextBase HttpContext
{
- return false;
+ get
+ {
+ return GetContext();
+ }
}
}
@@ -321,12 +359,26 @@ public class TestableV2Feed : V2Feed
{
}
- protected override bool UseHttps()
+ protected internal override HttpContextBase HttpContext
{
- return false;
+ get
+ {
+ return GetContext();
+ }
}
}
+ private static HttpContextBase GetContext(bool isSecure = false)
+ {
+ var httpRequest = new Mock<HttpRequestBase>();
+ httpRequest.Setup(s => s.IsSecureConnection).Returns(isSecure);
+ var httpContext = new Mock<HttpContextBase>();
+ httpContext.Setup(s => s.Request).Returns(httpRequest.Object);
+
+ return httpContext.Object;
+ }
+
+
private static void AssertPackage(dynamic expected, V2FeedPackage package)
{
Assert.Equal(expected.Id, package.Id);
@@ -9,7 +9,7 @@ public class DisregardODataInterceptor : ExpressionVisitor
{
protected override Expression VisitMethodCall(MethodCallExpression node)
{
- var methodsToIgnore = new HashSet<string>(new[] { "Take", "Skip", "OrderBy", "ThenBy" }, StringComparer.Ordinal);
+ var methodsToIgnore = new HashSet<string>(new[] { "Skip", "OrderBy", "ThenBy", "OrderByDescending", "ThenByDescending" }, StringComparer.Ordinal);
var method = node.Method;
if ((method.DeclaringType == typeof(Queryable)) && methodsToIgnore.Contains(method.Name))
{
@@ -1,24 +1,31 @@
using System;
-using System.Data.Entity;
using System.Data.Services;
using System.Data.Services.Common;
using System.Data.Services.Providers;
+using System.Diagnostics;
using System.IO;
using System.Linq;
using System.ServiceModel;
using System.Web;
using System.Web.Mvc;
+using Microsoft.Data.OData.Query;
+using Microsoft.Data.OData.Query.SyntacticAst;
using QueryInterceptor;
namespace NuGetGallery
{
[ServiceBehavior(IncludeExceptionDetailInFaults = true, ConcurrencyMode = ConcurrencyMode.Multiple)]
public abstract class FeedServiceBase<TPackage> : DataService<FeedContext<TPackage>>, IDataServiceStreamProvider, IServiceProvider
{
+ /// <summary>
+ /// Determines the maximum number of packages returned in a single page of an OData result.
+ /// </summary>
+ private const int MaxPageSize = 40;
private readonly IEntitiesContext entities;
private readonly IEntityRepository<Package> packageRepo;
private readonly IConfiguration configuration;
private readonly ISearchService searchService;
+ private HttpContextBase httpContext;
public FeedServiceBase()
: this(DependencyResolver.Current.GetService<IEntitiesContext>(),
@@ -61,19 +68,38 @@ protected ISearchService SearchService
get { return searchService; }
}
+ protected internal virtual HttpContextBase HttpContext
+ {
+ get
+ {
+ return httpContext ?? new HttpContextWrapper(System.Web.HttpContext.Current);
+ }
+ set
+ {
+ httpContext = value;
+ }
+ }
+
+ protected internal string SiteRoot
+ {
+ get
+ {
+ string siteRoot = Configuration.GetSiteRoot(UseHttps());
+ return EnsureTrailingSlash(siteRoot);
+ }
+ }
+
// This method is called only once to initialize service-wide policies.
protected static void InitializeServiceBase(DataServiceConfiguration config)
{
config.SetServiceOperationAccessRule("Search", ServiceOperationRights.AllRead);
config.SetServiceOperationAccessRule("FindPackagesById", ServiceOperationRights.AllRead);
config.SetEntitySetAccessRule("Packages", EntitySetRights.AllRead);
- config.SetEntitySetPageSize("Packages", 100);
+ config.SetEntitySetPageSize("Packages", MaxPageSize);
config.DataServiceBehavior.MaxProtocolVersion = DataServiceProtocolVersion.V2;
config.UseVerboseErrors = true;
}
- protected abstract override FeedContext<TPackage> CreateDataSource();
-
public void DeleteStream(
object entity,
DataServiceOperationContext operationContext)
@@ -140,31 +166,24 @@ public object GetService(Type serviceType)
return null;
}
- protected virtual IQueryable<Package> SearchCore(string searchTerm, string targetFramework, bool includePrerelease)
+ protected virtual IQueryable<Package> SearchCore(IQueryable<Package> packages, string searchTerm, string targetFramework, bool includePrerelease)
{
- // Filter out unlisted packages when searching. We will return it when a generic "GetPackages" request comes and filter it on the client.
- var packages = PackageRepo.GetAll()
- .Include(p => p.PackageRegistration)
- .Include(x => x.Authors)
- .Include(x => x.PackageRegistration.Owners)
- .Where(p => p.Listed);
-
- if (String.IsNullOrEmpty(searchTerm))
- {
- return packages;
- }
-
- var request = new HttpRequestWrapper(HttpContext.Current.Request);
- SearchFilter searchFilter;
- // We can only use Lucene if the client queries for the latest versions (IsLatest \ IsLatestStable) versions of a package.
- if (TryReadSearchFilter(request, out searchFilter))
+ SearchFilter searchFilter;
+ // We can only use Lucene if the client queries for the latest versions (IsLatest \ IsLatestStable) versions of a package
+ // and specific sort orders that we have in the index.
+ if (TryReadSearchFilter(HttpContext.Request, out searchFilter))
{
searchFilter.SearchTerm = searchTerm;
searchFilter.IncludePrerelease = includePrerelease;
return GetResultsFromSearchService(packages, searchFilter);
}
+
+ if (!includePrerelease)
+ {
+ packages = packages.Where(p => !p.IsPrerelease);
+ }
return packages.Search(searchTerm);
}
@@ -188,48 +207,91 @@ private IQueryable<Package> GetResultsFromSearchService(IQueryable<Package> pack
private bool TryReadSearchFilter(HttpRequestBase request, out SearchFilter searchFilter)
{
- searchFilter = new SearchFilter
+ var odataQuery = SyntacticTree.ParseUri(new Uri(SiteRoot + request.RawUrl), new Uri(SiteRoot));
+
+ var keywordPath = odataQuery.Path as KeywordSegmentQueryToken;
+ searchFilter = new SearchFilter
{
- Take = ReadInt(request["$top"], 30),
- Skip = ReadInt(request["$skip"], 0),
- CountOnly = request.Path.TrimEnd('/').EndsWith("$count")
+ // HACK: The way the default paging works is WCF attempts to read up to the MaxPageSize elements. If it finds as many, it'll assume there
+ // are more elements to be paged and generate a continuation link. Consequently we'll always ask to pull MaxPageSize elements so WCF generates the
+ // link for us and then allow it to do a Take on the results. The alternative to do is roll our IDataServicePagingProvider, but we run into
+ // issues since we need to manage state over concurrent requests. This seems like an easier solution.
+ Take = MaxPageSize,
+ Skip = odataQuery.Skip ?? 0,
+ CountOnly = keywordPath != null && keywordPath.Keyword == KeywordKind.Count,
+ SortDirection = SortDirection.Ascending
};
- switch(request["$orderby"])
+ var filterProperty = odataQuery.Filter as PropertyAccessQueryToken;
+ if (filterProperty == null ||
+ !(filterProperty.Name.Equals("IsLatestVersion", StringComparison.Ordinal) ||
+ filterProperty.Name.Equals("IsAbsoluteLatestVersion", StringComparison.Ordinal)))
+ {
+ // We'll only use the index if we the query searches for latest \ latest-stable packages
+ return false;
+ }
+
+ var orderBy = odataQuery.OrderByTokens.FirstOrDefault();
+ if (orderBy == null || orderBy.Expression == null)
+ {
+ searchFilter.SortProperty = SortProperty.Relevance;
+ }
+ else if (orderBy.Expression.Kind == QueryTokenKind.PropertyAccess)
{
- case "DownloadCount desc,Id":
+ var propertyAccess = (PropertyAccessQueryToken)orderBy.Expression;
+ if (propertyAccess.Name.Equals("DownloadCount", StringComparison.Ordinal))
+ {
searchFilter.SortProperty = SortProperty.DownloadCount;
- break;
- case "Published desc,Id":
+ }
+ else if (propertyAccess.Name.Equals("Published", StringComparison.Ordinal))
+ {
searchFilter.SortProperty = SortProperty.Recent;
- break;
- case "concat(Title,Id),Id":
+ }
+ else if (propertyAccess.Name.Equals("Id", StringComparison.Ordinal))
+ {
searchFilter.SortProperty = SortProperty.DisplayName;
- searchFilter.SortDirection = SortDirection.Ascending;
- break;
- case "concat(Title,Id) desc,Id":
+ }
+ else
+ {
+ Debug.WriteLine("Order by clause {0} is unsupported", propertyAccess.Name);
+ return false;
+ }
+ }
+ else if (orderBy.Expression.Kind == QueryTokenKind.FunctionCall)
+ {
+ var functionCall = (FunctionCallQueryToken)orderBy.Expression;
+ if (functionCall.Name.Equals("concat", StringComparison.OrdinalIgnoreCase))
+ {
+ // We'll assume this is concat(Title, Id)
searchFilter.SortProperty = SortProperty.DisplayName;
- searchFilter.SortDirection = SortDirection.Descending;
- break;
- default:
- searchFilter.SortProperty = SortProperty.Relevance;
- break;
+ searchFilter.SortDirection = orderBy.Direction == OrderByDirection.Descending ? SortDirection.Descending : SortDirection.Ascending;
+ }
+ else
+ {
+ Debug.WriteLine("Order by clause {0} is unsupported", functionCall.Name);
+ return false;
+ }
}
-
- string filterValue = request["$filter"];
- return (filterValue.IndexOf("IsLatestVersion", StringComparison.Ordinal) != -1) ||
- (filterValue.IndexOf("IsAbsoluteLatestVersion", StringComparison.Ordinal) != -1);
+ else
+ {
+ Debug.WriteLine("Order by clause {0} is unsupported", orderBy.Expression.Kind);
+ return false;
+ }
+ return true;
}
- private int ReadInt(string requestValue, int defaultValue)
+ protected virtual bool UseHttps()
{
- int result;
- return Int32.TryParse(requestValue, out result) ? result : defaultValue;
+ return HttpContext.Request.IsSecureConnection;
}
- protected virtual bool UseHttps()
+ private static string EnsureTrailingSlash(string siteRoot)
{
- return HttpContext.Current.Request.IsSecureConnection;
+ if (!siteRoot.EndsWith("/", StringComparison.Ordinal))
+ {
+ siteRoot = siteRoot + '/';
+ }
+ return siteRoot;
}
}
}
@@ -1,8 +1,6 @@
using System;
-using System.Collections.ObjectModel;
using System.Data.Entity;
using System.Linq;
-using System.Linq.Expressions;
using OData.Linq;
using QueryInterceptor;
@@ -16,9 +14,8 @@ public static IQueryable<V1FeedPackage> ToV1FeedPackageQuery(this IQueryable<Pac
{
siteRoot = EnsureTrailingSlash(siteRoot);
return packages
- .WithoutNullPropagation()
- .WithoutVersionSort()
.Include(p => p.PackageRegistration)
+ .WithoutNullPropagation()
.Select(p => new V1FeedPackage
{
Id = p.PackageRegistration.Id,
@@ -56,9 +53,8 @@ public static IQueryable<V2FeedPackage> ToV2FeedPackageQuery(this IQueryable<Pac
{
siteRoot = EnsureTrailingSlash(siteRoot);
return packages
- .WithoutNullPropagation()
- .WithoutVersionSort()
.Include(p => p.PackageRegistration)
+ .WithoutNullPropagation()
.Select(p => new V2FeedPackage
{
Id = p.PackageRegistration.Id,
Oops, something went wrong.

0 comments on commit 87344bd

Please sign in to comment.