Skip to content

Commit 65185e6

Browse files
joelverhagenjoelverhagen
authored
Add feature flags to all OData endpoints (#8178)
Progress on NuGet/Engineering#2902
1 parent 8f51484 commit 65185e6

12 files changed

Lines changed: 371 additions & 41 deletions

File tree

‎src/NuGetGallery.Services/Configuration/FeatureFlagService.cs‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,16 @@ public class FeatureFlagService : IFeatureFlagService
4040
private const string PackageRenamesFeatureName = GalleryPrefix + "PackageRenames";
4141
private const string EmbeddedReadmeFlightName = GalleryPrefix + "EmbeddedReadmes";
4242

43+
private const string ODataV1GetAllNonHijackedFeatureName = GalleryPrefix + "ODataV1GetAllNonHijacked";
44+
private const string ODataV1GetSpecificNonHijackedFeatureName = GalleryPrefix + "ODataV1GetSpecificNonHijacked";
45+
private const string ODataV1FindPackagesByIdNonHijackedFeatureName = GalleryPrefix + "ODataV1FindPackagesByIdNonHijacked";
46+
private const string ODataV1SearchNonHijackedFeatureName = GalleryPrefix + "ODataV1SearchNonHijacked";
47+
48+
private const string ODataV2GetAllNonHijackedFeatureName = GalleryPrefix + "ODataV2GetAllNonHijacked";
49+
private const string ODataV2GetSpecificNonHijackedFeatureName = GalleryPrefix + "ODataV2GetSpecificNonHijacked";
50+
private const string ODataV2FindPackagesByIdNonHijackedFeatureName = GalleryPrefix + "ODataV2FindPackagesByIdNonHijacked";
51+
private const string ODataV2SearchNonHijackedFeatureName = GalleryPrefix + "ODataV2SearchNonHijacked";
52+
4353
private readonly IFeatureFlagClient _client;
4454

4555
public FeatureFlagService(IFeatureFlagClient client)
@@ -118,11 +128,6 @@ public bool IsForceFlatContainerIconsEnabled()
118128
return _client.IsEnabled(ForceFlatContainerIconsFeatureName, defaultValue: false);
119129
}
120130

121-
private bool IsEnabled(string flight, User user, bool defaultValue)
122-
{
123-
return _client.IsEnabled(flight, user, defaultValue);
124-
}
125-
126131
public bool IsODataDatabaseReadOnlyEnabled()
127132
{
128133
return _client.IsEnabled(ODataReadOnlyDatabaseFeatureName, defaultValue: false);
@@ -197,5 +202,45 @@ public bool AreEmbeddedReadmesEnabled(User user)
197202
{
198203
return _client.IsEnabled(EmbeddedReadmeFlightName, user, defaultValue: false);
199204
}
205+
206+
public bool IsODataV1GetAllEnabled()
207+
{
208+
return _client.IsEnabled(ODataV1GetAllNonHijackedFeatureName, defaultValue: true);
209+
}
210+
211+
public bool IsODataV1GetSpecificNonHijackedEnabled()
212+
{
213+
return _client.IsEnabled(ODataV1GetSpecificNonHijackedFeatureName, defaultValue: true);
214+
}
215+
216+
public bool IsODataV1FindPackagesByIdNonHijackedEnabled()
217+
{
218+
return _client.IsEnabled(ODataV1FindPackagesByIdNonHijackedFeatureName, defaultValue: true);
219+
}
220+
221+
public bool IsODataV1SearchNonHijackedEnabled()
222+
{
223+
return _client.IsEnabled(ODataV1SearchNonHijackedFeatureName, defaultValue: true);
224+
}
225+
226+
public bool IsODataV2GetAllNonHijackedEnabled()
227+
{
228+
return _client.IsEnabled(ODataV2GetAllNonHijackedFeatureName, defaultValue: true);
229+
}
230+
231+
public bool IsODataV2GetSpecificNonHijackedEnabled()
232+
{
233+
return _client.IsEnabled(ODataV2GetSpecificNonHijackedFeatureName, defaultValue: true);
234+
}
235+
236+
public bool IsODataV2FindPackagesByIdNonHijackedEnabled()
237+
{
238+
return _client.IsEnabled(ODataV2FindPackagesByIdNonHijackedFeatureName, defaultValue: true);
239+
}
240+
241+
public bool IsODataV2SearchNonHijackedEnabled()
242+
{
243+
return _client.IsEnabled(ODataV2SearchNonHijackedFeatureName, defaultValue: true);
244+
}
200245
}
201246
}

