JasperFx / marten

.NET Transactional Document DB and Event Store on PostgreSQL
https://martendb.io
MIT License
2.84k stars 450 forks source link

Make it easier to use strongly-typed identifiers #2487

Closed agross closed 4 months ago

agross commented 1 year ago

I'm referring to this excellent article: https://event-driven.io/en/using_strongly_typed_ids_with_marten/

Since my domain mostly uses Guids handling IDs can become very confusing. I prefer to also use them for projections, since some projections contain multiple such IDs and I did confuse them at least once. The following example shows one such case where it is easy to mix up the schedule ID and the workplace ID.

public record WorkplaceCalendar([property: Identity] Guid ScheduleId,
                                Guid WorkplaceId,
                                DateOnly Date)

I was trying to use a F# single-case discriminated union with FSharp.SystemTextJson to define such an ID:

type WorkplaceId = WorkplaceId of System.Guid
type ScheduleId = ScheduleId of System.Guid

The serializer project above appears to have special support for single-case unions where only the Guid will be serialized.

It causes unions that have a single case with a single field to be treated as simple "wrappers", and serialized as their single field's value.

Source: https://github.com/Tarmil/FSharp.SystemTextJson/blob/master/docs/Customizing.md#unwrap-single-case-unions

Unfortunately, Marten no longer considers the Identity to be usable.

I tried to customize the SystemTextJson serializer for the case where Marten does not consider the ID property.

// This is how I would the record to look like.
public record WorkplaceCalendar(ScheduleId Id,
                                WorkplaceId WorkplaceId,
                                DateOnly Date)

storeOptions.Policies.ForAllDocuments(mapping =>
          {
            if (mapping.IdType == null) // null for the record above.
            {
              mapping.IdMember = mapping.DocumentType.GetProperty("Id");
              mapping.IdStrategy = new CustomIdGeneration();
            }
          });

public class CustomIdGeneration : IIdGeneration
{
  public IEnumerable<Type> KeyTypes => new[] { typeof(WorkplaceId) };

  public bool RequiresSequences { get; } = false;
  public void GenerateCode(GeneratedMethod assign, DocumentMapping mapping)
  {
    // Not sure how this should look like, since I didn't get past Marten init.
    var document = new Use(mapping.DocumentType);
    assign.Frames.Code($"_setter({{0}}, \"newId\");", document);
    assign.Frames.Code($"return {{0}}.{mapping.IdMember.Name};", document);
  }
}

The above fails with System.ArgumentOutOfRangeException: Id members must be an int, long, Guid, or string (Parameter 'IdMember') which is sad because the IdMember is a Guid that I would like to wrap and unwrap myself.

It would be great if this could be improved in a future version.

kirkbrauer commented 1 year ago

I am also having the same problem with this. I want to be able to use my custom Identity<T> identifier classes in LINQ queries, however I haven't been able to figure out how to make them work without throwing the ArgumentOutOfRangeException. As mentioned in the issue, our JSON serializer already converts these to strings and that is how they're persisted in the document databse.

It would be awesome if I could register a method to wrap/unwrap these identities to their underlying Guid and string IDs and use them in LINQ without falling back to raw SQL statements. I also would like to use these to create indexes, but for now that doesn't seem to be possible.

jeremydmiller commented 1 year ago

Hey guys, we know that many people are interested in having Marten support strong typed identifiers, and we'll get around to it someday. It hasn't happened yet because it's a huge change and has way more impact on the API surface than I'm personally excited about before you even get into what a PITA that's going to be in the Linq support.

dgrozenok commented 1 year ago

Would be great to have that supported. I was able to achieve this with MongoDB driver and custom bson serialization. The LINQ was supported with the wrapper types, but the actual types stored was the simple internal type like string or guid. I agree though it might be challenging to do.

ngoov commented 1 year ago

Is there someway we can upvote issues? This would be a great addition, but in the meanwhile, I am using the solution explained by Oskar Dudycz https://event-driven.io/en/using_strongly_typed_ids_with_marten/, sadly needing explicit persistence logic in the domain object being exposed, the public Id property.

jeremydmiller commented 10 months ago

Notes and problems:

Concerns

Ideas

Test Cases

What about events?

Holy hell, I don't know. Might treat this one completely separately. Might be nice to utilize the strong typed id to identify the aggregate streams

Blackclaws commented 9 months ago

So I've been using Vogen (https://github.com/SteveDunn/Vogen) as my go-to for creating ValueObjects. Its been working like a charm and the JsonConverters actually just convert it to its basic value, so there is no .Value or anything that pops up in the serializer.

So for my use case Marten converting the value to search using the Json converter and then simply comparing the raw Json at the respective points would probably already solve the issue.

jeremydmiller commented 9 months ago

@Blackclaws As we've discussed ad nauseam in Discord, that doesn't really do much at all to solve the usability issues here and there's no magic "just" action that is going to make strong typed ids work for a wide range of use cases.

Blackclaws commented 9 months ago

@Blackclaws As we've discussed ad nauseam in Discord, that doesn't really do much at all to solve the usability issues here and there's no magic "just" action that is going to make strong typed ids work for a wide range of use cases.

Right, I just thought you'd be interested in what exactly I'm using and what my use case is given that you asked for comment and I assumed this was a thread to collect use-cases etc. to then formulate a plan for what Marten might do to accommodate StrongIds.

In my comment I was mainly thinking about the Linq Queries and Includes which, as far as I can tell, are built bit by bit right now and transformed into a postgres query. For StrongIds that keep their Value as separate field inside this method doesn't really work as the path to the actual value is wrong when compared to the serialized flattened version.

The simple case, comparing a StrongId in a document to one passed in should be solvable by running the JsonConverter on the StrongId and using that to compare.

The much harder case, where a nonStrongId would be compared to a strongIds value, such as possibly the Include() case would need to handle this translation in a way where the access to the inner Value is silently ignored as soon as the StrongId object is reached in the expression tree and again the JsonSerialized value is used.

The requirements on StrongIds would thus include that they JsonSerialize to their contents only without keeping their internal structure if they want to be used for Include() that compares a strongId to a nonStrongId.

As for LoadAsync, you could double Template that, so LoadAsync<Document, IdType>(). That would probably be easier to handle than LoadAsync(object) as you said.

I don't think having to explicitly register StrongId types with Marten would be a significant problem either.

oskardudycz commented 9 months ago

I see Strongly-typed keys as really nice to have rather than a must-have. I think that for most cases, such strong IDs are overkill. Actually, they do not really value objects, as most of the implementations just wrap the primitive. They could be useful where the business identifier is built from multiple values.

I think that if providing the option to support that would be hard to do, or require rotten tradeoffs in the API then I'd vote for not implementing it, or providing partial support (e.g. having it on the store level, so IConvertible) and not having them on the Linq side.

I'd also prefer not to have Load(object) method, but we could trick that with some extension method. LoadAsync<Document, IdType>() is potential alternative, but IMHO it doesn't help much, as one could provide incompatible id as param (unless we introduce the IDocument<Key> marker interface, which I definitely wouldn't like to have).

What we could consider is maybe reviewing if we need around the place the real need for having strong restrictions on the id field in event stream and documents. Maybe we should start by thinking if we need to have it on the document level (so like Mongo has it as optional with id field). That could make things easier as we wouldn't query the payload but the column. That would also make live projections for write model better, as it wouldn't require Id . Create method handling could be also potentially simplified by that in some cases.

johnholliday commented 9 months ago

I ventured down this rabbit-hole for a minute, but I have to respectfully dissent on the whole idea of supporting strongly-typed keys in the persistence layer. Although I'm an avid proponent of strongly-typed identifiers, I quickly realized that whatever work-around I came up with for Marten would have to be re-invented if I needed to switch to a different repository. It's simple enough to declare DTO's at the repository level.

Blackclaws commented 9 months ago

I ventured down this rabbit-hole for a minute, but I have to respectfully dissent on the whole idea of supporting strongly-typed keys in the persistence layer. Although I'm an avid proponent of strongly-typed identifiers, I quickly realized that whatever work-around I came up with for Marten would have to be re-invented if I needed to switch to a different repository. It's simple enough to declare DTO's at the repository level.

The question is what happens when you're not using a repository abstraction on top of Marten? This is actually advocated by @jeremydmiller himself to a degree and I have to agree that it does make sense to have the full power of Marten available. I do like using Strong Ids or ValueObjects in general simply for the fact that it prevents confusion at the compile level already.

mdissel commented 9 months ago

Here's another library to maintain strongly typed identifiers, https://github.com/andrewlock/StronglyTypedId/

migajek commented 9 months ago

I was wondering whether assuming a constraint on id type would help reducing the complexity of the problem.

My approach would be to require the id type to be convertible to and from string. Either by expecting a constructor that takes the single string argument, or implementing a pair of implicit operators.

It'd be up to the user to provide the valid implementation of id type. Since it's pretty common for them to actually wrap a single value like @oskardudycz already pointed, it doesn't seem to be a problem.

I might be wrong here (very likely I am wrong here) but I think it would leverage current support for string identifiers in many areas internally with no major changes, except for casting to/from string where required, and changing id member utilities to handle custom id type when validating.

No idea how that'd play with code generation (that part of Marten is still a black box for me)

jeremydmiller commented 4 months ago

Notes

jeremydmiller commented 4 months ago

Punchlist / Remaining Tests

nkosi23 commented 4 months ago

Thanks a lot for the feedback you provided in the other issue!

I've looked at your notes posted above and find the approach used very cool, however I am wondering if your current implementation will work for F# discriminated unions.

The following F# type (which is representative of the way strongly typed ids are defined - nothing more fancy than a single case discriminated union):

type OrderId = Id of Guid

Is turned into the following C# class by the F# compiler:

public abstract class OrderId
{
    // Nested class for the 'Id' case
    public sealed class Id : OrderId
    {
        public Guid Value { get; } // Property to hold the Guid

        public Id(Guid value) // Constructor
        {
            Value = value;
        }

        // Overridden equality and GetHashCode for proper comparison 
        public override bool Equals(object obj) => obj is Id other && Value.Equals(other.Value);
        public override int GetHashCode() => Value.GetHashCode();
    }

    // Private constructor to prevent direct instantiation of OrderId
    private OrderId() { } 

    // Static factory methods for creating instances
    public static OrderId NewId(Guid value) => new Id(value);
}

What's tricky is that we can only reference union types,not specific cases such that for example an F# record typically looks like this:

type Order = { Id: OrderId; ProductName: string }

Therefore if deserialization is based on the type of the document property, all Marten will see is the abstract class. and it won't be able to know which nested class to use (which case to use).

If this is indeed an issue, I'd suggest sticking to only supporting single case discriminated unions (which is the only way strongly typed ids are defined in f# in practice anyway), so that one of the two following relatively easy tests can be made:

jeremydmiller commented 4 months ago

@nkosi23 Sounds like an awesome pull request to push through the F# discriminated union support!

The C# OrderId above would be usable with the existing approach

nkosi23 commented 4 months ago

@jeremydmiller Great I will give it a try :)