Skip to content

Add IFeatureService & Related Utilities #37

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

Merged
merged 11 commits into from
Oct 25, 2024
Original file line number Diff line number Diff line change
@@ -1,7 +1,22 @@
using System.Text.Json.Nodes;
using Bitwarden.Extensions.Hosting.Features;

var builder = WebApplication.CreateBuilder(args);

builder.UseBitwardenDefaults();

var app = builder.Build();

app.UseRouting();

app.UseFeatureChecks();

app.MapGet("/", (IConfiguration config) => ((IConfigurationRoot)config).GetDebugView());

app.MapGet("/requires-feature", (IFeatureService featureService) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐ŸŒฑ Should this example code be expected to declare known flags?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's declaring its known flags in appsettings.Development.json.

{
return featureService.GetAll();
})
.RequireFeature("feature-one");

app.Run();
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,13 @@
"Default": "Information",
"Microsoft.AspNetCore": "Warning"
}
},
"Features": {
"FlagValues": {
"feature-one": true,
"feature-two": 1,
"feature-three": "my-value"
},
"KnownFlags": ["feature-one", "feature-two"]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="LaunchDarkly.ServerSdk" Version="8.5.2" />
<PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.1" />
<PackageReference Include="OpenTelemetry.Exporter.OpenTelemetryProtocol" Version="1.9.0" />
<PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="1.9.0" />
Expand All @@ -43,5 +44,9 @@
<ItemGroup>
<None Include=".\README.md" Pack="true" PackagePath="\" />
</ItemGroup>


<ItemGroup>
<InternalsVisibleTo Include="Bitwarden.Extensions.Hosting.Tests" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using Bitwarden.Extensions.Hosting.Features;

namespace Microsoft.AspNetCore.Builder;

/// <summary>
/// Feature extension methods for <see cref="IApplicationBuilder"/>.
/// </summary>
public static class FeatureApplicationBuilderExtensions
{
/// <summary>
/// Adds the <see cref="FeatureCheckMiddleware"/> to the specified <see cref="IApplicationBuilder"/>, which enabled feature check capabilities.
/// <para>
/// This call must take place between <c>app.UseRouting()</c> and <c>app.UseEndpoints(...)</c> for middleware to function properly.
/// </para>
/// </summary>
/// <param name="app">The <see cref="IApplicationBuilder"/> to add the middleware to.</param>
/// <returns>A reference to <paramref name="app"/> after the operation has completed.</returns>
public static IApplicationBuilder UseFeatureChecks(this IApplicationBuilder app)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ on including "flag" in the language here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, I changed it. Do you think we should also name the namespace/folder FeatureFlags instead of Features?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am flip-flopping a bit in my head over this. We have "feature checks" and we "require features", but we use feature flags to do that. This may be overcomplicating it and we just call everything one or another. Would like to hear others' opinions.

{
ArgumentNullException.ThrowIfNull(app);

// This would be a good time to make sure that IFeatureService is registered but it is a scoped service
// and I don't think creating a scope is worth it for that. If we think this is a problem we can add another
// marker interface that is registered as a singleton and validate that it exists here.

app.UseMiddleware<FeatureCheckMiddleware>();
return app;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;

namespace Bitwarden.Extensions.Hosting.Features;

internal sealed class FeatureCheckMiddleware
{
private readonly RequestDelegate _next;
private readonly IHostEnvironment _hostEnvironment;
private readonly IProblemDetailsService _problemDetailsService;
private readonly ILogger<FeatureCheckMiddleware> _logger;

public FeatureCheckMiddleware(
RequestDelegate next,
IHostEnvironment hostEnvironment,
IProblemDetailsService problemDetailsService,
ILogger<FeatureCheckMiddleware> logger)
{
ArgumentNullException.ThrowIfNull(next);
ArgumentNullException.ThrowIfNull(hostEnvironment);
ArgumentNullException.ThrowIfNull(problemDetailsService);
ArgumentNullException.ThrowIfNull(logger);

_next = next;
_hostEnvironment = hostEnvironment;
_problemDetailsService = problemDetailsService;
_logger = logger;
}

public Task Invoke(HttpContext context, IFeatureService featureService)
{
// This middleware is expected to be placed after `UseRouting()` which will fill in this endpoint
var endpoint = context.GetEndpoint();

if (endpoint == null)
{
_logger.LogNoEndpointWarning();
return _next(context);
}

var featureMetadatas = endpoint.Metadata.GetOrderedMetadata<IFeatureMetadata>();

foreach (var featureMetadata in featureMetadatas)
{
if (!featureMetadata.FeatureCheck(featureService))
{
// Do not execute more of the pipeline, return early.
return HandleFailedFeatureCheck(context, featureMetadata);
}

// Continue checking
}

// Either there were no feature checks, or none were failed. Continue on in the pipeline.
return _next(context);
}

private async Task HandleFailedFeatureCheck(HttpContext context, IFeatureMetadata failedFeature)
{
if (_logger.IsEnabled(LogLevel.Debug))
{

Check warning on line 63 in extensions/Bitwarden.Extensions.Hosting/src/Features/FeatureCheckMiddleware.cs

View check run for this annotation

Codecov / codecov/patch

extensions/Bitwarden.Extensions.Hosting/src/Features/FeatureCheckMiddleware.cs#L63

Added line #L63 was not covered by tests
_logger.LogFailedFeatureCheck(failedFeature.ToString() ?? "Unknown Check");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ“ How could failedFeature return a null ToString()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically ToString is marked as a nullable return: https://github.com/dotnet/runtime/blob/9ecef8c2d709b88b959d9d6a00ad62a8f72a094f/src/libraries/System.Private.CoreLib/src/System/Object.cs#L39

I kinda doubt we will run into that scenario though, I'm happy to ! away the compiler warning instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, seems cleaner to me. I like having less hardcoded strings in all this.

}

Check warning on line 65 in extensions/Bitwarden.Extensions.Hosting/src/Features/FeatureCheckMiddleware.cs

View check run for this annotation

Codecov / codecov/patch

extensions/Bitwarden.Extensions.Hosting/src/Features/FeatureCheckMiddleware.cs#L65

Added line #L65 was not covered by tests

context.Response.StatusCode = StatusCodes.Status404NotFound;

var problemDetails = new ProblemDetails();
problemDetails.Title = "Resource not found.";
problemDetails.Status = StatusCodes.Status404NotFound;

// Message added for legacy reasons. We should start preferring title/detail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ“ Legacy? So Message isn't common anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, a lot of things in ASP.NET Core will return a ProblemDetails object unless we specifically go and and change it. So my preference would be to also make that be our return for problems. That way we can start on a unified error format.

It also sends back application/problem+json as the content type so you can introspect on that more on the client side to trust that if you got a bad status code and the content type indicates problem details, you can read the respond body like it's a problem details object and then return a more high quality error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I like that. Do we just get rid of the message then? I'd like to not carry in debt and just expect callers to adjust when they adopt this library.

problemDetails.Extensions["Message"] = "Resource not found.";

// Follow ProblemDetails output type? Would need clients update
if (_hostEnvironment.IsDevelopment())
{
// Add extra information
problemDetails.Detail = $"Feature check failed: {failedFeature}";
}

// We don't really care if this fails, we will return the 404 no matter what.
await _problemDetailsService.TryWriteAsync(new ProblemDetailsContext
{
HttpContext = context,
ProblemDetails = problemDetails,
// TODO: Add metadata?
});
}


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
using Bitwarden.Extensions.Hosting.Features;

namespace Microsoft.AspNetCore.Builder;

/// <summary>
/// Feature extension methods for <see cref="IEndpointConventionBuilder"/>.
/// </summary>
public static class FeatureEndpointConventionBuilderExtensions
{
/// <summary>
/// Adds a feature check for the given feature to the endpoint(s).
/// </summary>
/// <param name="builder">The endpoint convention builder.</param>
/// <param name="featureNameKey"></param>
/// <returns></returns>
public static TBuilder RequireFeature<TBuilder>(this TBuilder builder, string featureNameKey)
where TBuilder : IEndpointConventionBuilder
{
ArgumentNullException.ThrowIfNull(builder);
ArgumentNullException.ThrowIfNull(featureNameKey);

builder.Add(endpointBuilder =>
{
endpointBuilder.Metadata.Add(new RequireFeatureAttribute(featureNameKey));
});

return builder;
}

/// <summary>
/// Adds a feature check with the specified check to the endpoint(s).
/// </summary>
/// <param name="builder">The endpoint convention builder.</param>
/// <param name="featureCheck"></param>
/// <returns></returns>
public static TBuilder RequireFeature<TBuilder>(this TBuilder builder, Func<IFeatureService, bool> featureCheck)
where TBuilder : IEndpointConventionBuilder
{
ArgumentNullException.ThrowIfNull(builder);
ArgumentNullException.ThrowIfNull(featureCheck);

builder.Add(endpointBuilder =>
{
endpointBuilder.Metadata.Add(new RequireFeatureAttribute(featureCheck));
});

return builder;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
using System.Text.Json.Nodes;

namespace Bitwarden.Extensions.Hosting.Features;

/// <summary>
/// A collection of Launch Darkly specific options.
/// </summary>
public sealed class LaunchDarklyOptions
{
/// <summary>
/// The SdkKey to be used for retrieving feature flag values from Launch Darkly.
/// </summary>
public string? SdkKey { get; set; }
}

/// <summary>
/// A set of options for features.
/// </summary>
public sealed class FeatureFlagOptions
{
/// <summary>
/// All the flags known to this instance, this is used to enumerable values in <see cref="IFeatureService.GetAll()"/>.
/// </summary>
public HashSet<string> KnownFlags { get; set; } = new HashSet<string>();

/// <summary>
/// Flags and flag values to include in the feature flag data source.
/// </summary>
public Dictionary<string, string> FlagValues { get; set; } = new Dictionary<string, string>();

/// <summary>
/// Launch Darkly specific options.
/// </summary>
public LaunchDarklyOptions LaunchDarkly { get; set; } = new LaunchDarklyOptions();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
using System.Text.Json.Nodes;
using Bitwarden.Extensions.Hosting.Features;

namespace Microsoft.Extensions.DependencyInjection;

/// <summary>
/// Extensions for features on <see cref="IServiceCollection"/>.
/// </summary>
public static class ServiceCollectionExtensions
{
/// <summary>
/// Adds known feature flags to the <see cref="FeatureFlagOptions"/>. This makes these flags
/// show up in <see cref="IFeatureService.GetAll()"/>.
/// </summary>
/// <param name="services">The service collection to customize the options on.</param>
/// <param name="knownFlags">The flags to add to the known flags list.</param>
/// <returns>The <see cref="IServiceCollection"/> to chain additional calls.</returns>
public static IServiceCollection AddKnownFeatureFlags(this IServiceCollection services, IEnumerable<string> knownFlags)
{
ArgumentNullException.ThrowIfNull(services);

services.Configure<FeatureFlagOptions>(options =>
{
foreach (var flag in knownFlags)
{
options.KnownFlags.Add(flag);
}
});

return services;
}

/// <summary>
/// Adds feature flags and their values to the <see cref="FeatureFlagOptions"/>.
/// </summary>
/// <param name="services">The service collection to customize the options on.</param>
/// <param name="flagValues">The flags to add to the flag values dictionary.</param>
/// <returns>The <see cref="IServiceCollection"/> to chain additional calls. </returns>
public static IServiceCollection AddFeatureFlagValues(
this IServiceCollection services,
IEnumerable<KeyValuePair<string, string>> flagValues)
{
ArgumentNullException.ThrowIfNull(services);

services.Configure<FeatureFlagOptions>(options =>
{
foreach (var flag in flagValues)
{
options.FlagValues[flag.Key] = flag.Value;
}
});

return services;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
namespace Bitwarden.Extensions.Hosting.Features;

internal interface IFeatureMetadata
{
/// <summary>
/// A method to run to check if the feature is enabled.
/// </summary>
Func<IFeatureService, bool> FeatureCheck { get; set; }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
using System.Text.Json.Nodes;

namespace Bitwarden.Extensions.Hosting.Features;

/// <summary>
/// Checks feature status for the current request.
/// </summary>
public interface IFeatureService
{
/// <summary>
/// Checks whether a given feature is enabled.
/// </summary>
/// <param name="key">The key of the feature to check.</param>
/// <param name="defaultValue">The default value for the feature.</param>
/// <returns>True if the feature is enabled, otherwise false.</returns>
bool IsEnabled(string key, bool defaultValue = false);

/// <summary>
/// Gets the integer variation of a feature.
/// </summary>
/// <param name="key">The key of the feature to check.</param>
/// <param name="defaultValue">The default value for the feature.</param>
/// <returns>The feature variation value.</returns>
int GetIntVariation(string key, int defaultValue = 0);

/// <summary>
/// Gets the string variation of a feature.
/// </summary>
/// <param name="key">The key of the feature to check.</param>
/// <param name="defaultValue">The default value for the feature.</param>
/// <returns>The feature variation value.</returns>
string? GetStringVariation(string key, string? defaultValue = null);

/// <summary>
/// Gets all feature values.
/// </summary>
/// <returns>A dictionary of feature keys and their values.</returns>
IReadOnlyDictionary<string, JsonValue> GetAll();
}
Loading
Loading