-
Notifications
You must be signed in to change notification settings - Fork 644
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
Hijack IsLatest(Stable)Version OData filter when semVerLevel=2.0.0 #3966
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,20 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using Microsoft.Data.OData; | ||
using Microsoft.Data.OData.Query; | ||
using System; | ||
using System.Globalization; | ||
using System.Linq; | ||
using System.Net; | ||
using System.Net.Http; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using System.Web; | ||
using System.Web.Http; | ||
using System.Web.Http.OData; | ||
using System.Web.Http.OData.Query; | ||
using System.Web.Http.Results; | ||
using Microsoft.Data.OData; | ||
using Microsoft.Data.OData.Query; | ||
|
||
namespace NuGetGallery.WebApi | ||
{ | ||
|
@@ -37,6 +38,7 @@ public class QueryResult<TModel> | |
private readonly long? _totalResults; | ||
private readonly Func<ODataQueryOptions<TModel>, ODataQuerySettings, long?, Uri> _generateNextLink; | ||
private readonly bool _isPagedResult; | ||
private readonly int? _semVerLevelKey; | ||
|
||
private readonly ODataValidationSettings _validationSettings; | ||
private readonly ODataQuerySettings _querySettings; | ||
|
@@ -59,6 +61,9 @@ public QueryResult( | |
_totalResults = totalResults; | ||
_generateNextLink = generateNextLink; | ||
|
||
var queryDictionary = HttpUtility.ParseQueryString(queryOptions.Request.RequestUri.Query); | ||
_semVerLevelKey = SemVerLevelKey.ForSemVerLevel(queryDictionary["semVerLevel"]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this ok if semVerLevel isn't passed? Or are we guaranteed to always pass semVerLevel? Should we do a TryGet instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is OK with |
||
|
||
if (_totalResults.HasValue && generateNextLink != null) | ||
{ | ||
_isPagedResult = true; | ||
|
@@ -92,7 +97,7 @@ public async Task<HttpResponseMessage> ExecuteAsync(CancellationToken cancellati | |
|
||
return response; | ||
} | ||
catch(Exception e) | ||
catch (Exception e) | ||
{ | ||
QuietLog.LogHandledException(e); | ||
throw; | ||
|
@@ -121,7 +126,22 @@ public IHttpActionResult GetInnerResult() | |
|
||
if (queryOptions.Filter != null) | ||
{ | ||
queryResults = queryOptions.Filter.ApplyTo(queryResults, _querySettings); | ||
if (_semVerLevelKey != SemVerLevelKey.Unknown | ||
&& (string.Equals(queryOptions.Filter.RawValue, "IsLatestVersion", StringComparison.OrdinalIgnoreCase) | ||
|| string.Equals(queryOptions.Filter.RawValue, "IsAbsoluteLatestVersion", StringComparison.OrdinalIgnoreCase))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add constants for these? Looks like these strings are used in SearchAdapter as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 48846ad |
||
{ | ||
// The client uses IsLatestVersion and IsAbsoluteLatestVersion by default, | ||
// and just appends semVerLevel=2.0.0 to the query string. | ||
// When semVerLevel=2.0.0, we should not restrict the filter to only return IsLatest(Stable)=true, | ||
// but also include IsLatest(Stable)SemVer2=true. These additional properties are not exposed on the OData entities however. | ||
// As the proper filtering already should 've happened earlier in the pipeline (SQL or search service), | ||
// the OData filter is redundant, so all we need to do here is to avoid | ||
// the OData filter to be applied on an already correctly filtered result set. | ||
} | ||
else | ||
{ | ||
queryResults = queryOptions.Filter.ApplyTo(queryResults, _querySettings); | ||
} | ||
} | ||
|
||
if (queryOptions.OrderBy != null | ||
|
@@ -302,10 +322,10 @@ private IQueryable ExecuteQuery(IQueryable<TModel> queryable, ODataQueryOptions< | |
{ | ||
return projection; | ||
} | ||
|
||
return queryResult; | ||
} | ||
|
||
private BadRequestErrorMessageResult BadRequest(string message) | ||
{ | ||
return new BadRequestErrorMessageResult(message, _controller); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move under System namespaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 48846ad