dotnet / orleans

Cloud Native application framework for .NET
https://docs.microsoft.com/dotnet/orleans
MIT License
10.06k stars 2.03k forks source link

Static Interface Method on Grain results in ORLEANS0009 Error #8377

Closed cmeyertons closed 1 year ago

cmeyertons commented 1 year ago

Looks like the Orleans Analyzer isn't properly handling the new static interface methods:

public interface TestInterface : IGrainWithStringKey
{
    public static int GetId() => 0;
}

public class TestClass: Grain
{
    public static int GetId() => 0;
}

Interface method has error, class does not:

image

ReubenBond commented 1 year ago

What should Orleans do in this case? Do static interface members make sense on a grain? If so, could you lay out a use case for me to understand the scenario better?

cmeyertons commented 1 year ago

@ReubenBond thanks for the quick reply!

Brand new app w/ a number of compound keys -- after going through some iterations w/ manually managing keys, creating a factory per grain, etc, we wanted to come to an approach that allows for strong typing / contract & encapsulation of the Grain's ID and can be leveraged both internally w/in the grain (deconstruct the key) and by consumers (creating a grain ref).

Here's the implementation of the framework. Wanting to use the static interface method to call into the interface's static method, not the grain's (because we are create a ref to the grain interface, not class)

using System.Text.Json;

namespace AttributeService.Orleans;

public interface IJsonGrainId
{ }

public static class IJsonGrainIdExtensions
{
    public static T GetGrainJsonId<T>(this Grain grain)
        where T : IJsonGrainId
    {
        return JsonSerializer.Deserialize<T>(grain.GetPrimaryKeyString())!;
    }

    public static string ToJsonId<T>(this T id)
        where T : IJsonGrainId
    {
        return JsonSerializer.Serialize(id);
    }
}

public interface IJsonGrain<TGrainInterface, TId> : IGrainWithStringKey
    where TGrainInterface : IJsonGrain<TGrainInterface, TId>
    where TId : IJsonGrainId
{
#pragma warning disable ORLEANS0009 // not appropriately handling statics
    public static TGrainInterface GetGrain(IGrainFactory grainFactory, TId id)
    {
        return grainFactory.GetGrain<TGrainInterface>(id.ToJsonId());
    }
#pragma warning restore ORLEANS0009 // Grain interfaces methods must return a compatible type
}

public abstract class JsonGrainBase<TGrainInterface, TId> : Grain, IJsonGrain<TGrainInterface, TId>
    where TId : IJsonGrainId
    where TGrainInterface : IJsonGrain<TGrainInterface, TId>
{
    public TId Id { get; private set; }

    public JsonGrainBase()
    {
        Id = this.GetGrainJsonId<TId>();
    }

    //public static TGrainInterface GetGrain(IGrainFactory grainFactory, TId id)
    //{
    //    return grainFactory.GetGrain<TGrainInterface>(id.ToJsonId());
    //}
}

example usage:

public static class Examples
{
    private interface IExampleGrain : IJsonGrain<IExampleGrain, ExampleGrainId>
    { }

    private readonly record struct ExampleGrainId(string Id1, int Id2) : IJsonGrainId;

    private class ExampleGrain : JsonGrainBase<IExampleGrain, ExampleGrainId>, IExampleGrain
    { }

    private static IExampleGrain GetExampleGrain(IGrainFactory grainFactory)
    {
        return IExampleGrain.GetGrain(grainFactory, new ExampleGrainId("1", 2));
    }
}

obviously, JSON isn't the most compact / efficient format, so we will do some performance testing on the tradeoffs using it vs a / separate key positionally, etc.

There is additionally probably a story w/in the Orleans framework of adding stronger typing to Grain Ids and implementing source generators that generate GrainFactory extension methods (very similar to what Azure Function team is doing w/ the new Durable framework that is in preview)

ReubenBond commented 1 year ago

Thank you for the explanation and PR and apologies for the slow response here. My primary concern is what should the code generator do when it encounters a static abstract interface method? For example:


public interface IFooGrain : IGrain
{
    static abstract object Create();
}

public class FooReference : IFooGrain 
{
    // Code generator needs to generate an implementation of Create() here, but it cannot create a meaningful implementation
}

For your PR, perhaps the code generator should not skip static members if they are also abstract.

As a side note: I am concerned that the IJsonGrainId interface you have there will not be a suitable grain identifier: aside from the length, it seems likely to create non-deterministic grain keys. Are you guaranteed that JSON serialization never re-orders members? Eg, if you have a HashSet<string> in there, the set of strings may be ordered arbitrarily (the default string hash code function is randomized per process).

Having strongly typed keys is fine, but I think you should craft them with care to ensure determinism (and ideally terseness) while retaining human-readability (for debugging/logging).

cmeyertons commented 1 year ago

No apologies necessary at all! Thank you for the feedback on the JSON based grain ID design.

The HashSet case is good to think about for something more general purpose. We are strictly using readonly record struct w/ primitive types to model our compound keys so not a huge concern in our internal project.

We have appended JsonPropertyOrder of each of the properties to ensure determinism as well (might be overkill).

For the terseness bit, I agree, it definitely could be terser -- the problem is being constrained to string for the Grain key and string-based serialization format's increased verbosity (JSON included). We are now including [JsonPropertyName] w/ a terser name as an optimization.

It's unfortunate the framework doesn't have a "native" way to handle complex Grain Ids (as described in #2320) but I understand the reasoning why. Although, the Orleans Serializer does seem like a fantastic fit to handle this use case though... would've saved a lot of headache to just slap GenerateSerializer on my Id struct and pass it into IGrainFactory.

I will update the PR to include static abstract, static virtual, and static sealed based on the static interface members proposal.

cmeyertons commented 1 year ago

PR has been merged, closing this issue!