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

Make it easier to use strongly-typed identifiers #2487

Closed
agross opened this issue Feb 12, 2023 · 18 comments
Closed

Make it easier to use strongly-typed identifiers #2487

agross opened this issue Feb 12, 2023 · 18 comments
Labels

Comments

@agross
Copy link
Contributor

agross commented Feb 12, 2023

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
Copy link

kirkbrauer commented Feb 16, 2023

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
Copy link
Member

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
Copy link

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
Copy link

ngoov commented Jun 27, 2023

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 jeremydmiller added this to the 7.0.0 milestone Sep 8, 2023
@jeremydmiller
Copy link
Member

jeremydmiller commented Dec 5, 2023

Notes and problems:

  • Need to deal w/ the JSON converter
  • LINQ querying, use Id field no matter what
  • Depend on IConvertable, where the T can be string, Guid, int, long
  • Checkout Andrew Lock's post on strong typed ids
  • Watch projected aggregate Id

Concerns

  • Don't want to blow up the API with overloads, but not sure how to avoid that
  • Would like to avoid having a LoadAsync(object) method because of the boxing and the potential for all kinds of other usability problems
  • Would like to prevent any necessary coupling between the user's strong typed id record and Marten itself. Guarantee that DDD or Clean/Onion folks would complain about that
  • Don't want to make StrongTypedId mandatory, and don't think that's absolutely necessary
  • Have Marten targets in the StrongTypedId.Templates nuget maybe
  • JSON serialization converters could maybe be automatic? Maybe register converters automatically for the known identifiers if we make the presence of IParseable<T> / ISpanParsable<T> on the id types mandatory

Ideas

  • Definitely still store the raw Guid/string/int/long in the "id" field in the document tables
  • Use a new IQueryableMember type for the strong typed ids that "bakes in" how to get the raw Guid/int/string/long from the object in its internals. This member should act just like the existing IdMember except for adapting LINQ to the signature
  • Maybe make overloads for LoadAsync(IParseable<Guid/string/int/long>) et al methods as the tip off that "here be a strong typed identifier". I like that better than a LoadAsync(object) overload that could do all kinds of harm even though that's a ton of more overloads

Test Cases

  • Identity member determination on DocumentMapping
  • IDocumentStorage can set a new id on documents with the strong typed identifier if it's missing
  • Guid based identifiers use sequential Guids
  • int/long based identifiers use the HiLo
  • string based identifiers use "user defined" identifiers
  • Store()
  • Update()
  • Insert()
  • Delete() by id
  • IQuerySession.Load/LoadAsync()
  • IQuerySession.LoadMany/LoadManyAsync()
  • Where(x => x.Id == someId)
  • Use within an Include(x => x.OtherId) linkage (yikes)
  • Foreign Key to a document that uses strong typed id
  • Foreign Key from a document that uses a strong typed id
  • FK for strong typed id to strong typed id document
  • Projected document uses a strong typed id, automatically assigned by Marten

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
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Collaborator

oskardudycz commented Jan 5, 2024

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

mdissel commented Jan 15, 2024

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

@migajek
Copy link
Contributor

migajek commented Jan 19, 2024

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 jeremydmiller modified the milestones: 7.0.0, 7.1.0 Feb 20, 2024
@jeremydmiller jeremydmiller removed this from the 7.1.0 milestone Jun 7, 2024
@jeremydmiller
Copy link
Member

Notes

  • Consider any type that has a single public property of either int, long, string, or Guid
  • If it's string, it has to be strictly user supplied
  • Use either a public constructor with a single argument of int, long, or Guid or a public static method that takes the single argument to build the identifiers. That covers both StrongTypedId and Vogen.
  • If a Guid, use Marten's sequential guid creation behind the scenes
  • If an int or long, use HiLo

@jeremydmiller
Copy link
Member

jeremydmiller commented Jun 11, 2024

Punchlist / Remaining Tests

  • Assert on Store() for a string based identifier when the identifier is missing?
  • Enforce that the id must be a Nullable somehow?
  • Docs -- but not until everything else works!!!
  • Add ValueTypeTests to CI
  • Bulk writing for Guid
  • Bulk writing for int
  • Bulk writing for long
  • Bulk writing for string
  • Include usage for Guid
  • Include usage for int
  • Include usage for long
  • Include usage for string
  • int, long, Guid, string usage for pattern new SomethingId(inner)
  • StrongTypedId usage
  • JSON serialization uses only the value
  • Select(x => enumerable of value types)

@nkosi23
Copy link

nkosi23 commented Jun 14, 2024

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:

  • if the destination type is abstract, check if it has a single sealed nested non-abstract class. If this is the case, use the nested class as the deserialization type
  • if the destination type is abstract, check if it has a single sealed nested non-abstract class (same as above), but this time use the static factory method to instantiate the id type.

@jeremydmiller
Copy link
Member

@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
Copy link

nkosi23 commented Jun 18, 2024

@jeremydmiller Great I will give it a try :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests