Empty datetime will fail when returning results #3169
Conversation
If the stored value of datetime predicate is empty, types.Marshal will return empty when converting to binary. When we return the result the empty binary is converted to datetime, this causes time.UnmarshalBinary() to fail.
The zero-time value is an internal values that should be exposed to the client.
Added tests to preserve the behavior with empty values.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @srfrog)
query/outputnode.go, line 165 at r1 (raw file):
t := v.Value.(time.Time) if t.IsZero() { return []byte(`""`), nil
Should this be []byte("")? I am not entirely sure but I think quotes are escaped inside multi-line strings so this is the equivalent to a string containing two quotes.
types/conversion.go, line 255 at r1 (raw file):
{ var t time.Time // We must inverse the binary->datetime for zero-time values.
Can you clarify this comment? I don't understand what it means.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr)
query/outputnode.go, line 165 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Should this be
[]byte("")? I am not entirely sure but I think quotes are escaped inside multi-line strings so this is the equivalent to a string containing two quotes.
[]byte("") is equivalent to empty string which would result in JSON null value. I'm converting zero datetime to JSON empty string. So the question is whether we want to return null or "" for these. JS Date.parse() will return NaN for both, but in JS usually it is simpler to check typeof value === "string", whereas null will have a JS type of object.
types/conversion.go, line 255 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Can you clarify this comment? I don't understand what it means.
It was in reference to https://github.com/dgraph-io/dgraph/blob/master/types/conversion.go#L75
I will indicate that.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Empty string for a datetime should be rejected during mutation. That's the right behavior.
Reviewable status:
There was a problem hiding this comment.
We changed that behavior in types.Convert(). This bug was just a missed case. The behavior was changed because users couldnt export their data.
Reviewable status:
…ersions Conversion from binary to datetime should happen for value length greater than zero. Convertion from empty string to int or float results their zero value. Conversion from datetime binary value (stored) will unmarshal only if the length is correct. Fixed a panic when bool is invalid, which obscures the real reason.
The edge test cases are for unexpected values that must be handled nicely. Updated float and int tests when converting from empty string value.
There was a problem hiding this comment.
One comment. Address that please. Thanks for putting together the edge cases.
Reviewed 1 of 3 files at r1, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @srfrog)
types/conversion.go, line 84 at r3 (raw file):
// Only try to convert binary data with length > 0. If the value is null, // convert to zero-time value. if len(data) != 0 {
if len(data) > 0 would be consistent with the comment above.
types/conversion.go, line 270 at r3 (raw file):
// marshaling and return the ztv. Then we can handle it better if we need to return it // in a result. if len(data) == 15 {
Do not assume the size of the data. Work with whatever is available to you.
…errors instead Remove all handling of default values for types that can return error when syntax fails. Update the tests to check for behavior.
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @manishrjain and @srfrog)
types/conversion.go, line 197 at r4 (raw file):
*res = vc != 0
Before all your changes, this used to be *res = bool(vc != 1). Why did it change? Was this a bug?
types/conversion_test.go, line 88 at r4 (raw file):
Value: nil},
Tests with failure do not need to have a value. Can you set all the Values to nil (or omit them) since they are not used anywhere.
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @manishrjain and @martinmr)
types/conversion.go, line 84 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
if len(data) > 0would be consistent with the comment above.
Done.
types/conversion.go, line 270 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Do not assume the size of the data. Work with whatever is available to you.
Done.
types/conversion.go, line 197 at r4 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
*res = vc != 0Before all your changes, this used to be
*res = bool(vc != 1). Why did it change? Was this a bug?
I think it was. Why would 0 != 1 == true make sense ? Also the type cast was unnecessary.
types/conversion_test.go, line 88 at r4 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Value: nil},Tests with failure do not need to have a value. Can you set all the Values to nil (or omit them) since they are not used anywhere.
This test is not trying to test typical usage. There are several tests for that already.
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @martinmr, and @srfrog)
types/conversion_test.go, line 88 at r4 (raw file):
Previously, srfrog (Gus) wrote…
This test is not trying to test typical usage. There are several tests for that already.
it's not about that. It doesn't seem like the elements that contain a failure ever need to look at the Value so it should be safe to omit them since the only thing out is being used is to lookup the type needed for the conversion.
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @manishrjain and @martinmr)
types/conversion_test.go, line 88 at r4 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
it's not about that. It doesn't seem like the elements that contain a failure ever need to look at the Value so it should be safe to omit them since the only thing out is being used is to lookup the type needed for the conversion.
You are correct, the failure disables the value check. I included them for completeness for someone that is reading the tests to get familiar with the code. But I can remove them.
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @martinmr, and @srfrog)
types/conversion_test.go, line 88 at r4 (raw file):
Previously, srfrog (Gus) wrote…
You are correct, the failure disables the value check. I included them for completeness for someone that is reading the tests to get familiar with the code. But I can remove them.
I don't think they add much. In fact, they might confuse someone by making them think the value is relevant.
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @martinmr, and @srfrog)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @manishrjain, @martinmr, and @srfrog)
types/conversion_test.go, line 172 at r4 (raw file):
failure: "Time.UnmarshalBinary: no data"}, {in: Val{Tid: DateTimeID, Value: bs(time.Time{})}, out: Val{Tid: FloatID, Value: float64(time.Time{}.UnixNano()) / float64(nanoSecondsInSec)}},
100 chars.
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @martinmr)
types/conversion_test.go, line 88 at r4 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
I don't think they add much. In fact, they might confuse someone by making them think the value is relevant.
Done.
types/conversion_test.go, line 172 at r4 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
100 chars.
Done.
srfrog
deleted the
srfrog/issue-3166_alpha_stops_on_empty_datetime_value
branch
* types/conversion.go: dont convert datetime to datetime when empty If the stored value of datetime predicate is empty, types.Marshal will return empty when converting to binary. When we return the result the empty binary is converted to datetime, this causes time.UnmarshalBinary() to fail. * query/outputnode.go: return empty string when result is zero-time value The zero-time value is an internal values that should be exposed to the client. * types/conversion_test.go: add tests for empty values Added tests to preserve the behavior with empty values. * types/conversion.go: expand the comment so it is clear * types/conversion.go: add specific conversion checks for datetime conversions Conversion from binary to datetime should happen for value length greater than zero. Convertion from empty string to int or float results their zero value. Conversion from datetime binary value (stored) will unmarshal only if the length is correct. Fixed a panic when bool is invalid, which obscures the real reason. * types/conversion_test.go: add test for conversion edge cases The edge test cases are for unexpected values that must be handled nicely. Updated float and int tests when converting from empty string value. * types/conversion.go: add sanity check for data type assertion * types/conversion.go: remove any default value conversions and return errors instead Remove all handling of default values for types that can return error when syntax fails. Update the tests to check for behavior. * types/conversion_test.go: remove extra value in test, fix 100 char length.
Datetime predicates with empty values are stored internally as zero-time values (ZVT). But when returning the results of a query, the ZTV is converted to a binary value and converted again to datetime (to preTraverse results). This led to marshaling empty string to datetime which failed. This change adds detection of ZTV when returning results to avoid the marshaling failure.
Closes #3166
This change is