Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DateTime format in EpcisEvent for callbacks #1

Open
yvdh opened this issue May 8, 2024 · 10 comments
Open

DateTime format in EpcisEvent for callbacks #1

yvdh opened this issue May 8, 2024 · 10 comments
Assignees

Comments

@yvdh
Copy link

yvdh commented May 8, 2024

Hello,

In the EPCIS repository the EventTime is stored as a DateTime. It is always stored in Zulu time (UTC), even if another time zone was specified for that time. The EventTimeZoneOffset field represents the timezone of the original capture.

In the EPCIS subscription callbacks the eventTime field is DateTimeOffset instead of a DateTime. However, this does not seem to represent the local time equivalent of the stored database field.

Thus, capture Event contains:
{ ...
"eventTime": "2019-06-12T06:31:32-04:00",
"eventTimeZoneOffset": "-04:00",
....
}

Database contains (as expected)

EventTime EventTimeZoneOffset
2019-06-12 10:31:32.0000000 -240

However, when sending that EPCIS event in the callback we get:

EventTime = {12/06/2019 10:31:32 +02:00}

Instead of
12/06/2019 10:31:32 +00:00

It seems that the database field is interpreted as being in local time instead of UTC for the callback!

According to MSDN for Json.Text deserializer, when no time zone info is present (and there isn't in the database) the time will be interpreted as local time, not as UTC.

I have solved this using this code in the JsonCallbackParser

  // Added yvdh: DateTime default to local, while the db is UTC!!!!
  if (property.PropertyType == typeof(DateTimeOffset))
  { 
  // This date time has no idea of timezone ...
   var utcAsWrongLocalTime  = DateTime.SpecifyKind(((DateTimeOffset)    jsonValue.Value.Deserialize(property.PropertyType)).DateTime, DateTimeKind.Utc );
    value = new DateTimeOffset(utcAsWrongLocalTime);
  }

This does not take care of arrays of time ...

Cheers
Yves

@louisaxel-ambroise
Copy link
Owner

Hi @yvdh,

Thanks for raising this issue.
The date should be parsed correctly if the timezone is correctly specified, so it seems that the issue is in the EPCIS repository itself and not in this parsing library.

By looking at the JsonEventFormatter, it appears that the JSON formatter doesn't include the timezone by default. I created a PR for it in the EPCIS repository project. I did a few tests with that change and everything looks correct, at least for expected DateTime fields in the events.
For custom fields everything should be correct as the repository should return the text value that it received.

Can you explain what you mean with:

This does not take care of arrays of time ...

Do these arrays come from custom data (extension/ILMD)? If so, could it be possible for you to share an example to verify the fix in the repo is correct?

Thanks and regards,

@yvdh
Copy link
Author

yvdh commented May 13, 2024

Hello Louis-Axel.

thnx for the PR, I will check it out.

Regarding arrays, I just meant I think that the code I modified does not do anything for DateTime arrays/lists/collections in the parser. I don't think there are such properties currently in EPCIS, and I have none myself, but there could theoretically be in extensions at some point in the future.
I did not do any tests other than check that it worked for my specific case ..

Cheers,
Yves

@yvdh
Copy link
Author

yvdh commented May 13, 2024

Hello,

I checked out the code, but as far as I can tell this solves the problem for the main EPCIS hosting solution, but not for the EPCIS subscription callback client in your other project . This would simply also require inclusion of the EpcisDateFormatConverter : JsonConverter for its json serialization.
Truth be told I should have posted this issue in that project, my bad.
Cheers,
Yves

@yvdh
Copy link
Author

yvdh commented May 14, 2024

I checked out what DateTimeStyles.AdjustToUniversal does, and I don't think it does what we need!

If I give it a string without timezone info (as from the database) it will subtract the current server time zone offset from it:

"2019-06-12T10:31:32" => "2019-06-12T08:31:32+02:00"

As the date times in the database are supposedly always UTC and are to be interpreted as absolute times this should become either:

  • "2019-06-12T12:31:32+02:00" as local time in a +2 zone
  • "2019-06-12T10:31:32Z" as UTC time.

So, AdjustToUniversal still assumes times without any timezone are local times, which they are not!

This snippet will always interpret times without timezone as UTC.

var utcTime = DateTime.SpecifyKind(((DateTimeOffset)jsonValue.Value.Deserialize(property.PropertyType)).DateTime, DateTimeKind.Utc);
value = new DateTimeOffset(utcTime);

Also, I have integrated the new date formatter into the EPCIS host, but dates are still written to the database without timezone info (no 'Z'), which is what I think we ultimately need (correct me if I am wrong).

If I use DateTimeStyles.AssumeUniversal then the output seems correct:

"2019-06-12T10:31:32" => "2019-06-12T12:31:32+02:00"

private class EpcisDateFormatConverter : JsonConverter
{
public override DateTimeOffset Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
var timeString = reader.GetString();
var time = DateTimeOffset.Parse(timeString, null, DateTimeStyles.AssumeUniversal);
return time;
}

public override void Write(Utf8JsonWriter writer, DateTimeOffset dateTimeValue, JsonSerializerOptions options)
=> writer.WriteStringValue(dateTimeValue.ToString("yyyy-MM-ddTHH:mm:ss.fffZ"));
}

All in all, this time stuff is quite messy in .NET ...

@louisaxel-ambroise
Copy link
Owner

That's right, AdjustToUniversal will always assume that the original format is Local if it's not specified.. A combination of DateTimeStyles.AssumeUniversaland DateTimeStyles.AdjustToUniversal might return the expected result as a datetime without timezone would be treated as UTC, and if a timezone is specified it would be converted to UTC.

Very quick tests returned the expected values, i.e. "2019-06-12T10:31:32" is converted to "2019-06-12T10:31:32Z", and "2019-06-12T12:31:32+02:00" to "2019-06-12T10:31:32Z" as well.

Also as the formatted values are always formatted in UTC there's probably no need to update the callback library.

This will require a change in all the dates fields in both XML and JSON parsers, I'll try to update it as soon as possible in the EPCIS repository.

Thanks for all your time invested on this issue, that'll be a great improvement!

@yvdh
Copy link
Author

yvdh commented May 16, 2024

That sounds perfect to me. By persisting all the timestamps with a timezone in the database (even if not UTC) any ambiguity when retrieving those timestamps could easily be resolved ...

I am really happy to contribute and give something back, this project is a real life-saver for me.

Yves

@louisaxel-ambroise
Copy link
Owner

Hi @yvdh

The branch fix/json-formatting is updated to always use UTC timezone for the dates.

It'd be really appreciated if you could test it to see if it fully matches your expectations.
Thanks!

@yvdh
Copy link
Author

yvdh commented May 22, 2024

Hello,

I quickly setup a separate project with your fix branch and ran it. Here are my findings:

  1. It assumes UTC if no timezone is present in a capture: 2019-06-12T06:31:32 => 2019-06-12 06:31:32.0000000 in DB
  2. It uses the provided timezone if present in a capture: 2019-06-12T06:31:32-04:00 => 2019-06-12 10:31:32.0000000 in DB.
  3. It returns the wrong event time when querying: 2019-06-12 06:31:32.0000000 in DB => 2024-04-22T05:31:54.459Z in response
  4. The recordTime in a record response is wrong: 2024-05-22 10:22:48.1927291 in DB => 2024-04-22T12:44:56.921Z

My 2 cents: assuming UTC when no time zone is present is a step forward but I think just adding timezone info in the database is a safer solution (especially with the ambiguity in the .NET date/time coding). It really means we are using absolute time that leaves no room for interpretation. The callback library should then also still work out of the box I think.

Available for discussion in Whatsapp if you want ...

Cheers
Yves

@yvdh
Copy link
Author

yvdh commented May 22, 2024

I checked a few things and I understand that my suggestion requires any DateTime2 type column to become a DateTimeOffset column in the database. Mmm, that's quite disruptive ...

So your idea is probably a softer approach to the problem ...
Yves

@louisaxel-ambroise
Copy link
Owner

Hi @yvdh,

I didn't have much time to look at it this week, but regarding the point 3. and 4. I don't think this is only a timezone issue as the minutes and seconds are also different:
2019-06-12 06:31:32.0000000 => 2024-04-22T05:31:54.459Z
2024-05-22 10:22:48.1927291 => 2024-04-22T12:44:56.921Z

The recordTime field of the event is always overridden by the repository, so I don't think that the point 4. is an error.
Regarding point 3. I'd appreciate if you could share an example of event and query that you make as I'm not able to reproduce it in basic tests.

Using DateTimeOffset type could also be a solution, it can probably be added later on to the repository. But as you said it implies a column type change in the DB so it's not something that can be done very easily.

I should have more time next week to work on this repo so hopefully we can close it soon.

Regards,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants