Skip to content

Commit e92834a

Browse files
joelverhagenjoelverhagen
authored
Add hijack=false parameter to OData get specific package (#8161)
* Transition OData functional tests to a standard hijack=false pattern * Only allow hijack=false for simple queries * Don't cache hijack=false * hijack=false is a custom query Progress on NuGet/Engineering#2902
1 parent 9b85f4c commit e92834a

10 files changed

Lines changed: 184 additions & 81 deletions

File tree

‎src/NuGetGallery/Controllers/ODataV2FeedController.cs‎

Lines changed: 75 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,8 @@ public async Task<IHttpActionResult> GetCount(
175175
public async Task<IHttpActionResult> Get(
176176
ODataQueryOptions<V2FeedPackage> options,
177177
string id,
178-
string version)
178+
string version,
179+
[FromUri] bool hijack = true)
179180
{
180181
// We are defaulting to semVerLevel = "2.0.0" by design.
181182
// The client is requesting a specific package version and should support what it requests.
@@ -185,6 +186,7 @@ public async Task<IHttpActionResult> Get(
185186
id,
186187
version,
187188
semVerLevel: SemVerLevelKey.SemVerLevel2,
189+
allowHijack: hijack,
188190
return404NotFoundWhenNoResults: true);
189191

190192
return result.FormattedAsSingleResult<V2FeedPackage>();
@@ -221,6 +223,7 @@ public async Task<IHttpActionResult> FindPackagesById(
221223
id,
222224
version: null,
223225
semVerLevel: semVerLevel,
226+
allowHijack: true,
224227
return404NotFoundWhenNoResults: false);
225228
}
226229

@@ -243,6 +246,7 @@ private async Task<IHttpActionResult> GetCore(
243246
string id,
244247
string version,
245248
string semVerLevel,
249+
bool allowHijack,
246250
bool return404NotFoundWhenNoResults)
247251
{
248252
var packages = GetAll()
@@ -266,60 +270,88 @@ private async Task<IHttpActionResult> GetCore(
266270
var semVerLevelKey = SemVerLevelKey.ForSemVerLevel(semVerLevel);
267271
bool? customQuery = null;
268272

269-
// try the search service
270-
try
273+
if (allowHijack)
271274
{
272-
var searchService = _searchServiceFactory.GetService();
273-
var searchAdaptorResult = await SearchAdaptor.FindByIdAndVersionCore(
274-
searchService,
275-
GetTraditionalHttpContext().Request,
276-
packages,
277-
id,
278-
version,
279-
semVerLevel: semVerLevel);
280-
281-
// If intercepted, create a paged queryresult
282-
if (searchAdaptorResult.ResultsAreProvidedBySearchService)
275+
// try the search service
276+
try
283277
{
284-
customQuery = false;
278+
var searchService = _searchServiceFactory.GetService();
279+
var searchAdaptorResult = await SearchAdaptor.FindByIdAndVersionCore(
280+
searchService,
281+
GetTraditionalHttpContext().Request,
282+
packages,
283+
id,
284+
version,
285+
semVerLevel: semVerLevel);
285286

286-
// Packages provided by search service
287-
packages = searchAdaptorResult.Packages;
287+
// If intercepted, create a paged queryresult
288+
if (searchAdaptorResult.ResultsAreProvidedBySearchService)
289+
{
290+
customQuery = false;
288291

289-
// Add explicit Take() needed to limit search hijack result set size if $top is specified
290-
var totalHits = packages.LongCount();
292+
// Packages provided by search service
293+
packages = searchAdaptorResult.Packages;
294+
295+
// Add explicit Take() needed to limit search hijack result set size if $top is specified
296+
var totalHits = packages.LongCount();
297+
298+
if (totalHits == 0 && return404NotFoundWhenNoResults)
299+
{
300+
_telemetryService.TrackODataCustomQuery(customQuery);
301+
return NotFound();
302+
}
303+
304+
var pagedQueryable = packages
305+
.Take(options.Top != null ? Math.Min(options.Top.Value, MaxPageSize) : MaxPageSize)
306+
.ToV2FeedPackageQuery(
307+
GetSiteRoot(),
308+
_configurationService.Features.FriendlyLicenses,
309+
semVerLevelKey);
291310

292-
if (totalHits == 0 && return404NotFoundWhenNoResults)
311+
return TrackedQueryResult(
312+
options,
313+
pagedQueryable,
314+
MaxPageSize,
315+
totalHits,
316+
(o, s, resultCount) => SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { id }, o, s, semVerLevelKey),
317+
customQuery);
318+
}
319+
else
293320
{
294-
_telemetryService.TrackODataCustomQuery(customQuery);
295-
return NotFound();
321+
customQuery = true;
296322
}
297-
298-
var pagedQueryable = packages
299-
.Take(options.Top != null ? Math.Min(options.Top.Value, MaxPageSize) : MaxPageSize)
300-
.ToV2FeedPackageQuery(
301-
GetSiteRoot(),
302-
_configurationService.Features.FriendlyLicenses,
303-
semVerLevelKey);
304-
305-
return TrackedQueryResult(
306-
options,
307-
pagedQueryable,
308-
MaxPageSize,
309-
totalHits,
310-
(o, s, resultCount) => SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { id }, o, s, semVerLevelKey),
311-
customQuery);
312323
}
313-
else
324+
catch (Exception ex)
314325
{
315-
customQuery = true;
326+
// Swallowing Exception intentionally. If *anything* goes wrong in search, just fall back to the database.
327+
// We don't want to break package restores. We do want to know if this happens, so here goes:
328+
QuietLog.LogHandledException(ex);
316329
}
317330
}
318-
catch (Exception ex)
331+
332+
// When non-hijacked queries are disabled, allow only one non-hijacked pattern: query for a specific ID and
333+
// version without any fancy OData options. This enables some monitoring and testing and is known to produce
334+
// a very fast SQL query based on an optimized index.
335+
var isSimpleLookup = !string.IsNullOrWhiteSpace(id)
336+
&& !string.IsNullOrWhiteSpace(version)
337+
&& options.RawValues.Expand == null
338+
&& options.RawValues.Filter == null
339+
&& options.RawValues.Format == null
340+
&& options.RawValues.InlineCount == null
341+
&& options.RawValues.OrderBy == null
342+
&& options.RawValues.Select == null
343+
&& options.RawValues.Skip == null
344+
&& options.RawValues.SkipToken == null
345+
&& options.RawValues.Top == null;
346+
347+
if (!allowHijack)
319348
{
320-
// Swallowing Exception intentionally. If *anything* goes wrong in search, just fall back to the database.
321-
// We don't want to break package restores. We do want to know if this happens, so here goes:
322-
QuietLog.LogHandledException(ex);
349+
if (!isSimpleLookup)
350+
{
351+
return BadRequest(Strings.ODataParametersDisabled);
352+
}
353+
354+
customQuery = true;
323355
}
324356

325357
if (return404NotFoundWhenNoResults && !packages.Any())

‎src/NuGetGallery/Infrastructure/ODataCacheOutputAttribute.cs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,22 @@ public ODataCacheOutputAttribute(ODataCachedEndpoint endpoint, int serverTimeSpa
4040
ServerTimeSpan = serverTimeSpan;
4141
}
4242

43+
protected override bool IsCachingAllowed(HttpActionContext actionContext, bool anonymousOnly)
44+
{
45+
if (_endpoint == ODataCachedEndpoint.GetSpecificPackage)
46+
{
47+
// Don't cache the non-hijacked ID+version lookup pattern.
48+
if (actionContext.ActionArguments.TryGetValue("hijack", out var hijackObj)
49+
&& hijackObj is bool hijack
50+
&& !hijack)
51+
{
52+
return false;
53+
}
54+
}
55+
56+
return base.IsCachingAllowed(actionContext, anonymousOnly);
57+
}
58+
4359
/// <summary>
4460
/// This setting determines how frequently the cache duration setting should be checked. Since the OData
4561
/// endpoints get a lot of traffic, we don't want to check every time. The 60 seconds here is picked to mimic

‎src/NuGetGallery/Strings.Designer.cs‎

Lines changed: 18 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/NuGetGallery/Strings.resx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1178,4 +1178,7 @@ The {1} Team</value>
11781178
<data name="UploadPackage_InvalidReadmeName" xml:space="preserve">
11791179
<value>the name of &lt;readme&gt; element is case sensitive, must use the &lt;readme&gt;</value>
11801180
</data>
1181+
<data name="ODataParametersDisabled" xml:space="preserve">
1182+
<value>The combination of parameters provided to this OData endpoint is not supported.</value>
1183+
</data>
11811184
</root>

‎tests/NuGetGallery.Facts/Infrastructure/ODataCacheOutputAttributeFacts.cs‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,49 @@ public class ODataCacheOutputAttributeFacts
2626
{
2727
public class OnActionExecutedAsync : Facts
2828
{
29+
[Theory]
30+
[InlineData(ODataCachedEndpoint.GetSpecificPackage, true)]
31+
[InlineData(ODataCachedEndpoint.FindPackagesById, true)]
32+
[InlineData(ODataCachedEndpoint.FindPackagesById, false)]
33+
public async Task CacheHijackFalseOrNonGetSpecificPackage(ODataCachedEndpoint endpoint, bool hijack)
34+
{
35+
ActionContext.ActionArguments["hijack"] = hijack;
36+
var target = new ODataCacheOutputAttribute(endpoint, 100);
37+
target.OnActionExecuting(ActionContext);
38+
39+
var before = DateTimeOffset.Now;
40+
await target.OnActionExecutedAsync(ActionExecutedContext, CancellationToken.None);
41+
var after = DateTimeOffset.Now;
42+
43+
Cache.Verify(
44+
x => x.Add(
45+
It.IsAny<string>(),
46+
It.IsAny<object>(),
47+
It.IsAny<DateTimeOffset>(),
48+
null),
49+
Times.Once);
50+
}
51+
52+
[Fact]
53+
public async Task DoesNotCacheHijackTrueAndGetSpecificPackage()
54+
{
55+
ActionContext.ActionArguments["hijack"] = false;
56+
var target = new ODataCacheOutputAttribute(ODataCachedEndpoint.GetSpecificPackage, 100);
57+
target.OnActionExecuting(ActionContext);
58+
59+
var before = DateTimeOffset.Now;
60+
await target.OnActionExecutedAsync(ActionExecutedContext, CancellationToken.None);
61+
var after = DateTimeOffset.Now;
62+
63+
Cache.Verify(
64+
x => x.Add(
65+
It.IsAny<string>(),
66+
It.IsAny<object>(),
67+
It.IsAny<DateTimeOffset>(),
68+
null),
69+
Times.Never);
70+
}
71+
2972
[Theory]
3073
[MemberData(nameof(CacheEndpointTestData))]
3174
public async Task ObservesConfiguredValue(ODataCachedEndpoint endpoint)

‎tests/NuGetGallery.FunctionalTests.Core/Helpers/ClientSdkHelper.cs‎

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Generic;
66
using System.IO;
77
using System.Linq;
8+
using System.Net;
89
using System.Threading.Tasks;
910
using NuGet;
1011
using NuGet.Versioning;
@@ -67,37 +68,39 @@ public bool CheckIfPackageExistsInSource(string packageId, string sourceUrl)
6768
}
6869

6970
/// <summary>
70-
/// Checks if the given package version is present in V2 and V3. This method bypasses the hijack.
71+
/// Checks if the given package version is present in V2. This method bypasses the hijack.
7172
/// </summary>
72-
private async Task<bool> CheckIfPackageVersionExistsInV2Async(string packageId, string version, bool? isListed)
73+
private async Task<bool> CheckIfPackageVersionExistsInV2Async(string packageId, string version, bool? shouldBeListed)
7374
{
7475
var sourceUrl = UrlHelper.V2FeedRootUrl;
7576
var normalizedVersion = NuGetVersion.Parse(version).ToNormalizedString();
76-
var filter = $"Id eq '{packageId}' and NormalizedVersion eq '{normalizedVersion}' and 1 eq 1";
77-
if (isListed.HasValue)
78-
{
79-
filter += $" and Published {(isListed.Value ? "ne" : "eq")} datetime'1900-01-01T00:00:00'";
80-
}
8177

82-
var url = UrlHelper.V2FeedRootUrl + $"/Packages/$count?$filter={Uri.EscapeDataString(filter)}";
78+
var url = UrlHelper.V2FeedRootUrl + $"/Packages(Id='{packageId}',Version='{normalizedVersion}')?hijack=false";
8379
using (var httpClient = new System.Net.Http.HttpClient())
8480
{
8581
return await VerifyWithRetryAsync(
8682
$"Verifying that package {packageId} {version} exists on source {sourceUrl} (non-hijacked).",
8783
async () =>
8884
{
89-
var count = int.Parse(await httpClient.GetStringAsync(url));
90-
if (count == 0)
91-
{
92-
return false;
93-
}
94-
else if (count == 1)
95-
{
96-
return true;
97-
}
98-
else
85+
using (var response = await httpClient.GetAsync(url))
9986
{
100-
Assert.False(true, $"The count returned by {url} was {count} and it should have been 0 or 1.");
87+
if (response.StatusCode == HttpStatusCode.NotFound)
88+
{
89+
return false;
90+
}
91+
else if (response.StatusCode == HttpStatusCode.OK)
92+
{
93+
if (shouldBeListed.HasValue)
94+
{
95+
var responseString = await response.Content.ReadAsStringAsync();
96+
var isActuallyListed = !responseString.Contains("<d:Published m:type=\"Edm.DateTime\">1900-01-01T00:00:00</d:Published>");
97+
return shouldBeListed == isActuallyListed;
98+
}
99+
100+
return true;
101+
}
102+
103+
response.EnsureSuccessStatusCode();
101104
return false;
102105
}
103106
});

‎tests/NuGetGallery.FunctionalTests.Core/Helpers/ODataHelper.cs‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public async Task<string> TryDownloadPackageFromFeed(string packageId, string ve
6464
string version,
6565
string timestampPropertyName)
6666
{
67-
var url = GetPackagesAppearInFeedInOrderUrl(packageId, version, timestampPropertyName);
67+
var url = $"{UrlHelper.V2FeedRootUrl}/Packages(Id='{packageId}',Version='{version}')?hijack=false";
6868
WriteLine($"Fetching URL: {url}");
6969
var packageResponse = await GetPackageDataInResponse(url, packageId, version);
7070

@@ -91,13 +91,6 @@ public async Task<string> TryDownloadPackageFromFeed(string packageId, string ve
9191
return timestamp;
9292
}
9393

94-
private static string GetPackagesAppearInFeedInOrderUrl(string packageId, string version, string timestampPropertyName)
95-
{
96-
return $"{UrlHelper.V2FeedRootUrl}/Packages?" +
97-
$"$filter=Id eq '{packageId}' and NormalizedVersion eq '{version}' and 1 eq 1&" +
98-
$"$select={timestampPropertyName}";
99-
}
100-
10194
public async Task<string> GetPackageDataInResponse(string url, string packageId, string version)
10295
{
10396
WriteLine($"Getting data for package '{packageId}' with version '{version}'.");

‎tests/NuGetGallery.FunctionalTests/ODataFeeds/CuratedFeedTest.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public CuratedFeedTest(ITestOutputHelper testOutputHelper)
3434
public async Task SearchMicrosoftDotNetCuratedFeed()
3535
{
3636
var packageId = "microsoft.aspnet.webpages";
37-
var requestUrl = UrlHelper.DotnetCuratedFeedUrl + @"Packages()?$filter=tolower(Id)%20eq%20'" + packageId + "'&$orderby=Id&$skip=0&$top=30";
37+
var requestUrl = UrlHelper.DotnetCuratedFeedUrl + @"Search()?searchTerm='packageid%3A" + packageId + "'";
3838

3939
string responseText;
4040
using (var httpClient = new HttpClient())
@@ -54,7 +54,7 @@ public async Task ValidateMicrosoftDotNetCuratedFeed()
5454
{
5555
using (var httpClient = new HttpClient())
5656
{
57-
var requestUrl = UrlHelper.DotnetCuratedFeedUrl + "Packages";
57+
var requestUrl = UrlHelper.DotnetCuratedFeedUrl + "FindPackagesById()?id='AutoMapper'";
5858

5959
string responseText;
6060
using (var response = await httpClient.GetAsync(requestUrl))

0 commit comments

Comments
 (0)