Skip to content

PHPC-790: Accept integer and floats in Timestamp and UTCDateTime constructors #467

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

Merged
merged 4 commits into from
Nov 25, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 29 additions & 17 deletions src/BSON/Timestamp.c
Original file line number Diff line number Diff line change
Expand Up @@ -91,22 +91,14 @@ static bool php_phongo_timestamp_init_from_string(php_phongo_timestamp_t *intern
* available on all platforms (e.g. HP-UX), and atoll() provides no error
* reporting at all. */

#if defined(PHP_WIN32)
increment = _atoi64(s_increment);
#else
increment = bson_ascii_strtoll(s_increment, &endptr, 10);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_atoi64() would parse "1234.5" as 1234 and raise no error, while bson_ascii_strtoll() is more strict. Also, bson_ascii_strtoll() now supports out-of-range error reporting in libbson 1.5.0 (CDRIVER-1337). That was the only benefit of using _atoi64() for Windows before, so I see no reason to use consistent parsing for all platforms.

#endif

if (errno || (endptr && endptr != ((const char *)s_increment + s_increment_len))) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Error parsing \"%s\" as 64-bit integer increment for %s initialization", s_increment, ZSTR_VAL(php_phongo_timestamp_ce->name));
return false;
}

#if defined(PHP_WIN32)
timestamp = _atoi64(s_timestamp);
#else
timestamp = bson_ascii_strtoll(s_timestamp, &endptr, 10);
#endif

if (errno || (endptr && endptr != ((const char *)s_timestamp + s_timestamp_len))) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Error parsing \"%s\" as 64-bit integer timestamp for %s initialization", s_timestamp, ZSTR_VAL(php_phongo_timestamp_ce->name));
Expand Down Expand Up @@ -148,28 +140,48 @@ static bool php_phongo_timestamp_init_from_hash(php_phongo_timestamp_t *intern,
return false;
}

/* {{{ proto void Timestamp::__construct(string $increment, string $timestamp)
/* {{{ proto void Timestamp::__construct(int|string $increment, int|string $timestamp)
Construct a new BSON timestamp type, which consists of a 4-byte increment and
4-byte timestamp. */
PHP_METHOD(Timestamp, __construct)
{
php_phongo_timestamp_t *intern;
zend_error_handling error_handling;
char *s_increment;
phongo_zpp_char_len s_increment_len;
char *s_timestamp;
phongo_zpp_char_len s_timestamp_len;
php_phongo_timestamp_t *intern;
zend_error_handling error_handling;
zval *increment = NULL, *timestamp = NULL;

zend_replace_error_handling(EH_THROW, phongo_exception_from_phongo_domain(PHONGO_ERROR_INVALID_ARGUMENT), &error_handling TSRMLS_CC);
intern = Z_TIMESTAMP_OBJ_P(getThis());

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ss", &s_increment, &s_increment_len, &s_timestamp, &s_timestamp_len) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "zz", &increment, &timestamp) == FAILURE) {
zend_restore_error_handling(&error_handling TSRMLS_CC);
return;
}
zend_restore_error_handling(&error_handling TSRMLS_CC);

php_phongo_timestamp_init_from_string(intern, s_increment, s_increment_len, s_timestamp, s_timestamp_len TSRMLS_CC);
if (Z_TYPE_P(increment) == IS_LONG && Z_TYPE_P(timestamp) == IS_LONG) {
php_phongo_timestamp_init(intern, Z_LVAL_P(increment), Z_LVAL_P(timestamp) TSRMLS_CC);
return;
}

if (Z_TYPE_P(increment) == IS_LONG) {
convert_to_string(increment);
}

if (Z_TYPE_P(increment) != IS_STRING) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected increment to be an unsigned 32-bit integer or string, %s given", zend_get_type_by_const(Z_TYPE_P(increment)));
return;
}

if (Z_TYPE_P(timestamp) == IS_LONG) {
convert_to_string(timestamp);
}

if (Z_TYPE_P(timestamp) != IS_STRING) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected timestamp to be an unsigned 32-bit integer or string, %s given", zend_get_type_by_const(Z_TYPE_P(timestamp)));
return;
}

php_phongo_timestamp_init_from_string(intern, Z_STRVAL_P(increment), Z_STRLEN_P(increment), Z_STRVAL_P(timestamp), Z_STRLEN_P(timestamp) TSRMLS_CC);
}
/* }}} */

Expand Down
62 changes: 37 additions & 25 deletions src/BSON/UTCDateTime.c
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
/* External libs */
#include <bson.h>
#include <mongoc.h>
#include <math.h>

