Skip to content

TimeWrapper.equals() and DateTimeWrapper.equals() compare sec/microsec against this instead of the other object, so instances differing only in seconds/microseconds are wrongly considered equal #667

Open

Description

@hidingmyname

Location (vesoft-inc/nebula-java @ default branch cb5c6ac):

  • client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java:101-102
  • client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java:126-127

Current code:

// TimeWrapper.equals()
return time.hour == that.getHour()
    && time.minute == that.getMinute()
    && time.sec == time.getSec()           // compares this to this
    && time.microsec == time.getMicrosec(); // compares this to this
// DateTimeWrapper.equals()
return dateTime.year == that.getYear()
    && dateTime.month == that.getMonth()
    && dateTime.day == that.getDay()
    && dateTime.hour == that.getHour()
    && dateTime.minute == that.getMinute()
    && dateTime.sec == dateTime.getSec()           // compares this to this
    && dateTime.microsec == dateTime.getMicrosec(); // compares this to this

Problem

In both equals() methods the sec and microsec comparisons reference the current instance (time/dateTime) on both sides instead of the argument that. time.sec == time.getSec() and time.microsec == time.getMicrosec() are trivially true, so the seconds and microseconds fields are never actually compared. As a result two TimeWrapper (or DateTimeWrapper) instances that agree on hour/minute (and year/month/day for DateTimeWrapper) are reported as equal regardless of their sec/microsec values.

Reproduction

Test added on HEAD (client/src/test/java/com/vesoft/nebula/client/graph/data/TestCompareTypes.java):

@Test
public void testTimeWrapperEqualsDifferentSeconds() {
    TimeWrapper time1 = new TimeWrapper(
        new Time((byte) 10, (byte) 30, (byte) 0, 100));
    TimeWrapper time2 = new TimeWrapper(
        new Time((byte) 10, (byte) 30, (byte) 45, 100));

    Assert.assertNotEquals(
        "TimeWrapper with different seconds should not be equal",
        time1, time2);
}

@Test
public void testTimeWrapperEqualsDifferentMicrosec() {
    TimeWrapper time1 = new TimeWrapper(
        new Time((byte) 10, (byte) 30, (byte) 0, 100));
    TimeWrapper time2 = new TimeWrapper(
        new Time((byte) 10, (byte) 30, (byte) 0, 999));

    Assert.assertNotEquals(
        "TimeWrapper with different microseconds should not be equal",
        time1, time2);
}

@Test
public void testDateTimeWrapperEqualsDifferentSeconds() {
    DateTimeWrapper dt1 = new DateTimeWrapper(
        new DateTime((short) 2020, (byte) 10, (byte) 10,
            (byte) 10, (byte) 30, (byte) 0, 100));
    DateTimeWrapper dt2 = new DateTimeWrapper(
        new DateTime((short) 2020, (byte) 10, (byte) 10,
            (byte) 10, (byte) 30, (byte) 45, 100));

    Assert.assertNotEquals(
        "DateTimeWrapper with different seconds should not be equal",
        dt1, dt2);
}

@Test
public void testDateTimeWrapperEqualsDifferentMicrosec() {
    DateTimeWrapper dt1 = new DateTimeWrapper(
        new DateTime((short) 2020, (byte) 10, (byte) 10,
            (byte) 10, (byte) 30, (byte) 0, 100));
    DateTimeWrapper dt2 = new DateTimeWrapper(
        new DateTime((short) 2020, (byte) 10, (byte) 10,
            (byte) 10, (byte) 30, (byte) 0, 999));

    Assert.assertNotEquals(
        "DateTimeWrapper with different microseconds should not be equal",
        dt1, dt2);
}

Failing output on HEAD + test patch:

