tlocke / pg8000

A Pure-Python PostgreSQL Driver
BSD 3-Clause "New" or "Revised" License
515 stars 46 forks source link

Negative MONEY amounts not supported in v1.19.1 #69

Closed mrmarkfrench closed 3 years ago

mrmarkfrench commented 3 years ago

In version 1.19.1 (but not earlier versions), columns of type MONEY are returned as the absolute value of the value actually stored in the table.

This has... significant... impact on financial calculations based on the resulting data.

mrmarkfrench commented 3 years ago

I should note that this problem is only on output. Writing negative values to the table is unaffected.

tlocke commented 3 years ago

Oops, thanks for reporting this. Won't be able to fix it for about 24 hrs though. If you need an urgent fix you can register custom input / output functions. To be honest I'm not sure how to handle the MONEY type. I included it more for completeness than anything else. Obviously we need to fix the negative bug. What about the currency prefix though? And should we be returning a Decimal or a string? Your thoughts would be much appreciated. If you go down the route of custom input / output functions it would be a great help if you could share them here. Thanks 🙂

mrmarkfrench commented 3 years ago

For now I fixed it by pinning us to version 1.19.0 and ignoring even minor version increments (previously we'd allowed minor version increments on the theory we'd automatically pick up bug fixes).

mrmarkfrench commented 3 years ago

The currency symbol isn't set on the column, it's just provided based on the locale the number was formatted in, so it doesn't provide any useful information. We expect the currency symbol (because Postgres provides it by default), but we just pre-process the string being returned and convert it to a Decimal. As a result, if you were to give us back a Decimal, I would not complain.

The value returned from Postgres is really a locale-aware numeric string with a currency symbol added before the most significant digit (but after the negation symbol, if there is one). That's nice if your plan is to just dump it straight to the UI, but for our purposes we're using it to perform calculations, and so getting back a numeric type would be most convenient for our specific use case (and by "numeric" I mean Decimal, please for the love of everything sacred do not convert monetary values to floats).

mrmarkfrench commented 3 years ago

So now that I've had a chance to collect my thoughts a bit, here's my less rambling answer: You should treat values from a column of type MONEY exactly like you treat any other fixed-precision numeric value. That likely means you return a Decimal.

tlocke commented 3 years ago

The problem is that the value comes back from the server with the currency string embedded in in it and the whole formatting is locale dependent. Rather than trying to parse a locale dependent string, pg8000 should probably just return the string it gets.

So out of curiosity, why don't you use a numeric type in your application, instead of a money type?

mrmarkfrench commented 3 years ago

Yeah. As I said, we're just forcing it to Decimal when we ingest it, so if that behavior continues to be that the library returns a formatted string, we're already set up to handle that.

The answer to your other question is a bit pedantic, but it boils down to "because these values represent amounts of money", and having the type set in the DB that way helps to hint to people what values they should expect, and also helps set some guidelines around audit trail requirements if things change, (and/or immutability of many values), and if it would be safe/allowable to use a floating point representation of those values in any calculations.

tlocke commented 3 years ago

I've just done release 1.19.2 https://github.com/tlocke/pg8000/releases/tag/1.19.2 which should revert back to the old behaviour of returning the MONEY type as a str. I'll close the issue now, but feel free to re-open it if there's still a problem though.