opencadc / uws

client and server implementations of the Universal Worker Service (UWS) specification
GNU Affero General Public License v3.0
2 stars 8 forks source link

Updates to date output. #48

Closed at88mph closed 2 years ago

at88mph commented 2 years ago

Date output conforms to expected string as per document:

https://www.ivoa.net/documents/UWS/20161024/REC-UWS-1.1-20161024.html#resourceuri

pdowler commented 2 years ago

List of files that use IVOA standard date format:

morgul:/data/local/pdowler/opencadc/uws.git/cadc-uws[master]>whichFilesContain IVOA_DATE_FORMAT ./src/main/java/ca/nrc/cadc/uws/JobReader.java: 2 ./src/main/java/ca/nrc/cadc/uws/JobWriter.java: 1 ./src/test/java/ca/nrc/cadc/uws/JobReaderWriterTest.java: 1 morgul:/data/local/pdowler/opencadc/uws.git/cadc-uws[master]>cd ../cadc-uws-server/

morgul:/data/local/pdowler/opencadc/uws.git/cadc-uws-server[master]>whichFilesContain IVOA_DATE_FORMAT ./src/main/java/ca/nrc/cadc/uws/server/MemoryJobPersistence.java: 1 ./src/main/java/ca/nrc/cadc/uws/server/JobDAO.java: 1 ./src/main/java/ca/nrc/cadc/uws/web/GetAction.java: 2 ./src/main/java/ca/nrc/cadc/uws/web/JobCreator.java: 1 ./src/main/java/ca/nrc/cadc/uws/web/PostAction.java: 1 ./src/main/java/ca/nrc/cadc/uws/JobListReader.java: 1 ./src/main/java/ca/nrc/cadc/uws/JobListWriter.java: 1 morgul:/data/local/pdowler/opencadc/uws.git/cadc-uws-server[master]>cd ../cadc-test-uws/

morgul:/data/local/pdowler/opencadc/uws.git/cadc-test-uws[master]>whichFilesContain IVOA_DATE_FORMAT ./src/main/java/ca/nrc/cadc/conformance/uws/DestructionTest.java: 1 ./src/main/java/ca/nrc/cadc/conformance/uws/QuoteTest.java: 1

at88mph commented 2 years ago

The JobReader is backward compatible as-is, I believe. If a DateFormat with the DateUtil.IVOA_DATE_FORMAT parses the new date format (DateUtil.ISO8601_DATE_FORMAT_MSZ), it succeeds. The current setUp() method in the JobReaderWriterTest does the date parsing already, which is why it passed:

       this.dateFormat = DateUtil.getDateFormat(DateUtil.IVOA_DATE_FORMAT, DateUtil.UTC);
       this.baseDate = dateFormat.parse(TEST_DATE);

If I add a parse test for the new format, though:

        this.dateFormat = DateUtil.getDateFormat(DateUtil.IVOA_DATE_FORMAT, DateUtil.UTC);
        this.baseDate = dateFormat.parse(TEST_DATE);

        // This is the TEST_DATE with "Z" after the milliseconds, which is the expected new format.
        private String TEST_DATE_Z = TEST_DATE + "Z";

        // This succeeds and matches the already parsed one.
        Assert.assertEquals("Wrong date parsed.", this.baseDate, dateFormat.parse(TEST_DATE_Z));

Is that enough for the backward compatible release?

pdowler commented 2 years ago

hmmm, DateFormat is usually really picky. I pasted th above in and it does work, but I also tried one more test:

 String TEST_DATE_Z = TEST_DATE + "Z";

        DateFormat uwsDF = DateUtil.getDateFormat(DateUtil.ISO8601_DATE_FORMAT_MSZ, DateUtil.UTC);
        Date uws = uwsDF.parse(TEST_DATE_Z);
        Assert.assertEquals(baseDate, uws);

        Date backcompat = dateFormat.parse(TEST_DATE_Z);
        Assert.assertEquals(baseDate, backcompat);

        Date forwardcompat = uwsDF.parse(TEST_DATE);
        Assert.assertEquals(baseDate, forwardcompat);

This fails on the last parse, so that means DataFormat with IVOA_DATE_FORMAT will tolerate an unspecified Z but the "right" format won't tolerate a missing Z. That's subtle... could be due to the explicit TimeZone.UTC

at88mph commented 2 years ago

Could be. OK, then we'll need something like?

try {
    uwsDFZ.parse(dateString);
} catch (ParseException) {
    ivoaDF.parse(dateString);
}
pdowler commented 2 years ago

There are a lot of places in the uws code that use IVOA_DATE_FORMAT (list above)... some parse and some format. That needs to be looked at carefully.

I'd like to grok exactly why the parsing is flexible on absence/presence of the Z and make sure it isn't an accidental thing that might change or be easily broken.

pdowler commented 2 years ago

resolved by PR #51