rebus-org / Rebus.MessageValidation

:x: DEPRECATED :bus: Message validation plugin for Rebus
https://mookid.dk/category/rebus
MIT License
2 stars 1 forks source link

Is this up for grabs? #1

Open CzechsMix opened 4 years ago

CzechsMix commented 4 years ago

I found an old stack overflow message that indicated this was supposed to be for FluentValidation integration. Are you still interested in that?

I wouldn't mind taking a look at it, as I'm planning on trying to wire up FluentValidation in my own projects with rebus.

mookid8000 commented 4 years ago

Oh yeah, sure 🙂 it's not something I'm using myself, but in case anyone is interested it would be absolutely awesome if you would pick it up.

CzechsMix commented 4 years ago

Do you see this more as Rebus validation library, that has a FluentValidation integration, or is this library the FluentValidation integration?

Or another way, do you think it's worth the effort to seperate Rebus validation and FluentValidation, in case somebody wanted to use some other validation method (data annotations?)

As far as the design goes, I was thinking it could be configured to validated on the way out, on the way in or both, and then have some options about what to do when validation fails, with the default being to log and send the message to the error queue.

mookid8000 commented 4 years ago

I think I see this as the FluentValidation integration.

Integrating a message schema validation library should not be that hard, so I don't think it's worth the effort atm to take the extra time to introduce a validation abstraction.

Your design sounds fine to me, although.... when you send a message, and the message could not be validated, would you then send the message to the error queue? Because, in my mind, throwing an exception would be more appropriate at this point in time....

CzechsMix commented 4 years ago

I think throwing exception when you've tried to send an invalid message makes sense, since that's some sort of application error, where as on the receiving end it isn't really that application's fault that they've been sent an invalid message.

I assume targeting latest stable FluentValidation would be fine?

Building on the API from the README:

Configure.With(...)
    .(...)
    .Options(o => {
        o.EnableMessageValidation()
                  .RegisterValidatorsFromAssemblyOf<T>()
                  .InvalidSendHandler<THandler>() // where THandler : IInvalidMessageSendHandler
                  .OnInvalidReceive(ctx => { }); // ctx: InvalidMessageReceivedContext
    })
    .Start();

So there's the option to define either a handling type, or just pass in an action which takes the context (and maybe an injectionist instance?)

mookid8000 commented 4 years ago

I think throwing exception when you've tried to send an invalid message makes sense, since that's some sort of application error, where as on the receiving end it isn't really that application's fault that they've been sent an invalid message.

Exactly. Great way to put it. 🙂

I assume targeting latest stable FluentValidation would be fine?

Sure. 👍

So there's the option to define either a handling type, (...)

Hmmmm.... Rebus actually doesn't readily support specifying a specific type to handle an incoming message, because that is entirely up to the handler activator (usually an IoC container) to resolve it.

The handler activator is an interface with a single method whose signature reads like this:

Task<IEnumerable<IHandleMessages<TMessage>>> GetHandlers<TMessage>(TMessage message, ITransactionContext transactionContext);

(taken from here)

which means that Rebus has no way of resolving a handler of a specific type, as it delegates the responsibility of returning handler instances to the handler activator, with the current message as input.

(....) or just pass in an action which takes the context (and maybe an injectionist instance?)

This would be much easier to support right now. It would be natural for the action to be async, and then maybe pass in the message context too, so the signature would be something like

Func<IMessageContext, object, Task>

(where the object would then be the message)

Injectionist should not be passed in, because it's only meant to be used while configuring and building the bus instance.

CzechsMix commented 4 years ago

That makes sense about just having an invalid handler function. If say I wanted a specific type from ServiceProvider, I can capture it it in the service.AddRebus call and then get the service I'd want to handle the validation error.

If this is FluentValidation specific, would we want

Func<IMessageContext, object, ValidationResult, Task>

instead? or would the ValidationResult object (with the specific error data) be somewhere on the message context?

