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

Entity invocation fails with explicit interface implementations #928

Open
aaronpowell opened this issue Sep 9, 2019 · 7 comments

Comments

@aaronpowell
Copy link

commented Sep 9, 2019

If you implement an interface explicitly on the Entity class the DispatchAsync will fail as the method(s) aren't found.

Example implementation:

public interface IChatRoom {
    DateTime Post(string content);
    List<KeyValuePair<DateTime, string>> Get();
}

public class ChatRoom : IChatRoom {
    public ChatRoom()
    {
        this.ChatEntries = new SortedDictionary<DateTime, string>();
    }

    [JsonProperty("chatEntries")]
    public SortedDictionary<DateTime, string> ChatEntries { get; set; }

    // an operation that adds a message to the chat
    DateTime IChatRoom.Post(string content)
    {
        var timestamp = DateTime.UtcNow;
        this.ChatEntries.Add(timestamp, content);
        return timestamp;
    }

    // an operation that reads all messages in the chat
    List<KeyValuePair<DateTime, string>> IChatRoom.Get()
    {
        return this.ChatEntries.ToList();
    }
}

The reason for this is that the method name(s) are Namespace.InterfaceName.MethodName on the type implementing it.

I came across this problem when trying to use Durable Entities with F# which only supports explicit interface implementations (ref 1 and ref 2).

Here's an F# sample:

type ITheatersForMovie =
    abstract member Init: Theater array -> unit

type TheatersForMovie() =
    member val Theaters = Array.empty<Theater> with get, set

    [<FunctionName("TheatersForMovie")>]
    member __.Run([<EntityTrigger>] ctx : IDurableEntityContext) =
        ctx.DispatchAsync<TheatersForMovie>()

    interface ITheatersForMovie with
        member this.Init theaters =
            this.Theaters <- theaters

The error message shown is similar to the one in #862.

Digging into the code it seems that the problem is on this line: https://github.com/Azure/azure-functions-durable-extension/blob/v2.0.0-beta2/src/WebJobs.Extensions.DurableTask/EntityScheduler/TypedInvocationExtensions.cs#L37-L42

A workaround is to create a method on the type that matches the name that is on the interface which just calls through to the interface:

type ITheatersForMovie =
    abstract member Init: Theater array -> unit

type TheatersForMovie() =
    member val Theaters = Array.empty<Theater> with get, set

    [<FunctionName("TheatersForMovie")>]
    member __.Run([<EntityTrigger>] ctx : IDurableEntityContext) =
        ctx.DispatchAsync<TheatersForMovie>()

    interface ITheatersForMovie with
        member this.Init theaters =
            this.Theaters <- theaters

    member this.Init theaters =
        (this :> ITheatersForMovie).Init theaters

@msftbot msftbot bot added the Needs: Triage 🔍 label Sep 9, 2019

@aaronpowell

This comment has been minimized.

Copy link
Author

commented Sep 9, 2019

The only solutions I can think of is to:

  • Use GetMethods and then run a LINQ Where against them to filter against the name, maybe in an EndsWith test
  • Require the interface to be passed in, not the class to DispatchAsync and then reflection can be done against the interface
    • Not sure that'd work because, if I understand it correctly, the type is created within that method
  • Use GetMethod as current, if it returns null then check for interfaces and look against each of them for the named method on them (or do a LINQ-Where lookup)
  • Not support explicit interface implementations (which will mean that F# support is really problematic)
@cgillum

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

FYI @shibayan in case you have any thoughts on a solution for this. I didn't realize that reflection behaves differently with explicit interface implementations!

@cgillum cgillum added this to the v2.0.0-beta3 milestone Sep 9, 2019

@cgillum cgillum added bug and removed Needs: Triage 🔍 labels Sep 9, 2019

@aaronpowell

This comment has been minimized.

Copy link
Author

commented Sep 9, 2019

I didn't realise either! Caught me off guard when I was trying to work out the problem.

@shibayan - I'm happy to implement the fix if you let me know which of the options are preferred.

@shibayan

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

@cgillum @aaronpowell I didn't know this either. I think that both the first and third solutions work well.

In my opinion, it's better to get a method from the type of the implemented interface. In that case, I want to restrict that only one interface can be implemented in Entity class.

Either way, there may be a method with the same name in the explicit and implicit implementations, so we should consider which method should take precedence.

@aaronpowell

This comment has been minimized.

Copy link
Author

commented Sep 10, 2019

That's a good point on the order of precedence around methods with the same name as the interface. My assumption would be that you want the interface method to be the one to call, since the proxy is really what you work against in the SignalEntityAsync call.

@shibayan

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Will it behave as follows?

  • If interface is implemented
    • Search by explicit implementation first. If not found, current behavior
  • If interface is not implemented
    • Keep current behavior
aaronpowell added a commit to aaronpowell/azure-functions-durable-extension that referenced this issue Sep 11, 2019
aaronpowell added a commit to aaronpowell/azure-functions-durable-extension that referenced this issue Sep 11, 2019
Azure#928 implementing support for explicit interfaces
- Found out that nested types have '+' in their FullName, which isn't in the member name
- Tried to create logic branches to bail early where it can (eg: implicit interfaces)
- Validation for entities only having 1 interface
@aaronpowell

This comment has been minimized.

Copy link
Author

commented Sep 11, 2019

@shibayan @cgillum I've opened PR #935 which addresses this bug. Tried to tackle a number of scenarios and edge cases along the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.