CA-397409: Extend SDK deserialization support for xen-api dates #5802

danilo-delbusso · 2024-07-10T09:43:23Z

non-Zulu dates were not parsed correctly.

This meant that calls to getServerLocaltime calls were failing, as they have no timezone information

psafont · 2024-07-10T09:47:09Z

I now think that using human-readable dates in the API and the database was a mistake, they all should be unix timestamps. If there's really a need for server timezones (which I very much doubt), it should be exposed through a specialized API call.

.../sdk-gen/java/autogen/xen-api/src/main/java/com/xensource/xenapi/CustomDateDeserializer.java

kc284

As above.

danilo-delbusso · 2024-07-17T09:58:56Z

Last update improves support for C, C#, Go, and Java SDKs.

I have tested them all with the following test dates (this one in C#)

var dates = new Dictionary<string, DateTime>(){
    // no dashes, no colons
    { "20220101T123045", new DateTime(2022, 1, 1, 12, 30, 45, DateTimeKind.Unspecified)},
    { "20220101T123045Z", new DateTime(2022, 1, 1, 12, 30, 45, DateTimeKind.Utc) },
    { "20220101T123045+03",  new DateTime(2022, 1, 1, 9, 30, 45, DateTimeKind.Local)},
    { "20220101T123045+0300", new DateTime(2022, 1, 1, 9, 30, 45, DateTimeKind.Local)},
    { "20220101T123045+03:00", new DateTime(2022, 1, 1, 9, 30, 45, DateTimeKind.Local)},
    
    { "20220101T123045.123", new DateTime(2022, 1, 1, 12, 30, 45,123, DateTimeKind.Unspecified)},
    { "20220101T123045.123Z", new DateTime(2022, 1, 1, 12, 30, 45,123, DateTimeKind.Utc) },
    { "20220101T123045.123+03",  new DateTime(2022, 1, 1, 9, 30, 45,123, DateTimeKind.Local)},
    { "20220101T123045.123+0300", new DateTime(2022, 1, 1, 9, 30, 45,123, DateTimeKind.Local)},
    { "20220101T123045.123+03:00", new DateTime(2022, 1, 1, 9, 30, 45,123, DateTimeKind.Local)},

    // no dashes, with colons
    { "20220101T12:30:45", new DateTime(2022, 1, 1, 12, 30, 45, DateTimeKind.Unspecified)},
    { "20220101T12:30:45Z", new DateTime(2022, 1, 1, 12, 30, 45, DateTimeKind.Utc) },
    { "20220101T12:30:45+03",  new DateTime(2022, 1, 1, 9, 30, 45, DateTimeKind.Local)},
    { "20220101T12:30:45+0300", new DateTime(2022, 1, 1, 9, 30, 45, DateTimeKind.Local)},
    { "20220101T12:30:45+03:00", new DateTime(2022, 1, 1, 9, 30, 45, DateTimeKind.Local)},
    
    { "20220101T12:30:45.123", new DateTime(2022, 1, 1, 12, 30, 45,123, DateTimeKind.Unspecified)},
    { "20220101T12:30:45.123Z", new DateTime(2022, 1, 1, 12, 30, 45,123, DateTimeKind.Utc) },
    { "20220101T12:30:45.123+03",  new DateTime(2022, 1, 1, 9, 30, 45,123, DateTimeKind.Local)},
    { "20220101T12:30:45.123+0300", new DateTime(2022, 1, 1, 9, 30, 45,123, DateTimeKind.Local)},
    { "20220101T12:30:45.123+03:00", new DateTime(2022, 1, 1, 9, 30, 45,123, DateTimeKind.Local)},

    // dashes and colons
    { "2022-01-01T12:30:45", new DateTime(2022, 1, 1, 12, 30, 45, DateTimeKind.Unspecified) },
    { "2022-01-01T12:30:45Z", new DateTime(2022, 1, 1, 12, 30, 45, DateTimeKind.Utc) },
    { "2022-01-01T12:30:45+03", new DateTime(2022, 1, 1, 9, 30, 45, DateTimeKind.Local) },
    { "2022-01-01T12:30:45+0300", new DateTime(2022, 1, 1, 9, 30, 45, DateTimeKind.Local) },
    { "2022-01-01T12:30:45+03:00", new DateTime(2022, 1, 1, 9, 30, 45, DateTimeKind.Local) },

    { "2022-01-01T12:30:45.123", new DateTime(2022, 1, 1, 12, 30, 45, 123, DateTimeKind.Unspecified) },
    { "2022-01-01T12:30:45.123Z", new DateTime(2022, 1, 1, 12, 30, 45, 123, DateTimeKind.Utc) },
    { "2022-01-01T12:30:45.123+03", new DateTime(2022, 1, 1, 9, 30, 45, 123, DateTimeKind.Local) },
    { "2022-01-01T12:30:45.123+0300", new DateTime(2022, 1, 1, 9, 30, 45, 123, DateTimeKind.Local) },
    { "2022-01-01T12:30:45.123+03:00", new DateTime(2022, 1, 1, 9, 30, 45, 123, DateTimeKind.Local) },
};

kc284 · 2024-08-02T10:06:06Z

ocaml/sdk-gen/go/test_data/time_convert.go

@@ -1,4 +1,31 @@
-var timeFormats = []string{time.RFC3339, "20060102T15:04:05Z", "20060102T15:04:05"}
+var timeFormats = []string{


Not caused by this PR, but why do we need to redefine the time formats in the test code?

Not sure, I think it's there to ensure SDK source files are generated as expected.

@minglumlu could you help clarify?

This time_convert.go is an expected string output of rendering the template ConvertTime.mustache. The output will be put into the convert.go in the SDK.
Since the template is changed, as the test data, this file has to be updated as well.

So it's just a test of the mustache rendering, right?

Yes, as it is under the test_data directory.

Ah ok, they're test data in this case, not formats.
Now that I'm thinking about it, it might be nicer to use patterns as date formats in the mustache template instead of hardcoded dates, even if Go works with both. It looks to me like a better coding style - thoughts?

actually now that I've had a look in Go we use hardcoded dates (see https://pkg.go.dev/time#Parse)

Below is a snippet I used for testing (with which I actually just spotted a typo!), you can run it in on go.dev/play if you want

`example.go`

package main import ( "fmt" "time" ) func main() { dates := map[string]time.Time{ // no dashes, no colons "20220101T123045": time.Date(2022, 1, 1, 12, 30, 45, 0, time.Local), "20220101T123045Z": time.Date(2022, 1, 1, 12, 30, 45, 0, time.UTC), "20220101T123045+03": time.Date(2022, 1, 1, 12, 30, 45, 0, time.FixedZone("", 3*60*60)), // +03 timezone "20220101T123045+0300": time.Date(2022, 1, 1, 12, 30, 45, 0, time.FixedZone("", 3*60*60)), "20220101T123045+03:00": time.Date(2022, 1, 1, 12, 30, 45, 0, time.FixedZone("", 3*60*60)), "20220101T123045.123": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.Local), "20220101T123045.123Z": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.UTC), "20220101T123045.123+03": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.FixedZone("", 3*60*60)), "20220101T123045.123+0300": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.FixedZone("", 3*60*60)), "20220101T123045.123+03:00": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.FixedZone("", 3*60*60)), // no dashes, with colons "20220101T12:30:45": time.Date(2022, 1, 1, 12, 30, 45, 0, time.Local), "20220101T12:30:45Z": time.Date(2022, 1, 1, 12, 30, 45, 0, time.UTC), "20220101T12:30:45+03": time.Date(2022, 1, 1, 12, 30, 45, 0, time.FixedZone("", 3*60*60)), "20220101T12:30:45+0300": time.Date(2022, 1, 1, 12, 30, 45, 0, time.FixedZone("", 3*60*60)), "20220101T12:30:45+03:00": time.Date(2022, 1, 1, 12, 30, 45, 0, time.FixedZone("", 3*60*60)), "20220101T12:30:45.123": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.Local), "20220101T12:30:45.123Z": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.UTC), "20220101T12:30:45.123+03": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.FixedZone("", 3*60*60)), "20220101T12:30:45.123+0300": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.FixedZone("", 3*60*60)), "20220101T12:30:45.123+03:00": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.FixedZone("", 3*60*60)), // dashes and colons "2022-01-01T12:30:45": time.Date(2022, 1, 1, 12, 30, 45, 0, time.Local), "2022-01-01T12:30:45Z": time.Date(2022, 1, 1, 12, 30, 45, 0, time.UTC), "2022-01-01T12:30:45+03": time.Date(2022, 1, 1, 12, 30, 45, 0, time.FixedZone("", 3*60*60)), "2022-01-01T12:30:45+0300": time.Date(2022, 1, 1, 12, 30, 45, 0, time.FixedZone("", 3*60*60)), "2022-01-01T12:30:45+03:00": time.Date(2022, 1, 1, 12, 30, 45, 0, time.FixedZone("", 3*60*60)), "2022-01-01T12:30:45.123": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.Local), "2022-01-01T12:30:45.123Z": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.UTC), "2022-01-01T12:30:45.123+03": time.Date(2022, 1, 1, 12, 30, 45, 123000000, time.FixedZone("", 3*60*60)), } var timeFormats = []string{ time.RFC3339, "2006-01-02T15:04:05", // no dashes, no colons "20060102T15:04:05Z", "20060102T15:04:05", "20060102T150405.999999999Z0700", "20060102T150405", "20060102T150405Z07", "20060102T150405Z07:00", // no dashes, with colons "20060102T15:04:05Z07", "20060102T15:04:05Z0700", "20060102T15:04:05Z07:00", "20060102T15:04:05.999999999Z07", "20060102T15:04:05.999999999Z07:00", "20060102T15:04:05.999999999Z07", // dashes and colon patterns not covered by `time.RFC3339` "2006-01-02T15:04:05Z07", "2006-01-02T15:04:05Z0700", "2006-01-02T15:04:05Z07:00", "2006-01-02T15:04:05.999999999Z07", "2006-01-02T15:04:05.999999999Z07:00", "2006-01-02T15:04:05.999999999Z07", } for input, expected := range dates { for _, timeFormat := range timeFormats { result, err := time.Parse(timeFormat, input) if err == nil { matching := expected.Equal(result) if !matching { fmt.Printf("Found for '%s'\n", input) fmt.Printf("Corresponding time format is: '%s'\n", timeFormat) fmt.Printf("Expected: %s\n", expected) fmt.Printf("Parsed: %s\n\n", result) } } } } }

Yes, what I'm saying here is not that it doesn't work, but rather whether we might want to refactor it to use patterns for the time formats (ddmmyy etc.) like we do in the other languages instead of hardcoded dates. It's a matter of style really, not an issue with the functionality.

.../sdk-gen/java/autogen/xen-api/src/main/java/com/xensource/xenapi/CustomDateDeserializer.java

non-Zulu dates were not parsed correctly Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>

Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>

…ion (#5950) The Date module has a very convoluted way to deal with timezones because of its historic clients. While we can't remove all the issues, we can remove most of them. - Dates (timestamps, really) without timezones do not contain a frame of reference, and hence placing them in the UTC timeline can't be done accurately in the general case. This means that comparing timestamps gives incorrect results. - XMLRPC enforces dates to be encoded in ISO8601format. This means timestamps can lack a timezone. - Xapi uses xmlrpc's dates, adding this unfortunate limitation. - While Date allows these timestamps, it assumes that these are in the UTC timezone. On top of that, it refuses to process any timestamps that are in any timezone other than UTC (`Z`) - The Date module tries really hard to hide the timezone of timestamps that lack it when printing them. This means that timezoneless timestamps can persist in the database, for no good reason, as they are treated as UTC timestamps. - Host.localtime is the only call that returns timezoneless timestamps, all the other calls correctly return timestamps in the UTC timezone. Because the call on purpose does not want to return the current time in UTC, changing this might break clients not ready to process any timezone, mainly SDK-built ones #5802 - Dates are stored as a tuple of date, hour, minutes, seconds. This has very limited precision, which might be unexpected. This PR does the following mitigation / fixes: - Document all calls with Datetimes as parameters that the timestamps will be interpreted as UTC ones if they miss the timezone. - Remove the limitation to process any valid timezone - Timezoneless timstamps are immediately processed as UTC timestamps, as refusing these timestamps can break clients. Issues that the PR does not fix - Host.localtime produces timestamps without timezones, this is needed as adding non-zero timestamps breaks the SDK clients. - The server does not reject timezoneless timestamp, this might break migrations, RPUs, or normal clients, so I've held back on this change. - Printed timestamp do _not_ retain sub-second precision Drafting as tests are ongoing

danilo-delbusso added the sdk-members-only label Jul 10, 2024

danilo-delbusso requested review from kc284, psafont and CitrixChris July 10, 2024 09:43

danilo-delbusso self-assigned this Jul 10, 2024

psafont reviewed Jul 10, 2024

View reviewed changes

.../sdk-gen/java/autogen/xen-api/src/main/java/com/xensource/xenapi/CustomDateDeserializer.java Outdated Show resolved Hide resolved

kc284 reviewed Jul 10, 2024

View reviewed changes

.../sdk-gen/java/autogen/xen-api/src/main/java/com/xensource/xenapi/CustomDateDeserializer.java Show resolved Hide resolved

danilo-delbusso force-pushed the bug/rfc-3339 branch from a2a1080 to 001ea15 Compare July 10, 2024 10:14

kc284 requested changes Jul 10, 2024

View reviewed changes

danilo-delbusso added the needs updating label Jul 15, 2024

danilo-delbusso force-pushed the bug/rfc-3339 branch from 001ea15 to 7e8f90a Compare July 17, 2024 09:57

danilo-delbusso changed the title ~~Extend Java deserialization support for xen-api dates~~ Extend SDK deserialization support for xen-api dates Jul 17, 2024

danilo-delbusso requested a review from kc284 July 17, 2024 09:59

danilo-delbusso removed the needs updating label Jul 17, 2024

danilo-delbusso force-pushed the bug/rfc-3339 branch from 7e8f90a to 74c14a6 Compare July 17, 2024 10:02

danilo-delbusso added the needs updating label Jul 17, 2024

danilo-delbusso force-pushed the bug/rfc-3339 branch from 74c14a6 to 6629c04 Compare July 17, 2024 12:33

danilo-delbusso removed the needs updating label Jul 17, 2024

kc284 added the updated label Jul 22, 2024

kc284 reviewed Aug 2, 2024

View reviewed changes

danilo-delbusso requested a review from kc284 August 8, 2024 07:34

danilo-delbusso changed the title ~~Extend SDK deserialization support for xen-api dates~~ CA-397409: Extend SDK deserialization support for xen-api dates Aug 13, 2024

kc284 reviewed Aug 13, 2024

View reviewed changes

.../sdk-gen/java/autogen/xen-api/src/main/java/com/xensource/xenapi/CustomDateDeserializer.java Outdated Show resolved Hide resolved

danilo-delbusso added needs updating and removed updated labels Aug 13, 2024

Extend Java deserialization support for xen-api dates

7d6967a

non-Zulu dates were not parsed correctly Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>

danilo-delbusso force-pushed the bug/rfc-3339 branch from 6629c04 to 6ffd9bd Compare August 13, 2024 10:19

danilo-delbusso removed the needs updating label Aug 13, 2024

danilo-delbusso added the updated label Aug 13, 2024

danilo-delbusso requested a review from kc284 August 13, 2024 10:19

danilo-delbusso added needs updating and removed updated labels Aug 21, 2024

psafont mentioned this pull request Aug 23, 2024

Date: Accept all valid timezones from client, allow sub-second precision #5950

Merged

danilo-delbusso force-pushed the bug/rfc-3339 branch from 6ffd9bd to d000344 Compare September 9, 2024 14:36

danilo-delbusso added updated and removed needs updating labels Sep 9, 2024

danilo-delbusso added 3 commits September 9, 2024 15:46

Expand Go deserialization support for xen-api dates

cf27ff9

Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>

Expand C# deserialization support for xen-api dates

8f4c112

Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>

Expand C deserialization support for xen-api dates

dcfc0e4

Signed-off-by: Danilo Del Busso <danilo.delbusso@cloud.com>

danilo-delbusso force-pushed the bug/rfc-3339 branch from d000344 to dcfc0e4 Compare September 9, 2024 14:47

kc284 approved these changes Sep 11, 2024

View reviewed changes

xueqingz approved these changes Sep 12, 2024

View reviewed changes

kc284 merged commit eda6239 into xapi-project:master Sep 12, 2024
14 checks passed

danilo-delbusso deleted the bug/rfc-3339 branch September 20, 2024 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CA-397409: Extend SDK deserialization support for xen-api dates #5802

CA-397409: Extend SDK deserialization support for xen-api dates #5802

danilo-delbusso commented Jul 10, 2024

psafont commented Jul 10, 2024

kc284 left a comment

danilo-delbusso commented Jul 17, 2024

kc284 Aug 2, 2024

danilo-delbusso Aug 2, 2024

minglumlu Aug 8, 2024 •

edited

Loading

danilo-delbusso Aug 8, 2024

minglumlu Aug 13, 2024

kc284 Aug 13, 2024

danilo-delbusso Sep 9, 2024 •

edited

Loading

kc284 Sep 9, 2024

		@@ -1,4 +1,31 @@
		var timeFormats = []string{time.RFC3339, "20060102T15:04:05Z", "20060102T15:04:05"}
		var timeFormats = []string{

CA-397409: Extend SDK deserialization support for xen-api dates #5802

CA-397409: Extend SDK deserialization support for xen-api dates #5802

Conversation

danilo-delbusso commented Jul 10, 2024

psafont commented Jul 10, 2024

kc284 left a comment

Choose a reason for hiding this comment

danilo-delbusso commented Jul 17, 2024

kc284 Aug 2, 2024

Choose a reason for hiding this comment

danilo-delbusso Aug 2, 2024

Choose a reason for hiding this comment

minglumlu Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

danilo-delbusso Aug 8, 2024

Choose a reason for hiding this comment

minglumlu Aug 13, 2024

Choose a reason for hiding this comment

kc284 Aug 13, 2024

Choose a reason for hiding this comment

danilo-delbusso Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

kc284 Sep 9, 2024

Choose a reason for hiding this comment

minglumlu Aug 8, 2024 •

edited

Loading

danilo-delbusso Sep 9, 2024 •

edited

Loading