mookid8000 commented 4 years ago

Oh yeah, you should definitely pass ValidationResult too!

Sendt fra min iPhone

– Mogens Heller Grabe Torsmark 4 8700 Horsens Danmark

+45 29367077 mookid8000@gmail.com

Den 27. nov. 2019 kl. 22.55 skrev Sam Ferree notifications@github.com:

 That makes sense about just having an invalid handler function. If say I wanted a specific type from ServiceProvider, I can capture it it in the service.AddRebus call and then get the service I'd want to handle the validation error.

If this is FluentValidation specific, would we want

Func<IMessageContext, object, ValidationResult, Task> instead? or would the ValidationResult object (with the specific error data) be somewhere on the message context?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

CzechsMix commented 4 years ago

In my fork, I just pushed up a rough pass at the MessageValidationConfigurationExtensions.cs and a OutgoingMessageValidator.cs as and IOutgoingStep in the pipeline. Also modified the ItWorks.cs test case to reflect the configuration API

When you get a chance I'd appreciate if you take a look and make sure I'm on the right track.

mookid8000 commented 4 years ago

It definitely looks like it can work. I think the outgoing step probably looks as it should, but you obviously need to hook it in somehow.

You would usually do that by decorating the outgoing message pipeline with a "step injector", e.g. via the OptionsConfigurer like this:

public static void EnableMessageValidation(this OptionsConfigurer configurer, Action<MessageValidationConfigurer> configure)
{
    var validationConfig = new MessageValidationConfigurer();

    configure(validationConfig);

    configurer.Decorate<IPipeline>(c => {
        var step = new OutgoingMessageValidator();
        var pipeline = c.Get<IPipeline>();

        return new PipelineStepInjector(pipeline)
            .OnSend(step, PipelineRelativePosition.Before, typeof(SerializeOutgoingMessageStep));
});
}

and then, if you add the StepDocumentationAttribute to your step, it will show up with its documentation text when people log the contents of their message pipelines:

Configure.With(...)
    .(...)
    .Options(o => o.LogPipeline(verbose: true))
    .Start();
CzechsMix commented 4 years ago

Hit something of a wall with the implementation.

I can scan and find types which implement AbstractValidator<> to build the validator dictionary, but I run into a wall trying to register them. See below:

public MessageValidationConfigurer MapValidatorsFromAssemblyOf<TValidator>()
{
  var assembly = typeof(TValidator).Assembly;
  foreach(Type validatorType in assembly.GetTypes())
  {
    if (validatorType.IsSubclassOf(typeof(AbstractValidator<>)))
    {
      var messageType = validatorType.BaseType.GenericTypeArguments[0];

      // create and register an instance of validatorType here.
      // hope for the best and call validatorType.GetConstructor().Invoke()?
      // or try to register it directly in case it has dependencies needed from 
      // injection context? 
    }
  }
  return this;
}

all the references I can find to get or register something require the generic parameter "ctx.Get<T>()" and not what I have readily available, an instance of the Type object. "ctx.Get(validatorType)"

mookid8000 commented 4 years ago

I'm not sure I understand the problem..... is it because you want to provide constructor injection into the validators, and you want Rebus's container to provide that?

Because then I am not sure I can see any good reason to do so. Could be my limited imagination though 🙂

CzechsMix commented 4 years ago

I think the vast majority of cases, Validators have no dependencies, So I think it'd be okay to leave it until there's a good reason to support it.

but even once I have the validator, I'm not sure how i'd register it using the generic method, when I have an insance of the actual type.

Perhaps this assembly scanning method isn't worth the hassle, What would be straightforward is somthing like this

private readonly Dictionary<Type, Func<object, ValidationResult> validators> _validators;
public MessageValidationConfigurer RegisterValidator<TMessage>(AbstractValidator<TMessage> validator)
{
  _validators[typeof(TMessage)] = message => validator.Validate((TMessage)message)
  return this;
}

