Marten: Event Store broken with NodaTime

[lngr]

It seems the Event Store is broken when using NodaTime:

System.InvalidCastException: Can't cast database type timestamp with time zone to DateTime
   at Npgsql.NpgsqlDefaultDataReader.GetFieldValue[T](Int32 column) in C:\projects\npgsql\src\Npgsql\NpgsqlDefaultDataReader.cs:line 104
   at Marten.Events.StreamStateByIdHandler`1.Resolve(DbDataReader reader, IIdentityMap map, QueryStatistics stats)
   at Marten.Services.CommandRunnerExtensions.<>c__DisplayClass2_0`1.<Fetch>b__0(NpgsqlCommand c)
   at Marten.Services.ManagedConnection.Execute[T](NpgsqlCommand cmd, Func`2 func)

The call was session.Events.FetchStreamState(streamId);

This is Marten 3.5.0 with Marten.NodaTime 1.0.0.

Looking at the source code, the reason seems to be several casts to DateTime, where DateTimeOffset would (maybe?) be a more appropriate option.

https://github.com/JasperFx/marten/blob/master/src/Marten/Events/StreamStateByGuidHandler.cs#L81

Unfortunately, DateTime is used at several places through the public interface of the event store (including IEventStore), so I'm not sure if this can easily be fixed...

[oskardudycz]

@lngr I'll check it asap. I'll keep you posted.

[lngr]

I did some tests. Changing DateTime to DateTimeOffset in the file above is not sufficient. The GetFieldValue requires a NodaTime type.

I think the calls to Resolve and ResolveAsync require a conversion strategy that is replaced by UseNodaTime().

(In StreamStateByGuidHandler, and probably also StringIdentifiedEventSelector and other places where GetFieldValue<DateTime> / GetFieldValue<DateTimeOffset> is used.

[oskardudycz]

Thanks for investigation. I think that it might end up with the need to extend that methods to be able to inject other date types (in this case taken from NodaTime).

[tetious]

With the current draft PR, the EventStore async daemon is failing when the store is set to UseNodaTime, with the following:

System.NotSupportedException: The CLR type System.TimeSpan isn't natively supported by Npgsql or your PostgreSQL. To use it with a PostgreSQL composite you n
eed to specify DataTypeName or to map it, please refer to the documentation.
   at Npgsql.TypeMapping.ConnectorTypeMapper.GetByClrType(Type type) in C:\projects\npgsql\src\Npgsql\TypeMapping\ConnectorTypeMapper.cs:line 144
   at Npgsql.NpgsqlParameter.ResolveHandler(ConnectorTypeMapper typeMapper) in C:\projects\npgsql\src\Npgsql\NpgsqlParameter.cs:line 529
   at Npgsql.NpgsqlCommand.ValidateParameters() in C:\projects\npgsql\src\Npgsql\NpgsqlCommand.cs:line 796
   at Npgsql.NpgsqlCommand.ExecuteDbDataReader(CommandBehavior behavior, Boolean async, CancellationToken cancellationToken) in C:\projects\npgsql\src\Npgsql
\NpgsqlCommand.cs:line 1141
   at Marten.Events.Projections.Async.Fetcher.buildEventPage(Int64 from, NpgsqlCommand cmd)
   at Marten.Events.Projections.Async.Fetcher.fetchNextPage(Int64 lastEncountered)
   at Marten.Events.Projections.Async.Fetcher.<>c__DisplayClass26_0.<<FetchNextPage>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Marten.Events.Projections.Async.ErrorHandling.DaemonErrorHandler.TryAction(Func`1 action, IMonitoredActivity activity, Int32 attempts)
System.NotSupportedException: The CLR type System.TimeSpan isn't natively supported by Npgsql or your PostgreSQL. To use it with a PostgreSQL composite you n
eed to specify DataTypeName or to map it, please refer to the documentation.
   at Npgsql.TypeMapping.ConnectorTypeMapper.GetByClrType(Type type) in C:\projects\npgsql\src\Npgsql\TypeMapping\ConnectorTypeMapper.cs:line 144
   at Npgsql.NpgsqlParameter.ResolveHandler(ConnectorTypeMapper typeMapper) in C:\projects\npgsql\src\Npgsql\NpgsqlParameter.cs:line 529
   at Npgsql.NpgsqlCommand.ValidateParameters() in C:\projects\npgsql\src\Npgsql\NpgsqlCommand.cs:line 796
   at Npgsql.NpgsqlCommand.ExecuteDbDataReader(CommandBehavior behavior, Boolean async, CancellationToken cancellationToken) in C:\projects\npgsql\src\Npgsql
\NpgsqlCommand.cs:line 1141
   at Marten.Events.Projections.Async.Fetcher.buildEventPage(Int64 from, NpgsqlCommand cmd)
   at Marten.Events.Projections.Async.Fetcher.fetchNextPage(Int64 lastEncountered)
   at Marten.Events.Projections.Async.Fetcher.<>c__DisplayClass26_0.<<FetchNextPage>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Marten.Events.Projections.Async.ErrorHandling.DaemonErrorHandler.TryAction(Func`1 action, IMonitoredActivity activity, Int32 attempts)

[tetious]

Added PR https://github.com/JasperFx/marten/pull/1248 to fix this and a couple other problems.

[oskardudycz]

@tetious my PR is just draft in progress, I’m waiting for the @jeremydmiller thoughts about way to solve the case. It’s unfortunately a bit more complicated.

Potential options that I see:

1. Quickest - provide some ugly fix for mapping NodaTime types into DateTime/DateTimeOffset for our internal fields. It would look more or less how I did in my PR#1246
2. Add some custom handler that will suport both NodaTime types and CLR dates. This solution is not ideal. The default NodaTime datetime handlers are internal, so I can't reuse them. I could write our own custom datetime mapper, but then we'll probably end up with the case as we had with Npgsql 4.x and TypeMappings. It would be probably painful maitainance. Maybe it would be possible to disccuss with @roji to make it public, but I'm not sure if that'd be good, as probably the wisest is to have opt out.
3. Setup our Marten code to be able to handle different DateTime types. That's not easy as it'd be needed to have generic type. So that will imply probably adding this parameter to DocumentStore etc. And it would be huge change.

Thank you for your feedback and PR. If we decide to go with first approach then I’ll gladly use it.

[roji]

I've just noticed this, but I don't have much context. Can you guys provide a bit more info on why both NodaTime and CLR date/time types are required on the same connection?

I don't have any particular problem with making the NodaTime handlers public - some handlers are public and some aren't, it's currently a bit arbitrary. I do have some concerns with people starting to code against the type handler API, which - although it's public - isn't as stable as the rest of the Npgsql public API surface. Specifically, we may be making perf-related improvements which may require changing how type handlers look.

[oskardudycz]

@roji thanks for jumping in 👍

In few places in our database structure we're using DateTime and DateTimeOffset so timestamp and timestamptz. Mostly for our metadata columns.

So our events table structure is:

-- Table: events.mt_events

-- DROP TABLE events.mt_events;

CREATE TABLE events.mt_events
(
    seq_id bigint NOT NULL,
    id uuid NOT NULL,
    stream_id uuid,
    version integer NOT NULL,
    data jsonb NOT NULL,
    type character varying(500) COLLATE pg_catalog."default" NOT NULL,
    "timestamp" timestamp with time zone NOT NULL DEFAULT now(),
    tenant_id character varying COLLATE pg_catalog."default" DEFAULT '*DEFAULT*'::character varying,
    mt_dotnet_type character varying COLLATE pg_catalog."default",
    CONSTRAINT pk_mt_events PRIMARY KEY (seq_id),
    CONSTRAINT pk_mt_events_id_unique UNIQUE (id),
    CONSTRAINT pk_mt_events_stream_and_version UNIQUE (stream_id, version),
    CONSTRAINT mt_events_stream_id_fkey FOREIGN KEY (stream_id)
        REFERENCES events.mt_streams (id) MATCH SIMPLE
        ON UPDATE NO ACTION
        ON DELETE CASCADE
)

So mostly event id (id column), event data stored in jsonb (data column) rest are more or less metadata. One of it is timestamp column that stores event creation date. Our .NET code and structures reflects this code. So we have currently DateTime field reflecting this structure and code responsible for loading these data (see eg. here: https://github.com/JasperFx/marten/pull/1246/files#diff-505b09278bffc44aa44b3694ac3a9feeR51).

This are our internals, that normally users don't use manually, but they might if they want to do some stuff with these metadata. If we switch to NodaTime, then they'll stay as DateTime or we need to find some way how to allow to switch those types. This is not so easy, as it's not easy to change core internals. Some potential options how to solve I described in previous message.

Do you have some other suggestions or ideas?

[tetious]

After thinking on this, my preference would be option 3, though slightly modified.

As I see it, there are two scenarios we'll need to consider:

1. Internal use of the metadata fields, such as the async daemon and a few other places.
2. API consumer use, which is totally undefined.

I think it is safe to assume that if someone opts in to NodaTime, they would like access to the metadata columns via NodaTime types. So I don't think it would be required to make a separate configuration option.

I'm not super familiar with the codebase, but my impression is that the date fields aren't used that much by Marten itself. I think we could encapsulate those few use cases with some kind of proxy object much like you describe, maybe with an implicit conversion to DateTime/Instant so they can automatically map to a POCO that has a property with the expected type. Also, for some, we may be able to eliminate the need for mapping entirely, like with the async daemon fix in my PR.

I haven't actually looked at implementing this yet, so the idea may not pan out. :)

[roji]

This are our internals, that normally users don't use manually, but they might if they want to do some stuff with these metadata. If we switch to NodaTime, then they'll stay as DateTime or we need to find some way how to allow to switch those types. This is not so easy, as it's not easy to change core internals. Some potential options how to solve I described in previous message.

I obviously have no idea about how Marten works, but is it not possible for you to detect whether the NodaTime plugin is in use, and use the NodaTime types if so? That sounds like option 3 in your list above, which does sound best but seems to be a big change for you (you write that you'd need some sort of generic type, I'm not sure why that is but I have no context/knowledge).

Writing a new type mapper which would handle both doesn't sound like a good way forward to me... You would have to maintain it (and again, I expect the type handler API to break from time to time). Also, part of the idea of plugins is to not force the NodaTime assembly on users if they don't want it - which this would do.

Just to give a bit of context on why it's difficult to make both BCL and NodaTime types coexist on the same connection... At the moment a given PostgreSQL type (e.g. timestamp) has a single handler associated with it; when we get back row description for a resultset, we look up the handler based on the type OID PostgreSQL sends us. It's a simple and efficient system - it's done once before resultset handling even starts. If we need to support two handlers, that means we need to switch on which CLR type the user requests (NodaTime or BCL), making the lookup more expensive, and possibly forcing us to do it every time the user reads a column (as opposed to once for the entire resultset).

So I'd rather avoid going into this unless we see evidence that a lot of users need it...

[jeremydmiller]

@roji We'd have to make Marten deviate quite a bit and branch how we generate database objects based on whether it's DateTime or NodaTime. Not only that, but we'd need to deviate .Net objects for both DateTime and NodaTime flavors. Probably a whole lot of generic types that are going to make Marten's event store be quite a bit less usable.

WTF would you make this all or nothing?

[roji]

WTF would you make this all or nothing?

I'm not sure I understand - are you asking why we would make this all or nothing, i.e. either NodaTime or BCL? Well, wanting to mix several CLR types as a representation for a single database type, in a single application, does seem a bit... extreme. Don't forget that until recently NodaTime wasn't even supported at all (and Npgsql is still the only driver which does it, AFAIK). This limitation likely affects only cases with layers above Npgsql (like Marten), as a regular application is really unlikely to need this. While we've had one or two complaints since NodaTime support was released, I don't think it's been very considerable.

I understand this is a frustrating situation. I've opened https://github.com/npgsql/npgsql/issues/2439 to think about this some more, but it definitely wouldn't happen in the immediate future. If there's no good solution on your end, I guess the only thing is to say that NodaTime isn't supported.

[lngr]

I think the public interface of Marten can stay with the standard CLR types, since the NodaTime types can easily converted to those. It's only the actual database fetching where one would first fetch to NodaTime types due to the way NodaTime's Npgsql-implementation works.

[roji]

@Ingr I may be misunderstanding things, but at the Npgsql level NodaTime is a purely optional feature, activated only when you set up a type mapping plugin. You can just as well not activate NodaTime and just use BCL types.

[lngr]

@roji Yes, the error happens when using the Npgsql NodaTime support and Marten's Npsgsql NodaTime plugin.

[oskardudycz]

For version 3.6.0 I'll try to prepare mapping from NodaTime types into DateTime for our internal clr dates usage.
For the version 4.0.0 we should think about some more modularity and different approach to our metadata.

Btw. @roji would you consider allowing to pass type as parameter instead of generic method for the reader.GetFieldValueAsync? That could be helpfull for such internal usages. I could try to prepare such PR.

[roji]

Btw. @roji would you consider allowing to pass type as parameter instead of generic method for the reader.GetFieldValueAsync? That could be helpfull for such internal usages. I could try to prepare such PR.

There's no such API in ADO.NET, and there may be some issues around such a non-generic API... It would have to return object (so boxing all value types it returns), and Npgsql is currently architectured to be generic all the way down, precisely to keep things fast.

However, why not simply generate compiled delegates at runtime which call GetFieldValueAsync<T>() with the type you want? It's quite easy to do, and if you cache the delegates properly the perf impact is minimal.

[oskardudycz]

@roji thanks for explanation. I understand, I'll provide a wrapper if it'll be needed. Thanks.

[roji]

Great. Note that I did open https://github.com/npgsql/npgsql/issues/2439 to think about supporting multiple handlers for the same type (i.e. BCL and NodaTime). I'd really appreciate it if you guys could tell me whether you've found a more or less satisfactory workaround, or whether this capability is absolutely necessary for you - because you're the only users who've really requested it (and haven't indicated it's fine without it).

[oskardudycz]

Thanks @roji. I'll be probably working on that the next weak. I'll send then my feedback/suggestions.

[oskardudycz]

with a help from @tetious I managed to provide workaround solution. See details in https://github.com/JasperFx/marten/pull/1246

[jeremydmiller]

Fixed by the work in https://github.com/JasperFx/marten/pull/1246