JasperFx / marten

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

V7 IMemberSource Seems to not be used #3148

Closed CptWesley closed 4 months ago

CptWesley commented 7 months ago

For 7.8.0:

Attempting to perform a query that contains a custom type will (logically) result in the following exception:

Marten.Exceptions.BadLinqExpressionException:
Marten cannot support custom value types in Linq expression.
Please query on either simple properties of the value type, or register a custom IMemberSource for this value type.

However, implementing the IMemberSource interface and registering it in the StoreOptions.Linq.MemberSources does not appear to actually result in the TryResolve being called. In previous versions we used to implement the IFieldSource interface and then register them using the StoreOptions.Linq.FieldSources, which worked fine. However, this interface appears to now have been removed/renamed in favor of IMemberSource. When inspecting the source code is seems that the TryResolve method and the .MemberSources have 0 references.

jeremydmiller commented 7 months ago

Nobody cared until you did:) Just got missed in the giant V7 LINQ rewrite work

JurJean commented 7 months ago

Why not pass queried parameters through the injected json serializer? So it will always match with the stored document? Instead of this interface that does basically the same.

CptWesley commented 7 months ago

@JurJean Not all possible pg types have a direct equivalent json type. Date/time-like types are usually stored as some string in json, but you probably would want to treat them as some timestamp type when querying, so you would also need some casting/conversion logic wrapped around that conversion.

CptWesley commented 7 months ago

@jeremydmiller is there currently any alternative way of supporting custom types in marten v7? Specifically, we have some strongly typed reference identifiers that for all intents and purposes should just be treated as varchar.

If not, are there any plans or desires to support this again in the (hopefully near) future?

EDIT: For now I figured out that implementing the type mapping fully in Npgsql seems to also make it work with weasel/marten.

jeremydmiller commented 6 months ago

@CptWesley That's what IMemberSource is meant to do in the first place. The strong typed identifier stuff is still in the backlog, but it's not top of mind for me anytime soon tbh. I'd say that I'd be happy to take a pull request for that, but it's a ton of work that's going to take someone willing to get into internals.

Just getting Marten to use IMemberSource should be pretty simple though if you're interested in taking that on as a PR. Dig into StoreOptions.CreateQueryableMember if you're up to that.

CptWesley commented 6 months ago

@jeremydmiller I was looking into doing that last night, but I ran into the issue that if I only handle it in the CreateQueryableMember location it would still fail somewhere further down the line because it was missing the corresponding Npgsql type resolvers. I previously also ran into some issues with the old IFieldSource interface that there were some specific sequences of actions that would lead to the same errors of missing Npgsql mappings.

Since from my quick tests Marten seems to properly handle the custom type mappings that are added to Npgsql (and they are required to work anyway), I am not sure what the value is of keeping the IMemberSource. There might of course be other places where not implementing the IMemberSource might break, but I am not yet that familiar with the code base and the exact flows through the library.

Ideally, I would personally like to have an interface that behind the scenes (also) registers the right mappings in the NpgsqlDataSourceBuilder such that it is possible to add support for type mappings that work everywhere in the chain in a single place. That is perhaps a bit more complicated by the fact that the StoreOption.Connection(NpgsqlDataSource) method takes a built data source that no longer allows changing the type mapping (I think?). But I am also not certain if that is something that should exist in Marten specifically or if this should solely be an Npgsql concern.

If I can help anywhere (and can find the time to do so) I would like to. Currently I'm not sure exactly what the "Marten" vision is in this regard. So let me know what your thoughts are :)

jeremydmiller commented 6 months ago

@CptWesley TBH, you're pushing this kind of thing much farther than I was thinking about anyway -- other than a notion that I'd eventually get around to the strong typed identifier support. The IQueryableMember interface also includes the parameter type information too, so I'm not sure a one place hook matters.

I saw some of the Weasel stuff. The PR looks fine, but if you could add some tests just to lock down the behavior, that would be helpful

But, just to make this more complicated, I'd also say you need to do something about the JSON serialization for these custom value types. Can you add some context about what your custom value types look like before we do too much more?

CptWesley commented 6 months ago

@jeremydmiller For some more context: In production we are using a bunch of (single) value object definitions from this library: https://github.com/Qowaiv/Qowaiv in order to have more strongly typed/meaningful types rather than using primitives everywhere. This includes types such as Date instead of a string, integer or DateTime (could be replaced by DateOnly that was introduced in .NET 6). MonthSpan instead of int, Amount instead of decimal, etc. So this is indeed different than the strongly-typed identifiers (which we also have, but we use a wrapper document with a string identifier). Previously we were using MongoDB where it was enough to register a BSON serializer/deserializer in order for the LINQ queries to work.

We recently started moving to Postgres with Marten and in order for the queries to work properly we are now setting/defining the following 3 things:

To simplify things, I have written an abstract IPgTypeInfoResolver implementation, let's call that SimpleResolver<TCustom, TNative>. Here TCustom is the custom type and TNative is the underlying type that it should be treated as. e.g. for Amount we would extend SimpleResolver<Amount, decimal>. The abstract class requires us to implement only two methods:

Under the hood this abstract class simply converts the type to the "native" type and then just uses whatever built-in type resolver Npgsql has built-in for that native type. This allows us to create IPgTypeInfoResolver implementations in a relatively easy way (since the original interface is quite convoluted).

This makes StoreOptions.CreateQueryableMember fall back to the BooleanMember or SimpleCastMember which create the proper queryables. Having registered the proper IPgTypeInfoResolvers also results in the value being converted to the proper query argument syntax. So far this seems to be working fine, with the exception of custom types with generic arguments (https://github.com/JasperFx/weasel/issues/128).

So to me it seems right now that IMemberSource in it's current state is redundant (besides it currently not being used at all of course). But I might be missing some scenario where the Npgsql route does not work.

Once I feel a bit better I could write some tests for Marten to ensure that the Npgsql IPgTypeInfoResolver route keeps working.

jeremydmiller commented 5 months ago

@CptWesley Would you mind pasting in your IPgTypeInfoResolver implementation here? I'm finally working on the strong typed identifier support for Marten -- and cursing all of you who have asked for that over the years because the complexity of accommodating that through every possible permutation of usage is being insane.

And no, "just" using an IPgTypeInfoResolver doesn't help all that much because you need to match the JSON serialization to the underlying value. It's definitely part of the answer, but not all of it by any means.

jeremydmiller commented 4 months ago

And this is done now, but do need a blog post or example or something. Not stressing about this too much. Comes in as part of the strong typed identifier work.

CptWesley commented 4 months ago

@jeremydmiller sorry for the late reply, I haven't been near a computer for a while. Unfortunately, the code I mentioned earlier is owned by my employer, so I can not share it directly. If it's still useful I might be able to write a different example at some point in the future