orleans icon indicating copy to clipboard operation
orleans copied to clipboard

Poor discoverability and inconsistent naming of serialization attributes

Open mnmr opened this issue 2 years ago • 9 comments

I've been trying to use the Orleans serializer as a stand-alone library, and think that it'd be good for discoverability (and ease of use) if it were to only use project-owned attributes with consistent naming, such as [OrleansSerializer], [OrleansIgnore], [OrleansConstructor], [OrleansId], [OrleansImmutable], etc.

Some attributes (e.g. [NonSerialized]) are likely re-used for compatibility with BinaryFormatter, but is this really desirable to have as the default? I would prefer to opt-in to this behavior by adding BinaryFormatter-specific attributes, even if this means some annotations are duplicated. This would also provide a way to opt-out of using BinaryFormatter altogether.

I was also only able to discover [ActivatorUtilitiesConstructor] by looking at the code, as it's not mentioned in any Orleans docs I could find.

In summary, I think Orleans would benefit from internalizing attributes.

This is related to #7872

mnmr avatar Aug 21 '22 11:08 mnmr

Triage: @ReubenBond we discussed this during triage ad it seems like a prudent idea to add the Orleans prefix to attribute name to avoid name collisions/improve discoverability. Possibly a help wanted issue?

captainsafia avatar Aug 25 '22 18:08 captainsafia

