Skip to content
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

Allow to define which interfaces are important for the dapr actor host #1341

Open
paule96 opened this issue Aug 21, 2024 · 4 comments · May be fixed by #1350
Open

Allow to define which interfaces are important for the dapr actor host #1341

paule96 opened this issue Aug 21, 2024 · 4 comments · May be fixed by #1350
Assignees
Labels
help wanted Extra attention is needed

Comments

@paule96
Copy link

paule96 commented Aug 21, 2024

Describe the proposal

Currently, if you do something like:

builder.Services.AddActors(options =>
{
    options.Actors.RegisterActor<MyActor>(nameof(MyActor));
});

The complete interface hierarchy needs to be compatible with actors. So if you have something like:

public interface IInternalMyActor : IMyActor
{
    void SomeInternalMethod()
}

public interface IMyActor: IActor
{
    Task SomeMethod()
}

public class MyActor: Actor, IInternalMyActor{
   // TODO: implementation
}

If you try to start this actor host you will get the following error message:

: 'Method 'SomeInternalMethod' of actor interface 'MySample.IInternalMyActor ' does not return Task or Task<>. The actor interface methods must be async and must return either Task or Task<>. (Parameter 'actorInterfaceType')'

Error happens at InterfaceDescription:224.

It would be nice if we would be able to provide at the actor registration another generic parameter which interface should be used to access the actor. So like:

builder.Services.AddActors(options =>
{
    options.Actors.RegisterActor<IMyActor, MyActor>(nameof(MyActor));
});

That would make it possible to have a quite complicated structure of interface on an actor. Also it would make it alot easier to use default interface methods, because you can have an more advanced IActor interface in your project that just map members from the class Actor, so you can use it in your default interface methods.

@philliphoff
Copy link
Collaborator

@paule96 What would the intent be for having IInternalMyActor inherit from IMyActor as it wouldn't be invoke-able outside of the application process?

@paule96
Copy link
Author

paule96 commented Aug 24, 2024

Yes it wouldn't be able to invoke these methods outside of the same actor instance. But I also like to use interfaces to force one way of implementation for a group of types.

And with the new capabilities of Default implementations of interfaces in C#, it makes even more sense to me.

    /// <summary>
    /// Base interface that should be used on all actors 
    /// that represent an XYZ element
    /// </summary>
    public interface IInternalXYZActor : IXYZActor
    {
        /// <summary>
        /// The reference key of the <see cref="InstanceInfo"/> 
        /// in the <see cref="StateManager"/>.
        /// </summary>
        const string selfElementStateKey = "self";

        /// <summary>
        /// Get the state of the actor
        /// </summary>
        IActorStateManager StateManager { get; }

        /// <summary>
        /// Get the host of this actor within the actor runtime
        /// </summary>
        ActorHost Host { get; }
        /// <summary>
        /// Gets the identity of this actor.
        /// </summary>
        /// <value>The <see cref="ActorId"/> for the actor.</value>
        ActorId Id { get; }

        /// <summary>
        /// Get the essential information about this actor
        /// </summary>
        /// <returns></returns>
        /// <exception cref="NotSupportedException">If the actor is not initialized</exception>
        async Task<InstanceInfo> IBpmnActor.GetInfo()
        {
            return await InternalGetInfo() ??
                throw new NotSupportedException("The actor needs to be initialized.");
        }

        /// <summary>
        /// Get the essential information about this actor,
        /// if the actor is not initialized it returns 'null'
        /// </summary>
        /// <returns></returns>
        protected async Task<InstanceInfo?> InternalGetInfo()
        {
            var result = await this.StateManager.TryGetStateAsync<InstanceInfo>(selfElementStateKey);
            return result.HasValue ? result.Value : null;
        }
    }

and the "actor interface" for this looks like:

/// <summary>
/// Base interface that should be used on all actors 
/// that represent an XYZelement
/// </summary>
public interface IXYZActor : IActor
{
    /// <summary>
    /// Get the essential information about this actor
    /// </summary>
    /// <returns></returns>
    /// <exception cref="NotSupportedException">If the actor is not initialized</exception>
    Task<InstanceInfo> GetInfo();
}

With this, it is possible to recommend some implementation and at the same time write on that interface level tests for it.

Ofc that is just what I use it for. Doesn't mean that this is the only usecase.

Right now I found a workaround by writing my own extension method like that:

public static void RegisterActor<TActor, TActorInterface>(
    this ActorRegistrationCollection collection,
    string actorTypeName,
    Action<ActorRegistration>? configure = null)
        where TActor : Actor
{
    collection.RegisterActor<TActor>(actorTypeName, o =>
    {
        configure?.Invoke(o);
        var typeOptionsType = o.Type.GetType();
        var interfaceProp = typeOptionsType.GetProperty(nameof(ActorTypeInformation.InterfaceTypes));
        interfaceProp!.SetValue(o.Type, new List<Type>(1) { typeof(TActorInterface) });
    });
}

@philliphoff philliphoff added help wanted Extra attention is needed labels Sep 11, 2024
@philliphoff
Copy link
Collaborator

Right now I found a workaround by writing my own extension method like that:

@paule96 Perhaps it's just a matter of exposing some new registration methods that are more targeted in the actor interfaces they register. This could be a good opportunity for 3rd party contribution; I'll mark it as such.

@siri-varma
Copy link

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants