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.

Sign up for GitHub

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

Jump to bottom

How to use NodaTime to interact with PostgreSQL date/time types? #1027

Closed
AKlaus opened this issue Jun 6, 2018 · 30 comments
Closed

How to use NodaTime to interact with PostgreSQL date/time types? #1027

AKlaus opened this issue Jun 6, 2018 · 30 comments
Assignees
@oskardudycz
Labels
enhancement
Milestone
3.5.0

Comments

@AKlaus
Copy link

AKlaus commented Jun 6, 2018

The NodaTime plugin is the recommended way to interact with PostgreSQL date/time types. However, we can't use it along with Martin due to:

  1. The recommended way to use NodaTime is by adding Npgsql.NodaTime plugin
  2. Npgsql.NodaTime is using Npgsql v4, when is Marten is still on Npgsql v3.x
  3. Adding a reference to Npgsql v4 breaks Martin as its API has changed.

Therefore, would be possible to upgrade the Npgsql dependency to v4? Or could you recommend another way of using NodaTime to interact with PostgreSQL date/time types?
@jeremydmiller
Copy link
Member

@AKlaus There's already a pull request (with some breaking tests) to update us to Npgsql 4 and that'll be ready soon. As for NodaTime w/ Marten, just be aware that you may not be able to use Linq against the NodaTime objects w/o some customizations.

A way to do this now is to still have raw DateTime or DateTimeOffset values persisted in the JSON serialization and do the NodaTime object creation in your document objects

@mhov mhov mentioned this issue Jul 13, 2018
Closed
@roji
Copy link

roji commented Jul 20, 2018

NodaTime support is new and was only added in Npgsql 4. Until this new version is supported, continue using regular DateTime as before.

@SeanFarrow
Copy link
Contributor

Is npgsql v4 support as of marten v2.9+?

@mysticmind
Copy link
Member

mysticmind commented Aug 20, 2018 via email

@SeanFarrow
Copy link
Contributor

SeanFarrow commented Aug 21, 2018 via email

@jeremydmiller
Copy link
Member

MEF? Ick;)

You might be able to query as is on properties of the NodaTime objects themselves, or this: http://jasperfx.github.io/marten/documentation/documents/advanced/customizing_linq/

@SeanFarrow
Copy link
Contributor

SeanFarrow commented Aug 21, 2018 via email

@oskardudycz
Copy link
Collaborator

I think that without changes to Marten codebase it won’ Work, I tried some time ago. I’ll revisit that while we have now new Npgsql with NodaTime support.

@oskardudycz oskardudycz added this to the 4.0 milestone Sep 28, 2018
@SeanFarrow
Copy link
Contributor

SeanFarrow commented Sep 28, 2018 via email

@oskardudycz
Copy link
Collaborator

oskardudycz commented Sep 28, 2018 •
edited
Loading

When I was checking it the last itme the biggest issue lied with the Expressions in .NET. They're strongly typed and even if at the end it'd be to construct proper query then it was stating that PG doesn't know this type. So at that time Npgsql was "biggest blocker". Now it's resolved and I can continue.

I'm thinking about giving some extensible mechanism for TypeMappings - for sure there some tweaks will be needed.

But to sum up - it's doable. I'm using NodaTime in my commercial project, so I'm personally interested on that ;) I'll try to work on that asap.

@SeanFarrow
Copy link
Contributor

SeanFarrow commented Sep 28, 2018 via email

@oskardudycz oskardudycz self-assigned this Sep 28, 2018
@oskardudycz
Copy link
Collaborator

I placed the issue on Npgsql that would be imho good first step to push it forward.

@oskardudycz
Copy link
Collaborator

Just to let you all know I'm working right now on the initial support for NodaTime, it will take me some time, as there are few places that I need to update and align implementation. I plan to do it simmilarly as Npgsql as new package Marten.NodaTime. I hope that I'll be able to provide PR for within one week.

@oskardudycz
Copy link
Collaborator

It's still in progres, but feel free to comment the main idea in WIP pull request https://github.com/JasperFx/marten/pull/1154/files

@oskardudycz oskardudycz mentioned this issue Dec 17, 2018
Merged
@oskardudycz oskardudycz modified the milestones: 4.0, 3.5.0 Feb 21, 2019
@oskardudycz
Copy link
Collaborator

oskardudycz commented Feb 21, 2019 •
edited
Loading

As I got back to NodaTime support topic. It appears to be an issue with supporting DateTime handling when NodaTime is turned on. So I'd like to have to go with the same way as it's on Npgsql - so you use either DateTime or NodaTime. EF behaves the same afaik.
Is anyone against that?

It comes from the Npgsql implementation. The default 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.