I have volunteered to fix these issues (#7935, #7936 and #7037), and possibly help with docs and other related tasks. Should have a bunch of PRs by the end of the week.

mnmr avatar Aug 25 '22 20:08 mnmr

I have asked the dotnet/runtime team to consider supporting alternate names for the ActivatorUtilitiesConstructorAttribute (issue dotnet/runtime #74655). However, even if accepted this is not likely to materialize before .NET 8.

mnmr avatar Aug 26 '22 15:08 mnmr

I thought it might be a good idea to be slightly more specific about the changes required to resolve this issue.

I am going to assume that we want all attributes to get an Orleans prefix, and that it is desirable to keep supporting the old attribute names for at least those attributes that were present in the latest 3.x release.

In the following I am listing all of the attributes to consider (sans the Attribute suffix), loosely grouped by their purpose:

The core serialization attributes get the Orleans prefix and a new name that is more descriptive:

  • GenerateSerializer => OrleansSerializable
  • GenerateMethodSerializers => OrleansSerializableInterface

All of the attributes for customizing invokable interfaces get the Orleans prefix and most of them also get a slightly modified name to make their purpose easier to derive from the name:

  • InvokeMethodName=> OrleansInvokeMethodName
  • DefaultInvokeMethodNameAttribute=> OrleansInvokeMethodNameDefault
  • InvokableCustomInitializer=> OrleansInvokeCustomInitializer
  • InvokableBaseType=> OrleansInvokeBaseType
  • DefaultInvokableBaseType=> OrleansInvokeBaseTypeDefault
  • GetCompletionSourceMethodName => OrleansInvokeCompletionSourceMethodName

All remaining attributes just get the Orleans prefix:

  • Id => OrleansId
  • WellKnownId => OrleansWellKnownId
  • WellKnownAlias => OrleansWellKnownAlias
  • RegisterSerializer => OrleansRegisterSerializer
  • RegisterActivator => OrleansRegisterActivator
  • RegisterCopier => OrleansRegisterCopier
  • RegisterConverter => OrleansRegisterConverter
  • UseActivator => OrleansUseActivator
  • SuppressReferenceTracking => OrleansSuppressReferenceTracking
  • OmitDefaultMemberValues => OrleansOmitDefaultMemberValues
  • Immutable => OrleansImmutable
  • ApplicationPart => OrleansApplicationPart
  • SerializationCallbacks => OrleansSerializationCallbacks
  • GeneratedActivatorConstructor => OrleansGeneratedActivatorConstructor
  • GenerateCodeForDeclaringAssembly => OrleansGenerateCodeForDeclaringAssembly

Let me know if you think this will work.

mnmr avatar Sep 07 '22 16:09 mnmr

I don't like the idea of sticking "Orleans" in front of everything. What other frameworks/libraries do that? You can already distinguish the attributes using a namespace, and that's what namespaces are for. Eg [Orleans.Id(x)]

ReubenBond avatar Sep 09 '22 19:09 ReubenBond

Some attributes (e.g. [NonSerialized]) are likely re-used for compatibility with BinaryFormatter, but is this really desirable to have as the default?

We could have an in-built [NonSerizalized] attribute (and we can make it applicable to properties, too, unlike the original). Another (not mutually exclusive) option is to make everything configurable via csproj properties, like the id attributes are

ReubenBond avatar Sep 09 '22 19:09 ReubenBond

I don't like the idea of sticking "Orleans" in front of everything. What other frameworks/libraries do that? You can already distinguish the attributes using a namespace, and that's what namespaces are for. Eg [Orleans.Id(x)]

I am also not a fan of unduly long or repetitive names, but I do think they have some advantages:

  • you'd get completion suggestions, even if the attributes are located across multiple namespaces
  • easier narrowing down of suggestions for IDE's that don't filter to attribute types when typing inside [] (e.g. Rider)
  • it helps when reading code, as it requires less knowledge to discern the purpose of the attributes
  • it avoids name clashes with other libraries or folks own code

Attributes in ProtoBuf and System.Text.Json.Serialization follow a similar convention (even though they are all in a single namespace).

That said, some of the attribute names are already very long, and perhaps not used so often as to warrant being included in a naming scheme. If the purpose of the attribute is also somewhat obscure and likely requires the user to study documentation before use, following a naming scheme seems much less important. I don't really feel qualified to evaluate this for the attributes in Orleans, though.

We could have an in-built [NonSerizalized] attribute (and we can make it applicable to properties, too, unlike the original).

I'd propose OrleansIgnore to follow the convention from Json/Proto.

Another (not mutually exclusive) option is to make everything configurable via csproj properties, like the id attributes are

I like that they are configurable, but am not entirely convinced the feature is useful. Let me run down my thoughts on pro/cons as I see them:

  • I can see this as a useful mechanism for supporting deprecated attributes, as well as for supporting well-known attributes (e.g. from the BCL).
  • It only really works for marker attributes that don't have parameters. Parameter values are extracted as positional constructor arguments, so a seemingly compatible attribute with an identically named property might not be (if it didn't have a parameter for the value, or had other parameters before it). This can be worked around in code, but then there's also type conversions (e.g. cast from int/long to ushort) and associated validations to deal with.
  • Maybe a less general solution could allow only the most relevant attributes (e.g. Id and the two WellKnown) to be configurable. Marker attributes are trivial to make configurable so they can be included at will.

mnmr avatar Sep 09 '22 23:09 mnmr

The main reason for making them configurable was people not wanting to link to any Orleans libraries or being unable to change the library which code is being generated for. An alternative to that is allowing them to define the same attributes in their own code.

I'm in favor of putting all attributes directly in the "Orleans" namespace so that they are more discoverable. If people don't know what they need, they can type [Orleans. and let the IDE completion list show the relevant results.

ReubenBond avatar Sep 10 '22 18:09 ReubenBond

The main reason for making them configurable was people not wanting to link to any Orleans libraries or being unable to change the library which code is being generated for. An alternative to that is allowing them to define the same attributes in their own code.

That makes sense.

I'm in favor of putting all attributes directly in the "Orleans" namespace so that they are more discoverable. If people don't know what they need, they can type [Orleans. and let the IDE completion list show the relevant results.

Okay, okay. Here is my amended proposal for changes:

  • Add OrleansIgnore
  • Rename GenerateSerializer to OrleansSerializable
  • Rename GenerateMethodSerializers to OrleansSerializableInterface
  • Ensure all the attributes are located in the Orleans namespace
  • Make all the attribute names configurable, but code essentially has to be a copy

mnmr avatar Sep 10 '22 21:09 mnmr