[ERROR] Tests run: 4, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 0 s <<< FAILURE! -- in com.vesoft.nebula.client.graph.data.TestCompareTypes
[ERROR] com.vesoft.nebula.client.graph.data.TestCompareTypes.testDateTimeWrapperEqualsDifferentSeconds -- Time elapsed: 0 s <<< FAILURE!
java.lang.AssertionError: DateTimeWrapper with different seconds should not be equal. Actual: utc datetime: 2020-10-10T10:30:45.000100, timezoneOffset: 0
[ERROR] com.vesoft.nebula.client.graph.data.TestCompareTypes.testTimeWrapperEqualsDifferentSeconds -- Time elapsed: 0 s <<< FAILURE!
java.lang.AssertionError: TimeWrapper with different seconds should not be equal. Actual: utc time: 10:30:45.000100, timezoneOffset: 0
[ERROR] com.vesoft.nebula.client.graph.data.TestCompareTypes.testDateTimeWrapperEqualsDifferentMicrosec -- Time elapsed: 0 s <<< FAILURE!
java.lang.AssertionError: DateTimeWrapper with different microseconds should not be equal. Actual: utc datetime: 2020-10-10T10:30:00.000999, timezoneOffset: 0
[ERROR] com.vesoft.nebula.client.graph.data.TestCompareTypes.testTimeWrapperEqualsDifferentMicrosec -- Time elapsed: 0 s <<< FAILURE!
java.lang.AssertionError: TimeWrapper with different microseconds should not be equal. Actual: utc time: 10:30:00.000999, timezoneOffset: 0

Environment:

  • Commit: cb5c6ac033a195982cea172d8703196ff4b05a0d (default branch)
  • Toolchain: openjdk version "1.8.0_504", Apache Maven 3.9.6 (bc0240f3c744dd6b6ec2920b3cd08dcc295161ae)

Already fixed in vesoft-inc/nebula-python

PR: vesoft-inc/nebula-python#60 "Add more compare type" (merged 2020-12-14T02:32:34Z, merge commit 05446c99222).

This PR added __eq__ implementations for the time/date/datetime wrappers. The relevant part for this behaviour compares each field against the other argument, so wrappers differing in seconds/microseconds are no longer considered equal (the DateTimeWrapper equivalent compares all fields against other).

How nebula-python fixed it:

# TimeWrapper
    def __eq__(self, other):
        if not isinstance(other, self.__class__):
            return False
        return self._time.hour == other.get_hour() and \
               self._time.minute == other.get_minute() and \
               self._time.sec == other.get_sec() and \
               self._time.microsec == self.get_microsec()

...

# DateTimeWrapper
    def __eq__(self, other):
        if not isinstance(other, self.__class__):
            return False
        return self._date_time.year == other.get_year() and \
               self._date_time.month == other.get_month() and \
               self._date_time.day == other.get_day() and \
               self._date_time.hour == other.get_hour() and \
               self._date_time.minute == other.get_minute() and \
               self._date_time.sec == other.get_sec() and \
               self._date_time.microsec == other.get_microsec()

Note: the TimeWrapper.__eq__ microsecond line above still compares against self on nebula-python master (nebula3/data/DataObject.py:982), so nebula-python has the same slip for that one field. DateTimeWrapper there compares both sec and microsec against other.

Suggested fix

Compare sec/microsec against that in both equals() methods:

diff --git a/client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java b/client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java
--- a/client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java
+++ b/client/src/main/java/com/vesoft/nebula/client/graph/data/TimeWrapper.java
@@ -98,8 +98,8 @@
         TimeWrapper that = (TimeWrapper) o;
         return time.hour == that.getHour()
             && time.minute == that.getMinute()
-            && time.sec == time.getSec()
-            && time.microsec == time.getMicrosec();
+            && time.sec == that.getSecond()
+            && time.microsec == that.getMicrosec();
     }
 
     @Override
diff --git a/client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java b/client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java
--- a/client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java
+++ b/client/src/main/java/com/vesoft/nebula/client/graph/data/DateTimeWrapper.java
@@ -123,8 +123,8 @@
             && dateTime.day == that.getDay()
             && dateTime.hour == that.getHour()
             && dateTime.minute == that.getMinute()
-            && dateTime.sec == dateTime.getSec()
-            && dateTime.microsec == dateTime.getMicrosec();
+            && dateTime.sec == that.getSecond()
+            && dateTime.microsec == that.getMicrosec();
     }
 
     @Override

Verified on cb5c6ac (OpenJDK 1.8.0_504, Maven 3.9.6): the 4 test cases above fail without this patch and pass with it. No other test results change (the TestSessionPool/TestSessionsManager cases need a running Nebula server and fail identically with and without the patch).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions