[Proposal] Consider optional support for `class` vs. `struct`

[prlcutting]

Hi Andrew. In your excellent series of blog posts on Strongly Typed Id's, you make mention of some downsides to using a struct to model identity versus a class, due to implicit parameterless constructors. You also cite Vladamir Khorikov's article on the subject who makes further arguments for not choosing a struct for modelling identity and favoring a class. Classes have their downsides too of course, not least of which is having to null-check them all over the place, so they're not a panacea, but if you're trying to follow DDD best practices where protection of invariants is paramount, a class-based option would be desirable.

So, with all that said, have you considered enabling class-based strongly typed identifiers in your library in addition to struct-based ones?

[andrewlock]

Hi @prlcutting, yes, it's definitely something I've considered🙂 It's just not entirely clear to me that it's worth the hassle, when you can do something similar with record types these days, e.g.

public record OrderId(int Value);

This is something Thomas Levesque discusses on his blog, though things are even easier with c#10 as you can add sealed to the ToString() implementation, which means you don't even need the source generator part, you can do it all with a base-type.

I considered whether you can do the same thing with struct record and make this library obsolete, but the lack of inheritance means you're still stuck with a lot of boilerplate, so I think this still adds value.

If the record approach doesn't work for you, I'd be interested to know why. I'm not opposed to adding it as an option, but just want to make sure it makes sense!

[prlcutting]

Thanks for getting back to me @andrewlock, and apologies for the delayed response.

Vladamir has an excellent blog post, C# 9 Records as DDD Value Objects which compares and contrasts C#'s records with a more traditional DDD ValueObject base class approach. Many of the points he raises don't apply in the case of strong typed identifiers where there is a single, scalar value to be encapsulated, but a couple do.

The ability to compare identifiers through an IComparable<T> interface implementation would be helpful, e.g. to provide a stable sort when the data isn't sourced from a database and pre-ordered. But probably my chief concern with using records out of the box would be protecting invariants, e.g. disallowing negative values for numeric based backing values, Guid.Zero in the case of Guid's, etc., assuming they were requirements (which I suspect they often would be). This is what puts me off the struct-based approach, because such invariants cannot be enforced, which undermines the model for the identifier.

You can of course achieve such protection of invariants with records, but now you're back to writing somewhat verbose, repetitive code, which obviates the benefits over traditional classes at that point. Avoiding that verbose, repetitive code could be aided by refactoring into a base class as you suggest, or generated automatically with a source generator. It would of course assume that the code generation could be configured (perhaps with additional attribute "options") to configure min/max bounds etc.

Your challenge on my proposal made me think about this a little more deeply, and I'm grateful for that! Perhaps at this point, in the context of strongly typed identifiers, the question is more generalized to: which is better, a class hierarchy with most logic refactored into a shared base class, or source generation? I'd be curious if you have any opinions on that. Thanks again.

[kyrvlasiuk]

One reason to choose your library over the record class and record struct is all the converters and interfaces that are included and generated. If it would be possible to use them with records, it would be nice. Considering all the debugging and syntax support they are getting/ going to get. But this is definitely something I can live without.
It would be nice to be able to specify any type (or have a composite id) and have converters generated around it for ef core and system.text.json especially.
Then it's like

[StronglyTypedId( StronglyTypedIdConverter.SystemTextJson | StronglyTypedIdConverter.EfCoreValueConverter)]
public partial record struct MyId(Int16 Value);

[StronglyTypedId( StronglyTypedIdConverter.SystemTextJson | StronglyTypedIdConverter.EfCoreValueConverter)]
public partial record struct CompositId(string Part1, string Part2);

This is nice-to-haves, affects few categories. Looking good but not worth the effort.

[dombrovsky]

As for me, this is a significant issue with this otherwise just awesome library, that reference type ids can be generated.

Main issue is invariants protection: too easy to just pass or accidently have default(StringId) for [StronglyTypedId(Template.String)] struct StringId { }, which would not be same as StringId.Empty, eventually getting NRE later on, instead of getting ArgumentException early.

It would be much more preferable if it was class StringId with constructor that can do necessary precondition checks to ensure that if StringId instance exists, it is sane. And that may not only be null checks, but also that string is in certain format. Reference type ids can be useful for value type underlying ids for that reason too, e.g. to ensure Int32Id has only positive values for example.

C# Records are not an equivalent replacement to this library, because of all the nice features that are coming with code generation, e.g. type converters, static Empty member, implicit cast operators etc.

[gorillapower]

Ive also encountered this problem with the struct default constructor behaviour. I wonder if a simple [Obsolete] attribute could be used to provide a compiler error like so.

public readonly record struct ProductsId
{
    [Obsolete("Cannot be empty", true)]
    public ProductsId() { }


    public ProductsId(string value)
    {
        ArgumentNullException.ThrowIfNullOrEmpty(value);
        //other validation
        this.Value = value;
    }
    
    public string Value { get; }
}

Which then looks like this at compile time

And in saying this, if its possible to add this to the SourceGenerator code for this library? Maybe its already possible with the custom templates feature.

As for me, this is a significant issue with this otherwise just awesome library, that reference type ids can be generated.

Main issue is invariants protection: too easy to just pass or accidently have default(StringId) for [StronglyTypedId(Template.String)] struct StringId { }, which would not be same as StringId.Empty, eventually getting NRE later on, instead of getting ArgumentException early.

It would be much more preferable if it was class StringId with constructor that can do necessary precondition checks to ensure that if StringId instance exists, it is sane. And that may not only be null checks, but also that string is in certain format. Reference type ids can be useful for value type underlying ids for that reason too, e.g. to ensure Int32Id has only positive values for example.

C# Records are not an equivalent replacement to this library, because of all the nice features that are coming with code generation, e.g. type converters, static Empty member, implicit cast operators etc.

[DorianGreen]

@andrewlock Reviving this.

The new templating is great, but the generator code explicitly looks for structs only.
Is it possible to extend the generator to work on record structs, and potentially classes and record classes?

[andrewlock]

FYI, I'm still torn on this one. Fundamentally I don't really want to use classes for this personally, plus it makes the templating some more complex if you want to create general templates, but I thought I'd take a look.

Unfortunately, I ran into one specific blocker - if you use a class-based StronglyTypedId with the source-generator JsonSerializationContext, the System.Text.Json generated code doesn't compile 😕

Specifically, this method, in the ObjectCreator, it tries to call the default constructor, which doesn't exist.

private global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::StronglyTypedIds.IntegrationTests.Types.ClassGuidId> Create_ClassGuidId(global::System.Text.Json.JsonSerializerOptions options)
        {
            if (!TryGetTypeInfoForRuntimeCustomConverter<global::StronglyTypedIds.IntegrationTests.Types.ClassGuidId>(options, out global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::StronglyTypedIds.IntegrationTests.Types.ClassGuidId> jsonTypeInfo))
            {
                var objectInfo = new global::System.Text.Json.Serialization.Metadata.JsonObjectInfoValues<global::StronglyTypedIds.IntegrationTests.Types.ClassGuidId>
                {
                    // 👇 Doesn't compile ❌
                    ObjectCreator = () => new global::StronglyTypedIds.IntegrationTests.Types.ClassGuidId(),
                    ObjectWithParameterizedConstructorCreator = null,
                    PropertyMetadataInitializer = _ => ClassGuidIdPropInit(options),
                    ConstructorParameterMetadataInitializer = null,
                    ConstructorAttributeProviderFactory = static () => typeof(global::StronglyTypedIds.IntegrationTests.Types.ClassGuidId).GetConstructor(InstanceMemberBindingFlags, binder: null, global::System.Array.Empty<global::System.Type>(), modifiers: null),
                    SerializeHandler = ClassGuidIdSerializeHandler,
                };
                
                jsonTypeInfo = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateObjectInfo<global::StronglyTypedIds.IntegrationTests.Types.ClassGuidId>(options, objectInfo);
                jsonTypeInfo.NumberHandling = null;
            }
        
            jsonTypeInfo.OriginatingResolver = this;
            return jsonTypeInfo;
        }

I assume there's some configuration we could add to the template to fix this, but I'm not sure whta it is right now 🤔 I've tried using the [JsonConstructorAttribute] but that doesn't seem to make any difference either...

Anyone have any ideas?

[andrewlock]

Came back to this again, and the issue above isn't strictly a blocker, as it comes from trying to use the System.Text.Json source generator in the same project as the generated IDs. And due to source generators not being able to "see" each other.

However, there's a fundamental design issue I'm struggling with. The problem is that the templates currently provided don't really work in some cases. For example, some of the nullability annotations end up incorrect

e.g. for a class we need

public bool Equals(PLACEHOLDERID? other) => this.Value.Equals(other?.Value);

vs. for a struct:

public bool Equals(PLACEHOLDERID other) => this.Value.Equals(other.Value);

That's not a big deal, we could even ignore the nullable attributes. But if we want to support record and record struct too (which we might want to, it makes sense if we allow class I think), then we run into other issues. Records synthesize some equality methods, and it's an error to define them yourself. But we want to define them for struct and class IDs, so we can't create "universal" templates.

All of which leads me to several possibilities

1. Only support class and struct IDs; don't support record or record struct; create "universal" templates that just ignore the nullability for "problematic" methods like Equals above
2. Create different built-in templates based on the target type (class/record/struct) and automatically choose the correct one based on the ID. Custom templates don't have any special support.
3. As above, but support "conventions" for template names. e.g. my-template.struct.typedid/my-template.class.typedid/my-template.record.typedid can be applied just using [StronglyTypedId("my-template"] and the correct template is selected based on the target type

I'm torn between all of these. I like the simplicity of 1, but it's a shame not to support records. 2 gives good support without too much "magic" or "unexpected" behavior in the custom templates. 3 allows to transparently "update" the existing custom templates with support for the other types.

What do others think? Would the lack of record/record struct support be an issue? If not, would you rather custom templates require you to be explicit or use some conventions?

[ajbeaven]

I'd love to see this.

I'm working on a project using entity framework mapping configuration. The struct requirement for StronglyTypedIds means it's not possible to model certain things that would otherwise be possible if classes were supported.

I know this particular example is a bit of an edge case, but the following code for mapping a list of ids in an entity to a database table (for cross-aggregate references when adhering to DDD) fails due to the struct on the StronglyTypedId e.g:

Code

[StronglyTypedId]
public partial struct DinnerId
{
}

public sealed class Menu : AggregateRoot<MenuId>
{
    private readonly List<DinnerId> _dinnerIds = new();
    public IReadOnlyList<DinnerId> DinnerIds => _dinnerIds.AsReadOnly();
    // ...
}

public class MenuConfigurations : IEntityTypeConfiguration<Menu>
{
    public void Configure(EntityTypeBuilder<Menu> builder)
    {
        builder.OwnsMany(m => m.DinnerIds, dib => // this fails because the TRelatedEntity (DinnerId in this case) must be a class
        {
            dib.ToTable("MenuDinnerIds");
            // ...
        });
    }
}

Could this be a progressive enhancement where you start with 1 and add support records later?