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.

Sign up for GitHub

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

Jump to bottom

Adds support for providing Actor type name during runtime. #680

Closed
wants to merge 1 commit into from
Closed

Adds support for providing Actor type name during runtime. #680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/Dapr.Actors/Runtime/ActorRegistrationCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// ------------------------------------------------------------
// ------------------------------------------------------------
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.
// ------------------------------------------------------------
Expand Down Expand Up @@ -36,5 +36,21 @@ public void RegisterActor<TActor>(Action<ActorRegistration> configure = null)
configure?.Invoke(registration);
this.Add(registration);
}

/// <summary>
/// Registers an actor type in the collection.
/// </summary>
/// <typeparam name="TActor">Type of actor.</typeparam>
/// <param name="actorTypeName">The name of the actor type represented by the actor.</param>
/// <param name="configure">An optional delegate used to configure the actor registration.</param>
/// <remarks>The value of <paramref name="actorTypeName"/> will have precedence over the default actor type name derived from the actor implementation type or any type name set via <see cref="ActorAttribute"/>.</remarks>
public void RegisterActor<TActor>(string actorTypeName, Action<ActorRegistration> configure = null)
where TActor : Actor
{
var actorTypeInfo = ActorTypeInformation.Get(typeof(TActor), actorTypeName);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's going to take me a bit to review this and rationalize the change. I have to check whether there are other places where we call ActorTypeInformation.Get.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. To clarify, this should be a non-breaking change. I added an overload for ActorTypeInformation.Get which takes an string parameter for specifying the actor type name.

public static ActorTypeInformation Get(Type actorType)

The above still exists, but will call the below with actorTypeName: null.

public static ActorTypeInformation Get(Type actorType, string actorTypeName)

And then in the above method I've just modified it to check is actorTypeName is provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. To clarify, this should be a non-breaking change. I added an overload for ActorTypeInformation.Get which takes an string parameter for specifying the actor type name.

What I'm concerned about isn't the addition of a new method it's a the introduction of a 'split brain' problem. We need to make sure it's not possible for two parts of the system to disagree about the type name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please do the following:

  • On ActorTypeInformation - mark TryGet(Type) and Get(Type) as [Obsolete]- we want to make sure we don't have code calling those since they don't accept a type name override.
  • On ActorHost we need another overload of CreateForTest that accepts a type name.
  • Update the remaining references to Get(Type) to call GetType(Type, null) - these will be unit tests

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this here bjorkstromm@e1b62c9

But since this PR was unintentionally closed, changes won't appear here. Could you please reopen, or should I open a new PR?

var registration = new ActorRegistration(actorTypeInfo);
configure?.Invoke(registration);
this.Add(registration);
}
}
}
21 changes: 19 additions & 2 deletions src/Dapr.Actors/Runtime/ActorTypeInformation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// ------------------------------------------------------------
// ------------------------------------------------------------
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.
// ------------------------------------------------------------
Expand Down Expand Up @@ -92,6 +92,23 @@ public static bool TryGet(Type actorType, out ActorTypeInformation actorTypeInfo
/// and is not marked as abstract.</para>
/// </exception>
public static ActorTypeInformation Get(Type actorType)
{
return Get(actorType, actorTypeName: null);
}

/// <summary>
/// Creates an <see cref="ActorTypeInformation"/> from actorType.
/// </summary>
/// <param name="actorType">The type of class implementing the actor to create ActorTypeInformation for.</param>
/// <param name="actorTypeName">The name of the actor type represented by the actor.</param>
/// <returns><see cref="ActorTypeInformation"/> created from actorType.</returns>
/// <exception cref="System.ArgumentException">
/// <para>When <see cref="System.Type.BaseType"/> for actorType is not of type <see cref="Actor"/>.</para>
/// <para>When actorType does not implement an interface deriving from <see cref="IActor"/>
/// and is not marked as abstract.</para>
/// </exception>
/// <remarks>The value of <paramref name="actorTypeName"/> will have precedence over the default actor type name derived from the actor implementation type or any type name set via <see cref="ActorAttribute"/>.</remarks>
public static ActorTypeInformation Get(Type actorType, string actorTypeName)
{
if (!actorType.IsActor())
{
Expand Down Expand Up @@ -121,7 +138,7 @@ public static ActorTypeInformation Get(Type actorType)

var actorAttribute = actorType.GetCustomAttribute<ActorAttribute>();

string actorTypeName = actorAttribute?.TypeName ?? actorType.Name;
actorTypeName ??= actorAttribute?.TypeName ?? actorType.Name;
bjorkstromm marked this conversation as resolved.
Show resolved Hide resolved

return new ActorTypeInformation()
{
Expand Down
27 changes: 27 additions & 0 deletions test/Dapr.Actors.Test/Runtime/ActorRuntimeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ namespace Dapr.Actors.Test
using Microsoft.Extensions.Logging;
using Xunit;
using Dapr.Actors.Client;
using System.Reflection;

public sealed class ActorRuntimeTests
{
private const string RenamedActorTypeName = "MyRenamedActor";
private const string ParamActorTypeName = "AnotherRenamedActor";
private readonly ILoggerFactory loggerFactory = new LoggerFactory();
private readonly ActorActivatorFactory activatorFactory = new DefaultActorActivatorFactory();

Expand Down Expand Up @@ -54,6 +56,31 @@ public void TestExplicitActorType()
Assert.Contains(RenamedActorTypeName, runtime.RegisteredActors.Select(a => a.Type.ActorTypeName), StringComparer.InvariantCulture);
}

[Fact]
public void TestExplicitActorTypeAsParamShouldOverrideInferred()
{
var actorType = typeof(TestActor);
var options = new ActorRuntimeOptions();
options.Actors.RegisterActor<TestActor>(ParamActorTypeName);
var runtime = new ActorRuntime(options, loggerFactory, activatorFactory, proxyFactory);

Assert.NotEqual(ParamActorTypeName, actorType.Name);
Assert.Contains(ParamActorTypeName, runtime.RegisteredActors.Select(a => a.Type.ActorTypeName), StringComparer.InvariantCulture);
}

[Fact]
public void TestExplicitActorTypeAsParamShouldOverrideActorAttribute()
{
var actorType = typeof(RenamedActor);
var options = new ActorRuntimeOptions();
options.Actors.RegisterActor<RenamedActor>(ParamActorTypeName);
var runtime = new ActorRuntime(options, loggerFactory, activatorFactory, proxyFactory);

Assert.NotEqual(ParamActorTypeName, actorType.Name);
Assert.NotEqual(ParamActorTypeName, actorType.GetCustomAttribute<ActorAttribute>().TypeName);
Assert.Contains(ParamActorTypeName, runtime.RegisteredActors.Select(a => a.Type.ActorTypeName), StringComparer.InvariantCulture);
}

// This tests the change that removed the Activate message from Dapr runtime -> app.
[Fact]
public async Task NoActivateMessageFromRuntime()
Expand Down
18 changes: 17 additions & 1 deletion test/Dapr.Actors.Test/Runtime/ActorTypeInformationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
// ------------------------------------------------------------
// ------------------------------------------------------------
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.
// ------------------------------------------------------------

namespace Dapr.Actors.Test
{
using System;
using System.Reflection;
using Dapr.Actors;
using Dapr.Actors.Runtime;
using Xunit;

public sealed class ActorTypeInformationTests
{
private const string RenamedActorTypeName = "MyRenamedActor";
private const string ParamActorTypeName = "AnotherRenamedActor";

private interface ITestActor : IActor
{
Expand All @@ -38,6 +41,19 @@ public void TestExplicitActorType()
Assert.Equal(RenamedActorTypeName, actorTypeInformation.ActorTypeName);
}

[Theory]
[InlineData(typeof(TestActor))]
[InlineData(typeof(RenamedActor))]
public void TestExplicitActorTypeAsParam(Type actorType)
{
Assert.NotEqual(ParamActorTypeName, actorType.Name);
Assert.NotEqual(ParamActorTypeName, actorType.GetCustomAttribute<ActorAttribute>()?.TypeName);

var actorTypeInformation = ActorTypeInformation.Get(actorType, ParamActorTypeName);

Assert.Equal(ParamActorTypeName, actorTypeInformation.ActorTypeName);
}

private sealed class TestActor : Actor, ITestActor
{
public TestActor(ActorHost host)
Expand Down