/* PHP Core stuff */
#include <php.h>
Expand Down Expand Up @@ -77,11 +78,7 @@ static bool php_phongo_utcdatetime_init_from_string(php_phongo_utcdatetime_t *in

errno = 0;

#if defined(PHP_WIN32)
milliseconds = _atoi64(s_milliseconds);
#else
milliseconds = bson_ascii_strtoll(s_milliseconds, &endptr, 10);
#endif

/* errno will set errno if conversion fails; however, we do not need to
* specify the type of error.
Expand Down Expand Up @@ -161,49 +158,64 @@ static bool php_phongo_utcdatetime_init_from_date(php_phongo_utcdatetime_t *inte
return true;
}

/* {{{ proto void UTCDateTime::__construct([string|DateTimeInterface $milliseconds = null])
/* {{{ proto void UTCDateTime::__construct([int|float|string|DateTimeInterface $milliseconds = null])
Construct a new BSON UTCDateTime type from either the current time,
milliseconds since the epoch, or a DateTimeInterface object. */
milliseconds since the epoch, or a DateTimeInterface object. Defaults to the
current time. */
PHP_METHOD(UTCDateTime, __construct)
{
php_phongo_utcdatetime_t *intern;
zend_error_handling error_handling;
zval *datetime = NULL;
zval *milliseconds = NULL;

zend_replace_error_handling(EH_THROW, phongo_exception_from_phongo_domain(PHONGO_ERROR_INVALID_ARGUMENT), &error_handling TSRMLS_CC);
intern = Z_UTCDATETIME_OBJ_P(getThis());

if (zend_parse_parameters_ex(ZEND_PARSE_PARAMS_QUIET, ZEND_NUM_ARGS() TSRMLS_CC, "|o!", &datetime) == SUCCESS) {
if (datetime == NULL) {
php_phongo_utcdatetime_init_from_current_time(intern);
} else if (instanceof_function(Z_OBJCE_P(datetime), php_date_get_date_ce() TSRMLS_CC)) {
php_phongo_utcdatetime_init_from_date(intern, Z_PHPDATE_P(datetime));
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|z!", &milliseconds) == FAILURE) {
zend_restore_error_handling(&error_handling TSRMLS_CC);
return;
}
zend_restore_error_handling(&error_handling TSRMLS_CC);

if (milliseconds == NULL) {
php_phongo_utcdatetime_init_from_current_time(intern);
return;
}

if (Z_TYPE_P(milliseconds) == IS_OBJECT) {
if (instanceof_function(Z_OBJCE_P(milliseconds), php_date_get_date_ce() TSRMLS_CC)) {
php_phongo_utcdatetime_init_from_date(intern, Z_PHPDATE_P(milliseconds));
#if PHP_VERSION_ID >= 50500
} else if (instanceof_function(Z_OBJCE_P(datetime), php_date_get_immutable_ce() TSRMLS_CC)) {
php_phongo_utcdatetime_init_from_date(intern, Z_PHPDATE_P(datetime));
} else if (instanceof_function(Z_OBJCE_P(milliseconds), php_date_get_immutable_ce() TSRMLS_CC)) {
php_phongo_utcdatetime_init_from_date(intern, Z_PHPDATE_P(milliseconds));
#endif
} else {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected instance of DateTimeInterface, %s given", ZSTR_VAL(Z_OBJCE_P(datetime)->name));
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected instance of DateTimeInterface, %s given", ZSTR_VAL(Z_OBJCE_P(milliseconds)->name));
}
return;
}

zend_restore_error_handling(&error_handling TSRMLS_CC);
if (Z_TYPE_P(milliseconds) == IS_LONG) {
php_phongo_utcdatetime_init(intern, Z_LVAL_P(milliseconds));
return;
}

{
char *s_milliseconds;
phongo_zpp_char_len s_milliseconds_len;
if (Z_TYPE_P(milliseconds) == IS_DOUBLE) {
char tmp[24];
int tmp_len;

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &s_milliseconds, &s_milliseconds_len) == FAILURE) {
zend_restore_error_handling(&error_handling TSRMLS_CC);
return;
}
tmp_len = snprintf(tmp, sizeof(tmp), "%.0f", Z_DVAL_P(milliseconds) > 0 ? floor(Z_DVAL_P(milliseconds)) : ceil(Z_DVAL_P(milliseconds)));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trunc() is available in C99, but I didn't want to risk any Windows build issues for older PHP issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to convert to a long straight away here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally used convert_to_long(), but you mentioned it would lose data. Although you were referring to precision after milliseconds, which I later explained we don't care about, that solution did have issues on 32-bit platforms. This method ensures that values larger than INT32_MAX in a float will convert properly on 32-bit.


php_phongo_utcdatetime_init_from_string(intern, s_milliseconds, s_milliseconds_len TSRMLS_CC);
php_phongo_utcdatetime_init_from_string(intern, tmp, tmp_len TSRMLS_CC);
return;
}

zend_restore_error_handling(&error_handling TSRMLS_CC);
if (Z_TYPE_P(milliseconds) != IS_STRING) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected integer or string, %s given", zend_get_type_by_const(Z_TYPE_P(milliseconds)));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HHVM only throws "Expected instance of DateTimeInterface" and "Error parsing %s as 64-bit integer" exceptions, so it may need a PR for this (see here).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the HHVM message is much clearer as an API point of view. The "error parsing x as 64-bit integer" is what people are interested in. Whether it's an "integer", "float", or "string" is irrelevant. The current message in your code also misses "float".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I omitted "float" from the message since the cast to a long is just for the user's convenience. I wasn't planning on documenting that we accept a float, since it is being truncated. This is mainly to skirt around scalar type hint errors.

In HHVM's case, the invalid argument is cast to a string value for the exception message. That leads to some odd output for null (empty string), boolean ("1" and "0"), and would likely emit some notice on an array. The type error I was using here was consistent with what we already did for ReadPreference, WriteConcern, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - add some tests for these additional types, so I can test HHVM against it as well? (If you haven't done so)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are in bson-utcdatetime_error-004.phpt.

return;
}

