Fix Decimal('Infinity') adaptation
#1735
Conversation
edgarrmondragon
force-pushed
the
1724-decimal-infinity
branch
from
0216931 to
fd76f3a
Compare
edgarrmondragon
force-pushed
the
1724-decimal-infinity
branch
from
fd76f3a to
464d17e
Compare
| @@ -74,12 +74,15 @@ def testDecimal(self): | |||
| self.failUnless(str(s) == "NaN", "wrong decimal quoting: " + str(s)) | |||
| self.failUnless(type(s) == decimal.Decimal, | |||
| "wrong decimal conversion: " + repr(s)) | |||
|
|
|||
| @testutils.skip_before_postgres(14, 0) | |||
There was a problem hiding this comment.
SELECT 'Infinity'::numeric AS foo causes
psycopg2.errors.InvalidTextRepresentation: invalid input syntax for type numeric: "Infinity"
in Postgres 12 and 13.
There was a problem hiding this comment.
Oh, now that I think about it more we should probably rather check the server version when adapting Decimal('Infinity'), and fall back to NaN'::numeric on Postgres < 14.
There was a problem hiding this comment.
Uhm... Thinking a bit harder abut it: is it even a good idea to change the way inf is converted? Yes, converting it to nan wasn't the right thing to do, but maybe there is code that assumes that behaviour.
I am reconsidering the opportunity of making this change.
Note: psycopg 3 doesn't check for version and doesn't convert inf -> nan. Trying to send an inf to a server that doesn't support it results in a server error, which is the right thing to do.
There was a problem hiding this comment.
I agree. Either option (converting it or failing) is a potential breaking change. Happy to close this if you think we should err on preserving current behavior.
| @@ -47,11 +47,24 @@ pdecimal_getquoted(pdecimalObject *self, PyObject *args) | |||
| } | |||
| goto output; | |||
| } | |||
| else if (check) { | |||
|
|
|||
| check = PyObject_CallMethod(self->wrapped, "is_nan", NULL); | |||
There was a problem hiding this comment.
This code is not checking that check is NULL (which is in case of error).
It also leaks a refcount, as you overwrite check without a DECREF.
| res = Bytes_FromString("'NaN'::numeric"); | ||
| goto end; | ||
| } | ||
|
|
||
| /* If the decimal is not finite and not NaN, it must be infinity, | ||
| * so all that is left is to check if it is positive or negative. */ | ||
| check = PyObject_CallMethod(self->wrapped, "is_signed", NULL); |
There was a problem hiding this comment.
Same problem: no error checking, no decref.
| @@ -74,12 +74,15 @@ def testDecimal(self): | |||
| self.failUnless(str(s) == "NaN", "wrong decimal quoting: " + str(s)) | |||
| self.failUnless(type(s) == decimal.Decimal, | |||
| "wrong decimal conversion: " + repr(s)) | |||
|
|
|||
| @testutils.skip_before_postgres(14, 0) | |||
There was a problem hiding this comment.
Uhm... Thinking a bit harder abut it: is it even a good idea to change the way inf is converted? Yes, converting it to nan wasn't the right thing to do, but maybe there is code that assumes that behaviour.
I am reconsidering the opportunity of making this change.
Note: psycopg 3 doesn't check for version and doesn't convert inf -> nan. Trying to send an inf to a server that doesn't support it results in a server error, which is the right thing to do.
| res = Bytes_FromString("'Infinity'::numeric"); | ||
| goto end; | ||
| } | ||
|
|
||
| /* is_finite() was introduced 2.5.1 < somewhere <= 2.5.4. |
There was a problem hiding this comment.
Wow, I'm not sure this code is still used 😄
|
Thank you very much for this MR, but as discussed it's probably not a good idea to do this in psycopg2. |
Closes #1724