-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-5395 - Add convert_decimal to CodecOptions #2491
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
Conversation
Will update the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor comments
@@ -791,14 +812,15 @@ int convert_codec_options(PyObject* self, PyObject* options_obj, codec_options_t | |||
|
|||
options->unicode_decode_error_handler = NULL; | |||
|
|||
if (!PyArg_ParseTuple(options_obj, "ObbzOOb", | |||
if (!PyArg_ParseTuple(options_obj, "ObbzOObb", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stared at this for a while but then found: https://docs.python.org/3/c-api/arg.html. So, just confirming that's a format string for the args below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol i was sitting on this one too, googled it, found that same site, was still confused / overwhelemed by words so then i asked mongo-gpt HAHA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup this is a format string.
@@ -1436,6 +1462,16 @@ static int _write_element_to_buffer(PyObject* self, buffer_t buffer, | |||
in_fallback_call); | |||
Py_DECREF(binary_value); | |||
return result; | |||
} else if (options->convert_decimal && _check_decimal(value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after case 100 or so and if case 19 wasn't the case, auto-convert ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with how the rest of _write_element_to_buffer
works, we check the converison edge cases such as this after the native BSON case
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR doesn't add new functionality since users can already configure a CodecOptions that automatically encodes Decimal, for example:
class DecimalEncoder(TypeEncoder):
@property
def python_type(self):
return Decimal
def transform_python(self, value):
return Decimal128(value)
DECIMAL_CODECOPTS = CodecOptions(type_registry=TypeRegistry([DecimalEncoder()]))
TypeRegistry can also be configured to auto-decode Decimal128 to decimal by implementing TypeDecoder.
Given this, is it necessary to add this convert_decimal
flag?
converted = Decimal128(value) | ||
return b"\x13" + name + converted.bid | ||
else: | ||
raise InvalidDocument("decimal.Decimal must be converted to bson.decimal128.Decimal128.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest mentioning the convert_decimal option in this error message.
The I'm fine to make this auto-conversion the default instead of adding a flag, but then we would explicitly disallow users from making their own custom Encoders for |
The Decimal128 specification explicitly does not allow this kind of auto-conversion. Closed. |
Context
We currently reject Python
decimal.Decimal
instances and require the user to manually convert to Decimal128.This PR adds a new
convert_decimal
kwarg to CodecOptions that enables automatic conversion ofdecimal.Decimal
toDecimal128
during BSON encoding.Summary and example of changes
Current behavior and new behavior when
convert_decimal=False
(default)New behavior when
convert_decimal=True
Testing
test/test_bson.py
has a new testTestCodecOptions.test_convert_decimal
to verify correctness.A changelog entry will be added in this PR if we determine that this is the correct approach to move ahead with.