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

jmikola · 2016-11-22T20:16:35Z

https://jira.mongodb.org/browse/PHPC-790
https://jira.mongodb.org/browse/PHPC-804

bson_ascii_strtoll() will support range checking in libbson 1.5.0 (CDRIVER-1377), so there is no longer any benefit of using _atoi64() on Windows. This will make parsing consistent across both platforms, since _atoi64() does not report errors for non-integer strings (e.g. "1.2").

jmikola · 2016-11-22T20:35:40Z

src/BSON/Timestamp.c

@@ -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);


_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.

jmikola · 2016-11-22T21:08:32Z

src/BSON/Timestamp.c

+	}
+
+	if (Z_TYPE_P(increment) != IS_STRING) {
+		phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected increment to be integer or string, %s given", zend_get_type_by_const(Z_TYPE_P(increment)));


HHVM will throw "Expected timestamp to be an unsigned 32-bit integer or string" on type errors (see: here). I'm happy to submit a PR to revise that once this is approved.

But the HHVM warning is not wrong? It has to be an unsigned 32-bit integer. I don't see you test for this here, but it's in php_phongo_timestamp_init I believe.

php_phongo_timestamp_init() performs range checking and will throw the "Expected increment to be an unsigned 32-bit integer, %d given" exceptions. I can change this to "Expected timestamp to be an unsigned 32-bit integer or string" and remove the "%s given" bit. Sharing the unexpected type was for consistency with our other exceptions for type errors.

I think that for HHVM, we should add the "%s given" bit, and you change the message to "Expected increment to be an unsigned 32-bit integer or string, %s given" — does that make sense? (note, in your reply you used timestamp and not increment).

Works for me. The timestamp/increment was indeed a typo on my part. I'll take care of the HHVM PR.

jmikola · 2016-11-22T21:12:36Z

src/BSON/UTCDateTime.c

 	}

-	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)));


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).

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".

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.

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

Tests are in bson-utcdatetime_error-004.phpt.

jmikola · 2016-11-22T21:14:59Z

tests/bson/bson-timestamp-set_state_error-004.phpt

@@ -8,19 +8,19 @@ MongoDB\BSON\Timestamp::__set_state() with broken numeric strings
 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']);


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.

jmikola · 2016-11-22T21:15:34Z

tests/bson/bson-timestamp_error-006.phpt

+OK: Got MongoDB\Driver\Exception\InvalidArgumentException
+Expected increment to be integer or string, %r(null|NULL)%r given
+OK: Got MongoDB\Driver\Exception\InvalidArgumentException
+Expected increment to be integer or string, %r(double|float)%r given


Pattern is necessary for PHP 5 and 7.

jmikola · 2016-11-22T21:15:41Z

tests/bson/bson-timestamp_error-006.phpt

+<?php exit(0); ?>
+--EXPECTF--
+OK: Got MongoDB\Driver\Exception\InvalidArgumentException
+Expected increment to be integer or string, %r(null|NULL)%r given


Pattern is necessary for PHP and HHVM.

jmikola · 2016-11-22T21:17:38Z

tests/bson/bson-timestamp-set_state_error-004.phpt

@@ -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'); } ?>


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

derickr

Mostly fine, but some queries about error messages.

derickr · 2016-11-23T11:11:34Z

src/BSON/Timestamp.c

+	}
+
+	if (Z_TYPE_P(increment) != IS_STRING) {
+		phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected increment to be integer or string, %s given", zend_get_type_by_const(Z_TYPE_P(increment)));


But the HHVM warning is not wrong? It has to be an unsigned 32-bit integer. I don't see you test for this here, but it's in php_phongo_timestamp_init I believe.

derickr · 2016-11-23T11:14:30Z

src/BSON/UTCDateTime.c

-			zend_restore_error_handling(&error_handling TSRMLS_CC);
-			return;
-		}
+	if (Z_TYPE_P(milliseconds) == IS_DOUBLE) {


This could cause data loss, and I'm not keen about that :-/

I admittedly has some trouble reading through all of the zend_dval_to_lval() cases to see if this would be an issue. Would you agree that the best solution is to convert this to a string via "%f.0"?

No, I don't think that's needed. It's just that we ignore a user's sub-milliseconds, because they passed in a float, without notifying them.

If this isn't a matter of stuffing a double into a 32-bit integer, I don't understand the concern. UTCDateTime is documented as only representing milliseconds and the purpose of this change was because users previously had to use microtime(true) * 1000 to create a UTCDateTime with the current time. This is no different than a userland method using an (int) cast to ensure it's working with an integer (before scalar type hinting was available).

That said, I think the 32-bit issue is still a concern worth addressing. microtime(true) * 1000 will easily result in values that exceed INT32_MAX.

Yes, I agree. That should be handled.

derickr · 2016-11-23T11:18:15Z

src/BSON/UTCDateTime.c

 	}

-	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)));


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".

derickr · 2016-11-23T11:19:33Z

tests/bson/bson-utcdatetime-serialization_error-002.phpt

@@ -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";}}');


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.

derickr · 2016-11-23T17:00:22Z

src/BSON/UTCDateTime.c

-			zend_restore_error_handling(&error_handling TSRMLS_CC);
-			return;
-		}
+	if (Z_TYPE_P(milliseconds) == IS_DOUBLE) {


Yes, I agree. That should be handled.

Changing "s" to "z" in zend_parse_parameters() allows integers to be accepted in strict types mode. If all necessary parameters are integers, we can also skip conversion to a string and back to a 64-bit integer.

jmikola · 2016-11-23T18:07:22Z

src/BSON/UTCDateTime.c

-			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)));


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

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

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.

jmikola force-pushed the phpc-790 branch 4 times, most recently from d972e3c to 23ca7ed Compare November 22, 2016 21:10

jmikola commented Nov 22, 2016

View reviewed changes

derickr suggested changes Nov 23, 2016

View reviewed changes

derickr approved these changes Nov 23, 2016

View reviewed changes

PHPC-804: Relax zpp types in Timestamp and UTCDateTime constructors

8d7e358

Changing "s" to "z" in zend_parse_parameters() allows integers to be accepted in strict types mode. If all necessary parameters are integers, we can also skip conversion to a string and back to a 64-bit integer.

jmikola force-pushed the phpc-790 branch from e863027 to 9f6aa0c Compare November 23, 2016 17:15

jmikola added 2 commits November 23, 2016 13:06

PHPC-790: UTCDateTime constructor should truncate floats

6fd69af

Remove 64-bit platform requirement

a916713

jmikola force-pushed the phpc-790 branch from 9f6aa0c to a916713 Compare November 23, 2016 18:06

jmikola commented Nov 23, 2016

View reviewed changes

jmikola merged commit a916713 into mongodb:master Nov 25, 2016

jmikola added a commit that referenced this pull request Nov 25, 2016

Merge pull request #467

c729150

jmikola deleted the phpc-790 branch November 25, 2016 17:06

jmikola mentioned this pull request Jul 31, 2023

PHPC-2230: Drop support for PHP 7.2 and 7.3 #1450

Merged

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

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

Uh oh!

Conversation

jmikola commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!