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

JSONRPC incorrectly decodes UTC values for DateTime Params #560

Closed
fastbike opened this issue Jun 10, 2022 · 3 comments
Closed

JSONRPC incorrectly decodes UTC values for DateTime Params #560

fastbike opened this issue Jun 10, 2022 · 3 comments
Assignees
Labels
accepted Issue has been accepted and inserted in a future milestone

Comments

@fastbike
Copy link
Contributor

fastbike commented Jun 10, 2022

We have a JSONRPC method with this signature
function EndPractitionerRole(ID: Int64; LastUpdated: TDateTime): Boolean;

We send data
{"jsonrpc":"2.0","id":926270,"method":"EndPractitionerRole","params":{"ID":1203521835,"LastUpdated":"2022-06-10T09:08:25+12:00"}}

As our current local UTC offset is +12 hours, we are expecting to see a local datetime in the method's parameter of 9:08:25 am on 10 June 2022

Instead we see a local datetime of 9:08:25 pm on 09 June 2022 which is 12 hours earlier i.e. it misses the offset.

NB: We also see the same behaviour if the JSON contains a zulu coded value i.e. 2022-06-09T21:08:25Z

This is caused by the mapping performed by JSONDataValueToTValueParam

The code in the following snippet will never call the code in the else part because by definition all UTC date times must contain "T"

    else if SameText(RTTIParameter.ParamType.Name, 'TDateTime') then
        begin
          if JSONDataValue.Value.Contains('T') then // always evaluates to TRUE
            JSONRPCRequestParams.Add(JSONDataValue.UtcDateTimeValue, pdtDateTime)
          else // never called with an UTC timestamp
            JSONRPCRequestParams.Add(ISOTimeStampToDateTime(JSONDataValue.Value), pdtDateTime);
        end

And the code in the underlying JsonDataObjects has a hard coded False for the "useLocalTime" param in the call to return UtcDateTimeValue

In the debugger if I jump into the else part so the ISOTimeStampToDateTime method is called then the correct value appears in the JSONRPC method's parameter.

@danieleteti danieleteti added the accepted Issue has been accepted and inserted in a future milestone label Jun 11, 2022
@danieleteti danieleteti added this to the 3.2.2-nitrogen milestone Jun 11, 2022
@fastbike
Copy link
Contributor Author

fastbike commented Jun 11, 2022

Interestingly, in my mind I had always conflated UTC and ISO8601. Having read this SO entry (https://stackoverflow.com/questions/58847869/utc-vs-iso-format-for-time) I now see that ISO8601 is the structure of the underlying data, and UTC, GMT, local time etc are presentation formats.
On that basis I would recommend changing the logic of the conversion code snippet so that if "T" is found as the separator between date and time parts of the input then an ISO timestamp to TDateTime conversion is attempted. Note that ISO 8601 allows calendar dates with dash separators e.g. 2022-06-09T21:08:25Z as well as 20220609T21:08:25Z so a nested helper function allows for this. However the subsequent call to ISOTimeStampToDateTime will fail if the timestamp is less than 19 characters long.

procedure JSONDataValueToTValueParam(const JSONDataValue: TJsonDataValueHelper; const RTTIParameter: TRttiParameter;
  const JSONRPCRequestParams: TJSONRPCRequestParams);

  function IsISOTimeStamp: Boolean;
  begin
    Result := JSONDataValue.Value.IndexOf('T') = 10;
    if not Result and (JSONDataValue.Value.Substring(4,1) <> '-') then
      Result := JSONDataValue.Value.IndexOf('T') = 8;
  end;

begin
  case RTTIParameter.ParamType.TypeKind of
// snip
    tkFloat:
      begin
// snip
        else if SameText(RTTIParameter.ParamType.Name, 'TDateTime') then
        begin
          if IsISOTimeStamp then
            JSONRPCRequestParams.Add(ISOTimeStampToDateTime(JSONDataValue.Value), pdtDateTime)
          else
            JSONRPCRequestParams.Add(JSONDataValue.UtcDateTimeValue, pdtDateTime);
        end
...

@fastbike
Copy link
Contributor Author

fastbike commented Jun 12, 2022

Given that the subsequent call to ISOTimeStampToDateTime fails if the timestamp is less than 19 characters long i.e. of the form that omits the dash date separators YYYYMMDDThh:mm:ss then there is no point doing the second check for the position of "T"
So the modified code simplifies to:

procedure JSONDataValueToTValueParam(const JSONDataValue: TJsonDataValueHelper; const RTTIParameter: TRttiParameter;
  const JSONRPCRequestParams: TJSONRPCRequestParams);
begin
  case RTTIParameter.ParamType.TypeKind of
// snip
    tkFloat:
      begin
// snip
        else if SameText(RTTIParameter.ParamType.Name, 'TDateTime') then
        begin
          if JSONDataValue.Value.IndexOf('T') = 10 then
            JSONRPCRequestParams.Add(ISOTimeStampToDateTime(JSONDataValue.Value), pdtDateTime)
          else
            JSONRPCRequestParams.Add(JSONDataValue.UtcDateTimeValue, pdtDateTime);
        end
...

@fastbike
Copy link
Contributor Author

Thanks for testing and accepting the changes. I'll close the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue has been accepted and inserted in a future milestone
Projects
None yet
Development

No branches or pull requests

2 participants