[PHP] Merging of Timestamp field from JSON string results in wrong seconds

[bitevo]

Merging of Timestamp field from JSON string results in wrong seconds. In my test, it was off by 1 sec for microsecond resolution string and even worse for nanosecond resolution string.

Test output (run on MacOS High Sierra):

PHP Version: 7.1.7

Original as string : 2018-02-25T03:48:17.428086+00:00
Original seconds   : 1519530497.428086

Convert nano string to/from JSON:
Nano input string  : 2018-02-25T03:48:17.428086000Z
JS Decoded seconds : 1519530490
JS Decoded nanos   : 428086000

Convert to and from protobuf:
Pre-enc seconds   : 1519530497
Pre-enc nanos     : 428086
PB Decoded sec    : 1519530497
PB Decoded nsec   : 428086

Convert to and from JSON:
JS Decoded sec    : 1519530496
JS Decoded nsec   : 428086

The issue described here is about correctness, not precision. I know PHP doesn't natively support nano second, but string parsing should not be affected by that. There will be lost of precision during conversion from string to PHP DateTime object, but the number of seconds should be always correct.

The proto file I used in the test:

syntax = "proto3";

import "google/protobuf/timestamp.proto";

message Sample {
  google.protobuf.Timestamp timestamp = 1;
}

PHP test script (using compiled PHP class):

$timestamp = new \DateTime('2018-02-25T03:48:17.428086', new \DateTimeZone('UTC'));
$gpbTimestamp = new \Google\Protobuf\Timestamp();
$gpbTimestamp->setSeconds((int) $timestamp->format('U'));
$gpbTimestamp->setNanos((int) $timestamp->format('u'));
echo "PHP Version: ", PHP_VERSION, PHP_EOL;
echo PHP_EOL;
echo "Original as string : ", $timestamp->format('Y-m-d\TH:i:s.uP'), PHP_EOL;
echo "Original seconds   : ", $timestamp->format('U.u'), PHP_EOL;
echo PHP_EOL;

echo "Convert nano string to/from JSON:", PHP_EOL;
$nanoArray = [
    'timestamp' => $timestamp->format('Y-m-d\TH:i:s.u') . '000Z',
];
$message = new Sample();
$message->mergeFromJsonString(json_encode($nanoArray));
echo "Nano input string  : ", $nanoArray['timestamp'], PHP_EOL;
echo "JS Decoded seconds : ", $message->getTimestamp()->getSeconds(), PHP_EOL;
echo "JS Decoded nanos   : ", $message->getTimestamp()->getNanos(), PHP_EOL;
echo PHP_EOL;

echo "Convert to and from protobuf:", PHP_EOL;
$message = new Sample(); // ensure clear start
$message->setTimestamp($gpbTimestamp);
echo "Pre-enc seconds   : ", $message->getTimestamp()->getSeconds(), PHP_EOL;
echo "Pre-enc nanos     : ", $message->getTimestamp()->getNanos(), PHP_EOL;
$pbEnc = $message->serializeToString();
$message = new Sample(); // ensure clear start
$message->mergeFromString($pbEnc);
echo "PB Decoded sec    : ", $message->getTimestamp()->getSeconds(), PHP_EOL;
echo "PB Decoded nsec   : ", $message->getTimestamp()->getNanos(), PHP_EOL;
echo PHP_EOL;

echo "Convert to and from JSON:", PHP_EOL;
$message = new Sample(); // ensure clear start
$message->setTimestamp($gpbTimestamp);
$jsonEnc = $message->serializeToJsonString();
$message = new Sample(); // ensure clear start
$message->mergeFromJsonString($jsonEnc);
echo "JS Decoded sec    : ", $message->getTimestamp()->getSeconds(), PHP_EOL;
echo "JS Decoded nsec   : ", $message->getTimestamp()->getNanos(), PHP_EOL;

[shogochiai]

For elaborating this issue, I want to clarify these.

1. JSON convert and Protobuf convert are using different logic to convert timestamp, and this is plugin, and (maybe) not tested well on protobuf-php

2. Those “logics” may be in mergeFromJsonString, serializeToString and serializeToJsonString.

3. fromDatetime, toDatetime, getSeconds, setSeconds, getNanos and setNanos are tested on php-timestamp-adaptor side, but we cannot confirm that is enough test case for No.2 mentioned functions (https://github.com/google/protobuf/blob/77f64bb7779ec2195f9bc4dc82497d12c18fc6b7/php/tests/well_known_test.php#L302-L320)

Is this gonna be critical path? If so, we'd better to put enough test case on these related funcs.

[TeBoring]

Are you using c extension?

[bitevo]

@TeBoring I didn't use the C ext. I used the PHP script (google/protobuf v3.5.1.1).

[bitevo]

Just in case you need it, you can find the test files as composer project on https://github.com/sbhalim/protobuf-issue-4335

[TeBoring]

@bitevo In your example, $timestamp->format('u') is usec. It should be multiplied by 1000.

[TeBoring]

Can't reproduce. Maybe this has been fixed.