neo4j / neo4j-ogm

Java Object-Graph Mapping Library for Neo4j
https://neo4j.com/docs/ogm-manual/
Apache License 2.0
337 stars 165 forks source link

DateStringConverter should convert empty String to null Date #424

Closed gusehr closed 6 years ago

gusehr commented 7 years ago

Expected Behavior

IMHO, when my node have an empty property mapped to a Date field, the DateStringConverter should return a null Date object.

What dou you think?

Current Behavior

When my node have an empty property mapped to a Date field, the DateStringConverter throws an exception because it tries to parse the empty String.

Your Environment

nmervaillie commented 7 years ago

Do you mean a non existing property or a property containing an empty String ?

gusehr commented 7 years ago

Hi @nmervaillie I mean a property containing an empty String.

nmervaillie commented 7 years ago

I'm mixed about this because an empty string might be a symptom of an error in a previous saving / import, and hiding this wouldn't fix the root cause. I usually like fail fast.

But I guess it depends on the use case. Can you tell a bit more about yours ?

Maybe we could add an attribute to the annotation strictMode=true/false.

jasperblues commented 7 years ago

While fail fast quickly communicates that something is wrong, if its a choice between two behaviors, I prefer the OGM to be forgiving:

gusehr commented 7 years ago

@nmervaillie My use case is actually a data import, and I can't change the data source. I can workaround this issue creating my own Converter, but I think that this could be the default behavior.

I know that from a "strict" point of view, the empty String should communicate an error. But from a "practical" point of view, the empty String represents a null Date.

An annotation configuration can solve the issue.

@jasperblues I agree that the default behavior should be less strict and more lenient.

meistermeier commented 6 years ago

We discussed this and have come to the conclusion that we want to keep the current behaviour but add a property (lenient) to the string-based annotation currently available (@DateString, @NumberString and @EnumString). In the current version this will handle blank strings as null values when reading those from the database.