Add support for configuring a custom servicebinder using fluent methods (#121)

[Euan-McVie]

@mgravell
As per discussion on #121 i've prepared a PR based on moving the metadata lookup to the service binder.
I've split into 3 commits to make it easier to see the actual implementation vs an update to your example to test the scenario.

Note that I moved the entire GetMetadata as calling a separate GetMetadata(MethodInfo methodInfo) is too late as the MethodInfo for the service type would already be incorrect in my use case.
We could split into seperate GetContractMetadata() or GetServiceMetaData() overrides, but the method ones need the service type for me to resolve before getting the methodInfo so I didn't think it was worth it if I could resolve the ServiceType before calling the base implementation.

If you want to see it with the existing behaviour simply comment out the .WithBinderConfiguration line from the Startup.cs and it will then start ignoring the [Authorize] attribute in the counter service.

Let me know if you need any changes or want the test example changes dropped from the PR.

Thanks
Euan

[Euan-McVie]

Hi @mgravell,
Any chance of a review for what I need to change in order to get this to an acceptable state?
Thanks

[mgravell]

note that this represents a hard break for semver purposes; I wonder whether it is possible to do instead:

// note no "this", so it is no longer an extension method and will not be selected by new builds
public static void AddCodeFirstGrpc(IServiceCollection services) => AddCodeFirstGrpc(services, null);
public static IGrpcServerBuilder AddCodeFirstGrpc(this IServiceCollection services, Action<BinderOptions>? binderOptions = null)
{...}

This avoids a hard break but gives you the same results.

[Euan-McVie]

What is the preference for when we also need the other override for GrpcServiceOptions? It's why I added as an extension instead of an override to AddCodeFirstGrpc
I could either add as an extra field to the BinderOptions(name change to CodeFirstGrpcConfiguration) or would you prefer dual options, Not sure I like adding it as an extra field as it would be added as an Action<GrpcServiceOptions> property as the underlying Configure is passed the action to lazy evaluate, which is slightly different to the eager evaluation of the CodeFirstGrpcConfiguration

//Existing + suggested override
public static void AddCodeFirstGrpc(IServiceCollection services) => AddCodeFirstGrpc(services, null);
public static IGrpcServerBuilder AddCodeFirstGrpc(this IServiceCollection services, Action<GrpcServiceOptions>? configureOptions);
public static IGrpcServerBuilder AddCodeFirstGrpc(this IServiceCollection services, Action<CodeFirstGrpcConfiguration>? grpcConfiguration = null);
// Extra
public static IGrpcServerBuilder AddCodeFirstGrpc(this IServiceCollection services, Action<GrpcServiceOptions>? configureOptions, Action<CodeFirstGrpcConfiguration>? grpcConfiguration);

// Or just switch to non action/option/config style
public static void AddCodeFirstGrpc(IServiceCollection services) => AddCodeFirstGrpc(services, null);
public static IGrpcServerBuilder AddCodeFirstGrpc(this IServiceCollection services, Action<GrpcServiceOptions>? configureOptions);
public static IGrpcServerBuilder AddCodeFirstGrpc(this IServiceCollection services, Action<BinderConfiguration>? binderConfiguration = null);
// Add Create override to to allow not setting a marshaller.
services.AddCodeFirstGrpc(() => BinderConfiguration.Create(new MyServiceBinder));

[Euan-McVie]

Think my preference would be just to get rid of my BinderOptions/CodeFirstGrpcConfiguration
and go with an extra override to Create to remove the null marshaller requirement.

[mgravell]

note that as I understand it (I'm not a DI expect), this not the normal pattern for "option" scenarios; not sure whether that matters much - this will probably work, although it may have atypical timing in the DI pipe

[Euan-McVie]

Makes sense. I'll switch to your AddCodeFirstGrpc suggestion as it fixes both issues.

[Euan-McVie]

It's not an options pattern. but it is fairly common when the config object passed in is not just an options object to be registered, but is actually used inside to configure how things are registered with DI.
I was going to name it BinderConfiguration, but that is what the main one is called.

[Euan-McVie]

Note it doesn't affect the DI pipeline either as everything is still registered/created explicitly as I invoke the BinderOptions() so it is all in place before the serviceprovider is built and requests made of it.

[Euan-McVie]

Or if you were meaning that I should have been wrapping in a "value factory"

builder.Services.TryAddSingleton(() => BinderConfiguration.Create(options.MarshallerFactories, options.ServiceBinder));

then I agree.

[mgravell]

not sure what this is doing or why it exists; PR doesn't mention it; what is the goal here? how is this different to the ambient marshaller setup that can be injected?

[Euan-McVie]

This was just a wrapper as it felt a little odd explicitly listing null for the marshallers
i.e.

config => 
{
  config.ServiceBinder = new MyServiceBinder()
}

instead of just passing in

BinderConfiguration.Create(null, new MyServiceBinder)

[mgravell]

the public API should have intellisense; if this didn't raise a warning, that's a bug that I should fix

[Euan-McVie]

I was building in Debug so it wasn't flagging them locally and I didn't review CI beyond seeing it passed. Maybe TreatWarningsAsErrors should/could be turned on?

[Euan-McVie]

@mgravell - Thanks for the review.
I've commented on a few bits.
Is the general approach for the ServiceBinder changes themselves ok. If so I might move the commit to a seperate PR as the fluent stuff is just nice to have so can continue discussions with yourself without holding up the functional change.

Thanks

[Euan-McVie]

@mgravell - I still intend to tidy this up a bit further as per some of your comments as I would still prefer a cleaner fluent style of setting a custom ServiceBinder, but have moved the ServiceBinder changes themselves to a seperate PR #138.