Possible bug in Date handling

[codylerum]

While looking at #167 I was poking around what it would take to convert all the old java.util.Date returns over to java.time.Instant which would more accurately reflect the epoch seconds being returned and I noticed this

https://github.com/hellosign/hellosign-java-sdk/blob/master/src/main/java/com/hellosign/sdk/resource/AbstractResource.java#L69

All the API return values appears to be epochSecods but this here is using https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Date.html#getTime() which returns epoch milliseconds.

However I could not find any cases where a date was being set so it's unclear if this ever gets used. I'm curious however what hellosign expects if a date value is sent. Seconds or milliseconds?

[jspaetzel]

When returning Dates we use AbstractResource::getDate which converts the value from the dataObj to seconds. So I think everywhere talks in seconds.

[codylerum]

@jspaetzel I see how the getDate returns a date by taking the value from the JSONObject and multiplying it by 1000 and passing it into the Date construction expecting milliseconds thus implying that it was seconds. So that looks correct.

However when setting a date into the JSONObject it is being set as milliseconds.

https://github.com/hellosign/hellosign-java-sdk/blob/master/src/main/java/com/hellosign/sdk/resource/AbstractResource.java#L69

So it looks like when you are setting a date you are setting it as epoch milliseconds but when you are getting as a date you are assuming it is stored is seconds and multiplying by 1000

Unless I'm completely missing something.

[codylerum]

@jspaetzel here is a little example

class Scratch {

    public static void main(String[] args) {
        TestResource testResource =new TestResource();
        Date value = new Date();
        testResource.setDate(value);
        Date returnedValue = testResource.getDate();

        System.out.println(String.format("Was %s and now %s", value.toString(), returnedValue.toString()));

    }

    static class TestResource extends AbstractResource {

        public void setDate(Date date) {
            super.set("date", date);
        }

        public Date getDate() {
            return super.getDate("date");
        }
    }
}

Prints
Was Thu Apr 14 12:48:22 MDT 2022 and now Wed Mar 21 08:17:09 MDT 54255

I don't think there is actually a case in the codebase where you set a date so I don't think is actually currently causing any issues.

[jspaetzel]

Yep, you're right, if we were to use this function it'd be wrong. I went ahead and put together https://github.com/hellosign/hellosign-java-sdk/pull/180/files which fixes this behavior.

[jspaetzel]

And I totally agree, using Instant would be better. That'd be a good improvement to deprecate the old Date methods at some point and switchover.

[codylerum]

Sounds good. I'll close this.