Giter Club home page Giter Club logo

Comments (16)

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024 1

Your test passes now, should we consider the issue resolved?

from eventuous.

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024 1

0.15.0-beta.4.19 is on MyGet

from eventuous.

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024 1

I am closing it as the tests you added are passing. Feel free to reopen it, or open a new one if something is still not working.

from eventuous.

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024

Hm, it should not be mapping object, it must have a proper type. I will try to reproduce it.

from eventuous.

iappwebdev avatar iappwebdev commented on September 25, 2024

Hm, it should not be mapping object, it must have a proper type. I will try to reproduce it.

I could reproduce it and can give the fix to this, I'll create a pull request.

The main issue is still there, registered and recognised handlers are not found on execution.

from eventuous.

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024

Maybe we can start by adding a test with this scenario, so the issue would be clearly illustrated.

from eventuous.

iappwebdev avatar iappwebdev commented on September 25, 2024

Maybe we can start by adding a test with this scenario, so the issue would be clearly illustrated.

I just did this today and found the problem. In file HttpCommandMapping.cs: when using app.MapDiscoveredCommands<TAggregate>(), the handlers are registered by using a generic made version of Map:

void MapAssemblyCommands(Assembly assembly) {
    var decoratedTypes = assembly.DefinedTypes.Where(
        x => x.IsClass && x.CustomAttributes.Any(a => a.AttributeType == attributeType)
    );

    var method = typeof(RouteBuilderExtensions).GetMethod(nameof(Map), BindingFlags.Static | BindingFlags.NonPublic)!;

    foreach (var type in decoratedTypes) {
        var attr = type.GetAttribute<HttpCommandAttribute>()!;

        if (attr.AggregateType != null && attr.AggregateType != typeof(TAggregate))
            throw new InvalidOperationException(
                $"Command aggregate is {attr.AggregateType.Name} but expected to be {typeof(TAggregate).Name}"
            );

        var genericMethod = method.MakeGenericMethod(typeof(TAggregate), type, type);
        genericMethod.Invoke(null, new object?[] { builder, attr.Route, null, attr.PolicyName });
    }
}

When calling app.MapDiscoveredCommands(), handlers are registered by a non generic version

void LocalMap(Type aggregateType, Type type, string? route, string? policyName) {
    var appServiceBase = typeof(ICommandService<>);
    var appServiceType = appServiceBase.MakeGenericType(aggregateType);

    var routeBuilder = builder
        .MapPost(
            GetRoute(type, route),
            async Task<IResult> (HttpContext context) => {
                var cmd = await context.Request.ReadFromJsonAsync(type, context.RequestAborted);

                if (cmd == null) throw new InvalidOperationException("Failed to deserialize the command");

                if (context.RequestServices.GetRequiredService(appServiceType) is not ICommandService
                    service) throw new InvalidOperationException("Unable to resolve the application service");

                var result = await service.Handle(cmd, context.RequestAborted);

                return result.AsResult();
            }
        )

Here, deeper at await service.Handle(cmd, context.RequestAborted); the generic CommandType information gets lost.


Testing

I introduced a new (duplicated) command for BookRoom with a wrapper class annotated with AggregateCommands in BookService:

[AggregateCommands<Booking>]
static class NestedCommands {
    [HttpCommand(Route = "nested-book")]
    internal record NestedBookRoom(string BookingId, string RoomId, LocalDate CheckIn, LocalDate CheckOut, float Price, string? GuestId);
}

Then I extended HttpCommandTests to test MapDiscoveredCommands()

[Fact]
public void RegisterAggregatesCommands() {
    var builder = WebApplication.CreateBuilder();

    using var app = builder.Build();

    var b = app.MapDiscoveredCommands(typeof(NestedCommands).Assembly);

    b.DataSources.First().Endpoints[0].DisplayName.Should().Be("HTTP: POST nested-book");
}

The test passed so registering commands using MapDiscoveredCommands() works correctly.


Next I introducded a test for executing the registered command:

[Fact]
public async Task MapDiscoveredCommand() {
    using var fixture = new ServerFixture(
        output,
        _ => { },
        app => app.MapDiscoveredCommands(typeof(NestedCommands).Assembly)
    );

    using var client = fixture.GetClient();

    var cmd = fixture.GetNestedBookRoom();

    var response = await client.PostJsonAsync("/nested-book", cmd);
    response.Should().Be(HttpStatusCode.OK);
}

The test failed with eventuous.application - EventId: [1], EventName: [CommandHandlerNotFound], Message: [Handler not found for command: 'NestedBookRoom']

Possible solution 1

I took a close look on how handlers are registered and how they are found. In CommandService, handlers are registered by AddHandlerUntyped()

void BuildHandlers() {
    lock (_handlersLock) {
        foreach (var commandType in _builders.Keys) {
            var builder = _builders[commandType];
            var handler = builder.Build();
            _handlers.AddHandlerUntyped(commandType, handler);
        }

        _initialized = true;
    }
}

Command are then searched by using

public async Task<Result<TState>> Handle<TCommand>(TCommand command, CancellationToken cancellationToken) where TCommand : class {
    if (!_initialized) BuildHandlers();

    if (!_handlers.TryGet<TCommand>(out var registeredHandler)) {
        Log.CommandHandlerNotFound<TCommand>();
...

As we saw before, the type information gets lost and TCommand is of type object when using app.MapDiscoveredCommands(). So I extended CommandHandlersMap by an additional method TryGetUntyped():

static readonly MethodInfo TryGetInternalMethod =
    typeof(HandlersMap<TAggregate, TId>).GetMethod(nameof(TryGetInternal), BindingFlags.NonPublic | BindingFlags.Instance)!;

...

internal bool TryGetUntyped(Type command, [NotNullWhen(true)] out RegisteredHandler<TAggregate, TId>? handler) {
    var parameters = new object?[] { null };
    bool tryGet = (bool)TryGetInternalMethod.MakeGenericMethod(command).Invoke(this, parameters)!;
    handler = tryGet ? (RegisteredHandler<TAggregate, TId>?)parameters[0] : null;
    return tryGet;

Now the handler was found correctly.

As I looked further in the method I realized that there are further places to adapt:

Log.CommandHandlerNotFound<TCommand>();
...
Log.CommandHandled<TCommand>();
...
Log.ErrorHandlingCommand<TCommand>(e);
...
return new ErrorResult<TState>($"Error handling command {typeof(TCommand).Name}", e);

I introduced the corresponding *Untyped methods in Eventuous.Diagnostics.ApplicationEventSource.

    [NonEvent]
    public void CommandHandlerNotFound<TCommand>() => CommandHandlerNotFound(typeof(TCommand).Name);

    [NonEvent]
    public void CommandHandlerNotFoundUntyped(Type commandType) => CommandHandlerNotFound(commandType.Name);

    [NonEvent]
    public void ErrorHandlingCommand<TCommand>(Exception e) => ErrorHandlingCommand(typeof(TCommand).Name, e.ToString());

    [NonEvent]
    public void ErrorHandlingCommandUntyped(Type commandType, Exception e) => ErrorHandlingCommand(commandType.Name, e.ToString());

    [NonEvent]
    public void CommandHandled<TCommand>() {
        if (IsEnabled(EventLevel.Verbose, EventKeywords.All)) CommandHandled(typeof(TCommand).Name);
    }

    [NonEvent]
    public void CommandHandledUntyped(Type commandType) {
        if (IsEnabled(EventLevel.Verbose, EventKeywords.All)) CommandHandled(commandType.Name);
    }

If all this are acceptable changes, the FunctionalCommandService should be adapted as well. I can create a pull request if you approve my thoughts

Possible solution 2 (?)

I did not succeed to change the LocalMap method such that the command type information gets passed to the Handle method. Maybe you have an idea?

from eventuous.

iappwebdev avatar iappwebdev commented on September 25, 2024

IMHO there should be a better solution instead of creating *Untyped methods for every generic method with TCommand...

from eventuous.

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024

Basically, the issue has nothing to do with multiple aggregates, or any sort of edge case. Discovered commands mapping got broken when I changed the Command Service to use the generic type map instead of a dictionary. As commands discovery didn't have any tests, I overlooked the consequences as Handle<T> must have the specific T for the type map to work.

Right now, I fixed it by creating a generic call and caching it for consecutive invocations. Ideally, of course, it should be solved by creating a code generator, which would also avoid calling MapDiscoveredCommands(Assembly) as it would just create code to map discovered commands.

from eventuous.

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024

IMHO there should be a better solution instead of creating *Untyped methods for every generic method with TCommand...

The code doesn't create untyped methods, the AddHandlerUntyped itself has no generic argument, that's why it has such a name.

from eventuous.

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024

Ok, it should work with the latest alpha package

from eventuous.

iappwebdev avatar iappwebdev commented on September 25, 2024

Thank you for checking my thought and for finding the correct cause of the problem. I will check my app with the latest alpha package and let you know if it worked.

from eventuous.

iappwebdev avatar iappwebdev commented on September 25, 2024

Ok, it should work with the latest alpha package

I'm a bit confused, does latest alpha package means 0.14.1-alpha.0.43? I have currently installed 0.15.0-beta.5...

image

from eventuous.

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024

Sorry, I think the publish job has failed because the tests crashed in the pipeline. Checking now.

from eventuous.

iappwebdev avatar iappwebdev commented on September 25, 2024

0.15.0-beta.4.19 is on MyGet

You mean 0.15.0-beta.6?

from eventuous.

alexeyzimarev avatar alexeyzimarev commented on September 25, 2024

I released beta.6 right after. It's a tagged beta and it went to NuGet. Other preview packages are only on MyGet.

from eventuous.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.