Decode text results defensively when the client encoding cannot decode them (SQL_ASCII) - #1628
Decode text results defensively when the client encoding cannot decode them (SQL_ASCII)#1628DiegoDAF wants to merge 1 commit into
Conversation
function metadata When the client encoding cannot decode a text value (most commonly SQL_ASCII), psycopg returns text columns as raw bytes. Decode those values defensively, using the same guard as the completion metadata fix for dbcli#1405: - get_socket_directory() and get_timezone() now decode their results, so the prompt no longer raises a TypeError on Unix socket connections and the timezone startup message no longer shows a b'...' value. - functions() now decodes every metadata row before yielding, so the background completion refresh no longer dies in parse_defaults with a TypeError. Closes dbcli#1484 and dbcli#1518. Related: dbcli#1405.
|
@DiegoDAF nit: the PR checklist may seem boilerplate, but we prefer to have it. It gives us some confidence that the contributor familiarized themselves with our requirements. |
|
Instead of decoding bytes, why don't we set client_encoding to utf8 on |
|
I tried a different approach a few weeks/months ago, in #1629. It's not fully tested but I think it goes in a better direction than forcefully and blindly converting bytes to strings. |
|
Good question, and thanks @dbaty for #1629, I had not seen it. I set up a real The catch is that Setup: Reading the data: Reading identifiers from the catalog (this is issue #1405's case): So on a That said, #1629 is clearly better where it applies. On a They are not really competing, then. #1629 gets the encoding right when the data allows it; the defensive decode is what keeps pgcli usable when it does not. If you like, I can rework this PR into that shape: I can also contribute the |
Fixes #1518. Fixes #1484.
When the client encoding cannot decode a text value (most commonly
SQL_ASCII), psycopg returns text columns as rawbytesinstead ofstr. Three code paths were passing those values through unguarded, producing the crashes reported in #1518 and #1484:TypeError: expected str, not bytes.show time zoneresult is printed in the startup message, showing ab'...'-prefixed value.parse_defaults()killed the refresh thread withTypeError: can only concatenate str (not "int") to str.The fix decodes those values defensively when they come back as bytes, with the same guard the completion metadata fix for #1405 already uses (
decode("utf-8", "replace")): a_decode_if_bytes()helper, plus a_decode_row()variant for the function metadata rows, which contain array columns.Deliberately narrow in scope:
function_definition(),search_path(),schemata()anddatabases()also return text and are still unguarded, but they are not involved in the reported crashes. Guarding them can be a follow-up if anyone hits them.Reproduced against a real
SQL_ASCIIcluster (3396 catalog functions; all metadata fields come back as plainstrafter the fix). 6 unit tests: 3 fail without the fix, 3 pin the already-workingstrpath so it stays unchanged.Checklist
changelog.rst.AUTHORSfile (or it's already there).pip install pre-commit && pre-commit install).Sorry about the missing checklist, that was me writing the description by hand instead of starting from the template.
The third box is honestly unchecked, with a small finding attached:
.pre-commit-config.yamlpinsruff-pre-commitatv0.11.7, while[testenv:style]intox.iniinstallsruffunpinned (0.15.x today). The two disagree on formatting, so running the hook reformats code that the CI style job then wants reformatted back. I ranruff checkandruff formatmatching the CI instead, both clean. Happy to open a separate PR bumping the pre-commitrevif you would like them to agree.