php_phongo_utcdatetime_init_from_string(intern, Z_STRVAL_P(milliseconds), Z_STRLEN_P(milliseconds) TSRMLS_CC);
}
/* }}} */

Expand Down
24 changes: 24 additions & 0 deletions tests/bson/bson-timestamp-serialization_error-004.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
MongoDB\BSON\Timestamp unserialization requires strings to parse as 64-bit integers
--FILE--
<?php

require_once __DIR__ . '/../utils/tools.php';

echo throws(function() {
unserialize('C:22:"MongoDB\BSON\Timestamp":60:{a:2:{s:9:"increment";s:4:"1.23";s:9:"timestamp";s:4:"5678";}}');
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

echo throws(function() {
unserialize('C:22:"MongoDB\BSON\Timestamp":60:{a:2:{s:9:"increment";s:4:"1234";s:9:"timestamp";s:4:"5.67";}}');
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Error parsing "1.23" as 64-bit integer increment for MongoDB\BSON\Timestamp initialization
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Error parsing "5.67" as 64-bit integer timestamp for MongoDB\BSON\Timestamp initialization
===DONE===
12 changes: 5 additions & 7 deletions tests/bson/bson-timestamp-set_state_error-004.phpt
Original file line number Diff line number Diff line change
@@ -1,26 +1,24 @@
--TEST--
MongoDB\BSON\Timestamp::__set_state() with broken numeric strings
--SKIPIF--
<?php if (8 !== PHP_INT_SIZE) { die('skip Only for 64-bit platform'); } ?>
MongoDB\BSON\Timestamp::__set_state() requires strings to parse as 64-bit integers
--FILE--
<?php
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was erroneously copy-pasted when the test was created in d25e565.


require_once __DIR__ . '/../utils/tools.php';

echo throws(function() {
MongoDB\BSON\Timestamp::__set_state(['increment' => 'broken', 'timestamp' => '5678']);
MongoDB\BSON\Timestamp::__set_state(['increment' => '1.23', 'timestamp' => '5678']);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test would have been problematic for Windows since _atoi64() stops parsing at the first invalid character without raising an error; however, bson_ascii_strtoll() is more strict.

}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

echo throws(function() {
MongoDB\BSON\Timestamp::__set_state(['increment' => '1234', 'timestamp' => 'broken']);
MongoDB\BSON\Timestamp::__set_state(['increment' => '1234', 'timestamp' => '5.67']);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Error parsing "broken" as 64-bit integer increment for MongoDB\BSON\Timestamp initialization
Error parsing "1.23" as 64-bit integer increment for MongoDB\BSON\Timestamp initialization
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Error parsing "broken" as 64-bit integer timestamp for MongoDB\BSON\Timestamp initialization
Error parsing "5.67" as 64-bit integer timestamp for MongoDB\BSON\Timestamp initialization
===DONE===
24 changes: 24 additions & 0 deletions tests/bson/bson-timestamp_error-005.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
MongoDB\BSON\Timestamp constructor requires strings to parse as 64-bit integers
--FILE--
<?php

require_once __DIR__ . '/../utils/tools.php';

echo throws(function() {
new MongoDB\BSON\Timestamp('1.23', '5678');
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

echo throws(function() {
new MongoDB\BSON\Timestamp('1234', '5.67');
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Error parsing "1.23" as 64-bit integer increment for MongoDB\BSON\Timestamp initialization
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Error parsing "5.67" as 64-bit integer timestamp for MongoDB\BSON\Timestamp initialization
===DONE===
46 changes: 46 additions & 0 deletions tests/bson/bson-timestamp_error-006.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
--TEST--
MongoDB\BSON\Timestamp constructor requires integer or string arguments
--FILE--
<?php

require_once __DIR__ . '/../utils/tools.php';

$invalidValues = [null, 1234.5, true, [], new stdClass];

foreach ($invalidValues as $invalidValue) {
echo throws(function() use ($invalidValue) {
new MongoDB\BSON\Timestamp($invalidValue, 0);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";
}

foreach ($invalidValues as $invalidValue) {
echo throws(function() use ($invalidValue) {
new MongoDB\BSON\Timestamp(0, $invalidValue);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";
}

?>
===DONE===
<?php exit(0); ?>
--EXPECTF--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected increment to be an unsigned 32-bit integer or string, %r(null|NULL)%r given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected increment to be an unsigned 32-bit integer or string, %r(double|float)%r given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected increment to be an unsigned 32-bit integer or string, boolean given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected increment to be an unsigned 32-bit integer or string, array given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected increment to be an unsigned 32-bit integer or string, object given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected timestamp to be an unsigned 32-bit integer or string, %r(null|NULL)%r given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected timestamp to be an unsigned 32-bit integer or string, %r(double|float)%r given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected timestamp to be an unsigned 32-bit integer or string, boolean given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected timestamp to be an unsigned 32-bit integer or string, array given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected timestamp to be an unsigned 32-bit integer or string, object given
===DONE===
32 changes: 32 additions & 0 deletions tests/bson/bson-utcdatetime-007.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
--TEST--
MongoDB\BSON\UTCDateTime constructor truncates floating point values
--FILE--
<?php

$tests = [
new MongoDB\BSON\UTCDateTime(1416445411987.0),
new MongoDB\BSON\UTCDateTime(2147483647.0),
new MongoDB\BSON\UTCDateTime(1234.5678),
];

foreach ($tests as $test) {
var_dump($test);
}

?>
===DONE===
<?php exit(0); ?>
--EXPECTF--
object(MongoDB\BSON\UTCDateTime)#%d (%d) {
["milliseconds"]=>
string(13) "1416445411987"
}
object(MongoDB\BSON\UTCDateTime)#%d (%d) {
["milliseconds"]=>
string(10) "2147483647"
}
object(MongoDB\BSON\UTCDateTime)#%d (%d) {
["milliseconds"]=>
string(4) "1234"
}
===DONE===
4 changes: 2 additions & 2 deletions tests/bson/bson-utcdatetime-serialization_error-002.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ MongoDB\BSON\UTCDateTime unserialization requires "milliseconds" string to parse
require_once __DIR__ . '/../utils/tools.php';

echo throws(function() {
unserialize('C:24:"MongoDB\BSON\UTCDateTime":40:{a:1:{s:12:"milliseconds";s:7:"INVALID";}}');
unserialize('C:24:"MongoDB\BSON\UTCDateTime":42:{a:1:{s:12:"milliseconds";s:9:"1234.5678";}}');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have kept the length of the argument the same, as to not have to mess with lengths in hardcoded serialized strings. But that's done now.

}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

/* TODO: Add tests for out-of-range values once CDRIVER-1377 is resolved */
Expand All @@ -16,5 +16,5 @@ echo throws(function() {
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Error parsing "INVALID" as 64-bit integer for MongoDB\BSON\UTCDateTime initialization
Error parsing "1234.5678" as 64-bit integer for MongoDB\BSON\UTCDateTime initialization
===DONE===
5 changes: 3 additions & 2 deletions tests/bson/bson-utcdatetime-set_state_error-002.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@
MongoDB\BSON\UTCDateTime::__set_state() requires "milliseconds" string to parse as 64-bit integer
--FILE--
<?php

require_once __DIR__ . '/../utils/tools.php';

echo throws(function() {
MongoDB\BSON\UTCDateTime::__set_state(['milliseconds' => 'INVALID']);
MongoDB\BSON\UTCDateTime::__set_state(['milliseconds' => '1234.5678']);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

/* TODO: Add tests for out-of-range values once CDRIVER-1377 is resolved */
Expand All @@ -15,5 +16,5 @@ echo throws(function() {
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Error parsing "INVALID" as 64-bit integer for MongoDB\BSON\UTCDateTime initialization
Error parsing "1234.5678" as 64-bit integer for MongoDB\BSON\UTCDateTime initialization
===DONE===
Loading