The initial configuration would be more verbose, in that you'd need to manually add each validator, but this would work for my purposes, and could always be changed later if it gets out of hand.

mookid8000 commented 4 years ago

Good point. It should be fairly easy to extend later on, but I think it makes sense that the underlying API is simply one that accepts a validator instance, and then you start out by exposing that.

CzechsMix commented 4 years ago

Do I want to Decorate IPipeline by returning a PipelineStepInjector or do I want to get the injector call the OnSend and OnReceive methods of it?

I don't see a constructor for PipelineStepInjector that has three arguments.

Also I'm getting a weird issue that maybe you've seen before: When trying to mark MessageValidationException as an IFailFastException, it can't find the type. I bumped the Rebus version to latest stable, and am in the Rebus.Exceptions namespace, but it can't seem to find it.

mookid8000 commented 4 years ago

Oh it's because I wrote the code from memory, and I messed it up 😁 I'll correct it above.

The IFailFastException interface is declared here: IFailFastException.cs

It was introduced in Rebus 5.4.0, and so it should be in all subsequent versions... although, as I'm writing this, I'm starting to doubt whether a prerelease of 6 was actually built after the fail fast feature was ported forward.... I just released Rebus 6.0.0-b20 – I'm sure it has IFailFastException 🙂

CzechsMix commented 4 years ago

I managed to get that resolved, had to bump netstandard support from 1.3 to 2.0

For the default InvalidReceiveAcion, what's the easiest way to get a hold of the errorQueueAddress? Or really just the easiest way to send a message to the error queue?

mookid8000 commented 4 years ago

Or really just the easiest way to send a message to the error queue?

Throw an exception that has been marked as a "fail-fast exception".

Rebus 6 has an extension that works like this:

Configure.With(...)
    .Transport(t => (...))
    .Options(o => o.FailFastOn<MyOwnException>())
    .Start();

Underneath the covers, the extension method FailFastOn will install a decorator of the IFailFastChecker, which I think would be the nicest way for you to ensure that your validation exception causes Rebus to fail fast.

You can see here how the decorator is implemented and how it is installed.

CzechsMix commented 4 years ago

Is there anything I would want to do differently such that the default behavior for attempting to send an invalid message causes a noisy exception and doesn't send the message to the error queue, but receiving a message that is invalid "fails gracefully" in that it just logs and sends to error queue?

I think we want to "Fail fast" and not retry in any case.

mookid8000 commented 4 years ago

There's actually another option... you can let your pipeline step have Rebus' IErrorHandler injected, and then just use that to forward the message to the dead-letter queue(*)

This would actually be quite elegant. 🙂


(*) that is what happens by default, but e.g. if you're using Fleet Manager, then the IErrorHandler implementation used will actually store the message in Fleet Manager's database.

skwasjer commented 4 years ago

I made smth like this for my customer (and was unaware of this repo) and uses a pattern similar to IFailed<> for inbound messages and wraps them in a IValidationFailed<> wrapper. It works really well, only executes relevant validator (per message type), and allows new handlers to be created for different use cases. Some convenience defaults can be easily made (aka. default to dead-letter or ignore message if no handler registered). I will see if I can share it, but if not, it is not much effort to redo it.

skwasjer commented 4 years ago

So it was a bit problematic with legal and all, so I just rewrote it over the weekend but it was easy enough with the original work in mind. It is a new repo/library though (Rebus.FluentValidation) instead of fork, as this was just easier for me to work on - fitting my own CI templates - if you don't mind, and allows me to maintain it easier going forward. Let me know what you think ;)

CzechsMix commented 4 years ago

My son was born recently so I haven't had as much time to work on this, but I was about to pick it back up.

But, If that repo does the job, I have no problem just going with that. No reason to fix what isn't broken, or duplicate work.

If @mookid8000 wants to just use that as the Rebus solution for fluent validation, I've got no complaints.