@SeanFarrow
Copy link
Contributor

SeanFarrow commented Feb 21, 2019 via email

@oskardudycz
Copy link
Collaborator

oskardudycz commented Feb 21, 2019 •
edited
Loading

I plan to release new package: Marten.NodaTime. It will add new extension method to store options. You could do just:

DocumentStore.For(_ =>
            {
                _.UseNodaTime();
            });

When you did that then you won't be able to use DateTime, you'll need to use LocalDateTime, LocalDate, Instant etc.

So you'll opt out from using .NET (date) time types.

Oh well, saving and loading document will work, querying by dates might fail. Event query like:

query.Query<TargetWithDates>().FirstOrDefault(d => d.DateTime == dateTime)

should work, but greater or greater or equal or any more sophisticated query might fail. In general we won't guarantee that DateTime will work.

Migration won't be needed in most cases, only Types replacement in .NET code. The only migration case that I see is when you used collection with interface in your model - so eg. IEnumerable<DateTime>. If that's an issue, I could prepare some migration tool for that case.

This concept is aligned with how it's done on Npgsql

@SeanFarrow
Copy link
Contributor

SeanFarrow commented Feb 21, 2019 via email

@oskardudycz
Copy link
Collaborator

It will setup everything what's needed for Marten to use NodaTime. So eg. JsonNetSerializer, JsonLocator, etc.

See more in my WIP PR: https://github.com/JasperFx/marten/pull/1154/files#diff-53eb368e074036b2639ff3d740492e30R12

Even if I didn't add this extension method, then you'd still add reference to the Npgsql.NodaTime package and call it somewhere.

@SeanFarrow
Copy link
Contributor

SeanFarrow commented Feb 21, 2019 via email

@oskardudycz
Copy link
Collaborator

The queries will work, exactly the same as they work for DateTime currently. Npgsql made the work for custom type mapping, and proper replacing DateTime with NodaTime. See more here: https://www.npgsql.org/doc/types/nodatime.html

@SeanFarrow
Copy link
Contributor

SeanFarrow commented Feb 21, 2019 via email

@oskardudycz
Copy link
Collaborator

Yes, they should work in both (of course Marten Queryable, have some limitation in dates queries, but this shouldn't be different than current DateTime support).

@roji
Copy link

roji commented Feb 22, 2019

Hi guys, just to say that I have my eyes on this discussion. I've received a complaint or two about the inability to mix both NodaTime and BCL date/time types in the same application, but up to now it doesn't seem actually blocking for users. If you have any input or feedback saying otherwise, I'll be happy to listen.

@oskardudycz
Copy link
Collaborator

oskardudycz commented Feb 22, 2019 •
edited
Loading

@roji thank you for the response!

TBT I'm not sure if that'll be huge issue for the users. I asked on this issue, on our gitter, @jeremydmiller asked on Twitter, to get initial feedback. Currently noone complains (which is good), but not all users are active on those channels.

Imho it might be an effort to refactor the code from .NET types to NodaTime. Gradual update won't be possible because out of the box in Npgsql they can't coexist. Also because we're storing our collections serialized with TypeNames (to be able to deserialize interfaces collections like IEnumerable, IReadonlyCollection etc.) then some data migration will be needed. As a user I'd prefer to have an option but I'm not sure if it's something nessesary.

I plan to release the first version of our Marten.NodaTime plugin with this hard opt-out rule (so same as Npgsql). If the users will complain that's problematic then I can prepare custom TypeHandler that will support both, although I would like to avoid that if possible, as it will be hard to mantain (as I can't reuse the Npgsql code because it's currently internal).

@oskardudycz
Copy link
Collaborator

Btw. for all interested. My PR is almost done #1154, I'm waiting for approvals from @jeremydmiller @mysticmind @jokokko. If they give green light then I'll probably add more tests and it will be ready to be released.

@roji
Copy link

roji commented Feb 22, 2019

@oskardudycz thanks, keep me posted. If it's really needed I can take a look at what it would involve with the Npgsql type handler model.

@SeanFarrow
Copy link
Contributor

@roji,
Generally, it is not best practice to share datetime api's, people should choose one and stick to it.

@oskardudycz
Copy link
Collaborator

Thank you @roji, I appreciate that 👍

@oskardudycz
Copy link
Collaborator

PR #1154 with NodaTime support was merged. It will be available in version 3.5.0 that should be released within a week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@oskardudycz oskardudycz

Labels
enhancement
Projects
None yet
Milestone
3.5.0
Development

No branches or pull requests
6 participants
@jeremydmiller @SeanFarrow @roji @mysticmind @AKlaus @oskardudycz