‎src/NuGetGallery.Services/Configuration/IFeatureFlagService.cs‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,5 +151,45 @@ public interface IFeatureFlagService
151151
/// Whether the user is able to publish the package with an embedded readme file.
152152
/// </summary>
153153
bool AreEmbeddedReadmesEnabled(User user);
154+
155+
/// <summary>
156+
/// Whether the /Packages() endpoint is enabled for the V1 OData API.
157+
/// </summary>
158+
bool IsODataV1GetAllEnabled();
159+
160+
/// <summary>
161+
/// Whether the /Packages(Id=,Version=) endpoint is enabled for non-hijacked queries for the V1 OData API.
162+
/// </summary>
163+
bool IsODataV1GetSpecificNonHijackedEnabled();
164+
165+
/// <summary>
166+
/// Whether the /FindPackagesById() endpoint is enabled for non-hijacked queries for the V1 OData API.
167+
/// </summary>
168+
bool IsODataV1FindPackagesByIdNonHijackedEnabled();
169+
170+
/// <summary>
171+
/// Whether the /Search() endpoint is enabled for non-hijacked queries for the V1 OData API.
172+
/// </summary>
173+
bool IsODataV1SearchNonHijackedEnabled();
174+
175+
/// <summary>
176+
/// Whether the /Packages() endpoint is enabled for non-hijacked queries for the V2 OData API.
177+
/// </summary>
178+
bool IsODataV2GetAllNonHijackedEnabled();
179+
180+
/// <summary>
181+
/// Whether the /Packages(Id=,Version=) endpoint is enabled for non-hijacked queries for the V2 OData API.
182+
/// </summary>
183+
bool IsODataV2GetSpecificNonHijackedEnabled();
184+
185+
/// <summary>
186+
/// Whether the /FindPackagesById() endpoint is enabled for non-hijacked queries for the V2 OData API.
187+
/// </summary>
188+
bool IsODataV2FindPackagesByIdNonHijackedEnabled();
189+
190+
/// <summary>
191+
/// Whether the /Search() endpoint is enabled for non-hijacked queries for the V2 OData API.
192+
/// </summary>
193+
bool IsODataV2SearchNonHijackedEnabled();
154194
}
155195
}

