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

Date properties are stored as string instead of temporal #441

Open
machosec opened this issue Apr 24, 2019 · 6 comments
Open

Date properties are stored as string instead of temporal #441

machosec opened this issue Apr 24, 2019 · 6 comments

Comments

@machosec
Copy link
Contributor

machosec commented Apr 24, 2019

Neomodel's date property types (DateProperty, DateTimeProperty and DateTimeFormatProperty) are inflated and stored as string values in neo4j instead of temporal values (https://neo4j.com/docs/cypher-manual/current/syntax/temporal/).
Using string instead of date/time types ruin many filtering/sorting capabilities which I guess are not desired.

@aanastasiou
Copy link
Collaborator

@machosec I think that what you may be pointing to here is that neomodel does not use a standardised datetime format when storing dates and/or times (?).

With server-side queries, you can always use the functions that cast a date bearing string to a date if you wish to extract specific parts of the date (e.g. in a "group by month" scenario) and at client-side, the date has already been turned into a datetime during filtering.

Is there a more specific example of any failures this might be leading to?

@machosec
Copy link
Contributor Author

Hi @aanastasiou ,
Neo4j version 3.4 introduced new temporal types and functions to better deal with dates.
You can see the example below where the date() function (https://neo4j.com/docs/cypher-manual/current/functions/temporal/date/) returns a temporal type that is treated differently than just a string type. New temporal functions like duration() won't work on a date represented as a string.

image

@aanastasiou
Copy link
Collaborator

@machosec I understand.

That would be the difference between strptime() and neo4j.v1.types.temporal.Date, etc at this point and of course, the inverse for deflate(). I would have to look more closely the data types that the neo4j-driver returns for dates as it might already be working with straightforward datetimes too.

Unfortunately, the DateTimeFormatProperty might not be as easily handled if it is impossible to represent partial dates.

Feel free to submit a PR if you can handle this.

@machosec
Copy link
Contributor Author

The neotime module used by neo4j-driver support the new temporal types as described here: https://neo4j.com/docs/api/python-driver/current/types/temporal.html
I can try to change the inflate and deflate logic of DateProperty and DateTimeProperty to properly store temporal types in neo4j without breaking the existing implementation

@aanastasiou
Copy link
Collaborator

@machosec That's fine, but if you think you are going to be applying what feels like bigger changes, can you please have a look at this ?.

If you are making big changes, it might be good to see what else could be taken care of at the same time, since you are getting into this "trouble". Note, I am not telling you to do more than providing this feature, I am just telling you that there are a few things open already. If the change you do could also work for something else, that would be great, let's take that into account as early as possible.

All the best

@Fedorov113
Copy link

Any update on this?

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

No branches or pull requests

3 participants