‎src/NuGetGallery/App_Data/Files/Content/flags.json‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,15 @@
88
"NuGetGallery.ODataCacheDurations": "Enabled",
99
"NuGetGallery.ShowEnable2FADialog": "Enabled",
1010
"NuGetGallery.Get2FADismissFeedback": "Disabled",
11-
"NuGetGallery.UsabillaEveryPage": "Enabled"
11+
"NuGetGallery.UsabillaEveryPage": "Enabled",
12+
"NuGetGallery.ODataV1GetAllNonHijacked": "Enabled",
13+
"NuGetGallery.ODataV1GetSpecificNonHijacked": "Enabled",
14+
"NuGetGallery.ODataV1FindPackagesByIdNonHijacked": "Enabled",
15+
"NuGetGallery.ODataV1SearchNonHijacked": "Enabled",
16+
"NuGetGallery.ODataV2GetAllNonHijacked": "Enabled",
17+
"NuGetGallery.ODataV2GetSpecificNonHijacked": "Enabled",
18+
"NuGetGallery.ODataV2FindPackagesByIdNonHijacked": "Enabled",
19+
"NuGetGallery.ODataV2SearchNonHijacked": "Enabled"
1220
},
1321
"Flights": {
1422
"NuGetGallery.TyposquattingFlight": {

‎src/NuGetGallery/Controllers/ODataV1FeedController.cs‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,11 @@ public ODataV1FeedController(
5454
[CacheOutput(NoCache = true)]
5555
public IHttpActionResult Get(ODataQueryOptions<V1FeedPackage> options)
5656
{
57+
if (!_featureFlagService.IsODataV1GetAllEnabled())
58+
{
59+
return BadRequest(Strings.ODataDisabled);
60+
}
61+
5762
if (!ODataQueryVerifier.AreODataOptionsAllowed(options, ODataQueryVerifier.V1Packages,
5863
_configurationService.Current.IsODataFilterEnabled, nameof(Get)))
5964
{
@@ -94,7 +99,12 @@ public IHttpActionResult GetCount(ODataQueryOptions<V1FeedPackage> options)
9499
ClientTimeSpan = ODataCacheConfiguration.DefaultGetByIdAndVersionCacheTimeInSeconds)]
95100
public async Task<IHttpActionResult> Get(ODataQueryOptions<V1FeedPackage> options, string id, string version)
96101
{
97-
var result = await GetCore(options, id, version, return404NotFoundWhenNoResults: true);
102+
var result = await GetCore(
103+
options,
104+
id,
105+
version,
106+
return404NotFoundWhenNoResults: true,
107+
isNonHijackEnabled: _featureFlagService.IsODataV1GetSpecificNonHijackedEnabled());
98108
return result.FormattedAsSingleResult<V1FeedPackage>();
99109
}
100110

@@ -108,7 +118,12 @@ public async Task<IHttpActionResult> Get(ODataQueryOptions<V1FeedPackage> option
108118
ClientTimeSpan = ODataCacheConfiguration.DefaultGetByIdAndVersionCacheTimeInSeconds)]
109119
public async Task<IHttpActionResult> FindPackagesById(ODataQueryOptions<V1FeedPackage> options, [FromODataUri]string id)
110120
{
111-
return await GetCore(options, id, version: null, return404NotFoundWhenNoResults: false);
121+
return await GetCore(
122+
options,
123+
id,
124+
version: null,
125+
return404NotFoundWhenNoResults: false,
126+
isNonHijackEnabled: _featureFlagService.IsODataV1FindPackagesByIdNonHijackedEnabled());
112127
}
113128

114129
// /api/v1/FindPackagesById()/$count?id=
@@ -123,7 +138,12 @@ public async Task<IHttpActionResult> FindPackagesByIdCount(ODataQueryOptions<V1F
123138
return result.FormattedAsCountResult<V1FeedPackage>();
124139
}
125140

126-
private async Task<IHttpActionResult> GetCore(ODataQueryOptions<V1FeedPackage> options, string id, string version, bool return404NotFoundWhenNoResults)
141+
private async Task<IHttpActionResult> GetCore(
142+
ODataQueryOptions<V1FeedPackage> options,
143+
string id,
144+
string version,
145+
bool return404NotFoundWhenNoResults,
146+
bool isNonHijackEnabled)
127147
{
128148
var packages = GetAll()
129149
.Include(p => p.PackageRegistration)
@@ -185,13 +205,20 @@ private async Task<IHttpActionResult> GetCore(ODataQueryOptions<V1FeedPackage> o
185205
customQuery = true;
186206
}
187207
}
188-
catch (Exception ex)
208+
catch (Exception ex) when (isNonHijackEnabled)
189209
{
190-
// Swallowing Exception intentionally. If *anything* goes wrong in search, just fall back to the database.
191-
// We don't want to break package restores. We do want to know if this happens, so here goes:
210+
// Swallowing exception intentionally if we are allowing a fallback to database. If non-hijacked
211+
// queries are disabled, let the exception bubble out and the client will retry.
192212
QuietLog.LogHandledException(ex);
193213
}
194214

215+
// If we've reached this point, the hijack to the search service has failed or is not applicable. If
216+
// non-hijacked queries are disabled, stop here.
217+
if (!isNonHijackEnabled)
218+
{
219+
return BadRequest(Strings.ODataParametersDisabled);
220+
}
221+
195222
if (return404NotFoundWhenNoResults && !packages.Any())
196223
{
197224
_telemetryService.TrackODataCustomQuery(customQuery);
@@ -292,6 +319,11 @@ public async Task<IHttpActionResult> Search(
292319
customQuery = true;
293320
}
294321

322+
if (!_featureFlagService.IsODataV1SearchNonHijackedEnabled())
323+
{
324+
return BadRequest(Strings.ODataParametersDisabled);
325+
}
326+
295327
if (!ODataQueryVerifier.AreODataOptionsAllowed(options, ODataQueryVerifier.V1Search,
296328
_configurationService.Current.IsODataFilterEnabled, nameof(Search)))
297329
{

‎src/NuGetGallery/Controllers/ODataV2FeedController.cs‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,8 @@ public async Task<IHttpActionResult> Get(
7979
var semVerLevelKey = SemVerLevelKey.ForSemVerLevel(semVerLevel);
8080
bool? customQuery = null;
8181

82+
var isNonHijackEnabled = _featureFlagService.IsODataV2GetAllNonHijackedEnabled();
83+
8284
// Try the search service
8385
try
8486
{
@@ -129,17 +131,22 @@ public async Task<IHttpActionResult> Get(
129131
customQuery = true;
130132
}
131133
}
132-
catch (ODataException ex) when (ex.InnerException != null && ex.InnerException is FormatException)
134+
catch (ODataException ex) when (isNonHijackEnabled && ex.InnerException != null && ex.InnerException is FormatException)
133135
{
134136
// Sometimes users make invalid requests. It's not exceptional behavior, don't trace.
135137
}
136-
catch (Exception ex)
138+
catch (Exception ex) when (isNonHijackEnabled)
137139
{
138140
// Swallowing Exception intentionally. If *anything* goes wrong in search, just fall back to the database.
139141
// We don't want to break package restores. We do want to know if this happens, so here goes:
140142
QuietLog.LogHandledException(ex);
141143
}
142144

145+
if (!isNonHijackEnabled)
146+
{
147+
return BadRequest(Strings.ODataParametersDisabled);
148+
}
149+
143150
// Reject only when try to reach database.
144151
if (!ODataQueryVerifier.AreODataOptionsAllowed(options, ODataQueryVerifier.V2Packages,
145152
_configurationService.Current.IsODataFilterEnabled, nameof(Get)))
@@ -184,10 +191,11 @@ public async Task<IHttpActionResult> Get(
184191
var result = await GetCore(
185192
options,
186193
id,
187-
version,
188-
semVerLevel: SemVerLevelKey.SemVerLevel2,
194+
version,
195+
semVerLevel: SemVerLevelKey.SemVerLevel2,
189196
allowHijack: hijack,
190-
return404NotFoundWhenNoResults: true);
197+
return404NotFoundWhenNoResults: true,
198+
isNonHijackEnabled: _featureFlagService.IsODataV2GetSpecificNonHijackedEnabled());
191199

192200
return result.FormattedAsSingleResult<V2FeedPackage>();
193201
}
@@ -224,7 +232,8 @@ public async Task<IHttpActionResult> FindPackagesById(
224232
version: null,
225233
semVerLevel: semVerLevel,
226234
allowHijack: true,
227-
return404NotFoundWhenNoResults: false);
235+
return404NotFoundWhenNoResults: false,
236+
isNonHijackEnabled: _featureFlagService.IsODataV2FindPackagesByIdNonHijackedEnabled());
228237
}
229238

230239
// /api/v2/FindPackagesById()/$count?semVerLevel=
@@ -247,7 +256,8 @@ private async Task<IHttpActionResult> GetCore(
247256
string version,
248257
string semVerLevel,
249258
bool allowHijack,
250-
bool return404NotFoundWhenNoResults)
259+
bool return404NotFoundWhenNoResults,
260+
bool isNonHijackEnabled)
251261
{
252262
var packages = GetAll()
253263
.Include(p => p.PackageRegistration)
@@ -344,7 +354,7 @@ private async Task<IHttpActionResult> GetCore(
344354
&& options.RawValues.SkipToken == null
345355
&& options.RawValues.Top == null;
346356

347-
if (!allowHijack)
357+
if (!allowHijack || !isNonHijackEnabled)
348358
{
349359
if (!isSimpleLookup)
350360
{
@@ -474,6 +484,11 @@ public async Task<IHttpActionResult> Search(
474484
customQuery = true;
475485
}
476486

487+
if (!_featureFlagService.IsODataV2SearchNonHijackedEnabled())
488+
{
489+
return BadRequest(Strings.ODataParametersDisabled);
490+
}
491+
477492
//Reject only when try to reach database.
478493
if (!ODataQueryVerifier.AreODataOptionsAllowed(options, ODataQueryVerifier.V2Search,
479494
_configurationService.Current.IsODataFilterEnabled, nameof(Search)))

‎src/NuGetGallery/Strings.Designer.cs‎

Lines changed: 9 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,6 +1178,9 @@ 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="ODataDisabled" xml:space="preserve">
1182+
<value>This OData endpoint is disabled.</value>
1183+
</data>
11811184
<data name="ODataParametersDisabled" xml:space="preserve">
11821185
<value>The combination of parameters provided to this OData endpoint is not supported.</value>
11831186
</data>

‎tests/NuGetGallery.Facts/Controllers/ODataFeedControllerFactsBase.cs‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,19 @@ protected abstract TController CreateController(
5757
ITelemetryService telemetryService,
5858
IFeatureFlagService featureFlagService);
5959

60-
protected TController CreateTestableODataFeedController(HttpRequestMessage request)
60+
protected TController CreateTestableODataFeedController(
61+
HttpRequestMessage request,
62+
Mock<IFeatureFlagService> featureFlagService)
6163
{
6264
var searchService = new Mock<ISearchService>().Object;
6365
var configurationService = new TestGalleryConfigurationService();
6466
configurationService.Current.SiteRoot = _siteRoot;
6567
var telemetryService = new Mock<ITelemetryService>();
66-
var featureFlagService = new Mock<IFeatureFlagService>();
67-
featureFlagService.Setup(ff => ff.IsODataDatabaseReadOnlyEnabled()).Returns(true);
68+
if (featureFlagService == null)
69+
{
70+
featureFlagService = new Mock<IFeatureFlagService>();
71+
featureFlagService.SetReturnsDefault(true);
72+
}
6873
var readWritePackagesRepositoryMock = new Mock<IEntityRepository<Package>>();
6974

7075
var controller = CreateController(
@@ -146,22 +151,24 @@ protected static ODataQueryContext CreateODataQueryContext<TFeedPackage>()
146151

147152
protected IHttpActionResult GetActionResult<TFeedPackage>(
148153
Func<TController, ODataQueryOptions<TFeedPackage>, IHttpActionResult> controllerAction,
149-
string requestPath)
154+
string requestPath,
155+
Mock<IFeatureFlagService> featureFlagService = null)
150156
where TFeedPackage : class
151157
{
152158
var request = new HttpRequestMessage(HttpMethod.Get, $"{_siteRoot}{requestPath}");
153-
var controller = CreateTestableODataFeedController(request);
159+
var controller = CreateTestableODataFeedController(request, featureFlagService);
154160

155161
return controllerAction(controller, new ODataQueryOptions<TFeedPackage>(CreateODataQueryContext<TFeedPackage>(), request));
156162
}
157163

158164
protected async Task<IHttpActionResult> GetActionResultAsync<TFeedPackage>(
159165
Func<TController, ODataQueryOptions<TFeedPackage>, Task<IHttpActionResult>> asyncControllerAction,
160-
string requestPath)
166+
string requestPath,
167+
Mock<IFeatureFlagService> featureFlagService = null)
161168
where TFeedPackage : class
162169
{
163170
var request = new HttpRequestMessage(HttpMethod.Get, $"{_siteRoot}{requestPath}");
164-
var controller = CreateTestableODataFeedController(request);
171+
var controller = CreateTestableODataFeedController(request, featureFlagService);
165172

166173
return await asyncControllerAction(controller,
167174
new ODataQueryOptions<TFeedPackage>(CreateODataQueryContext<TFeedPackage>(), request));

0 commit comments

Comments
 (0)