Skip to content
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

Ctrl-C on JSONB string creates badly quoted contents #8157

Closed
riuttner opened this issue Nov 21, 2024 · 14 comments
Closed

Ctrl-C on JSONB string creates badly quoted contents #8157

riuttner opened this issue Nov 21, 2024 · 14 comments
Assignees
Milestone

Comments

@riuttner
Copy link

Trying to copy any JSONB contents from Data Output window by keystroke Ctrl-C ends up with a copy buffer containing a double-quoted string, having all double quotes inside doubled.

Execute:
select '{"a":"bbb", "c":"ddd"}'::jsonb

Double click single result opens a view showing:

{
   "a": "bbb",
   "c": "ddd"
}

Select all lines, press Ctrl-C, paste contents anywhere in pgadmin or in any text editor. This will yield the wrong result:
"{""a"": ""bbb"", ""c"": ""ddd""}"

Expected result is:
{"a":"bbb", "c":"ddd"}

This bug also appears for contents of type JSON, not only for JSONB. I could observe it all the time after upgrading from 8.12 to 8.13. Apparently the latest release just accidentally uses the wrong quotes for copied JSON strings, which internally also leads to doubled double quotes.

Tested in environment:
Version
8.13
Application Mode
Desktop
Commit:
b278265 2024-11-11
Current User
[email protected]
Electron Version
33.2.0
Browser
Chrome 130.0.6723.118
Operating System
Windows-10-10.0.19045-SP0

@Bilge
Copy link

Bilge commented Nov 30, 2024

Also applies to regular JSON. This also breaks copying EXPLAINs, which is a huge break. The preview popover window displays correct formatting, but copying is disabled, for some bizarre reason, forcing you to copy the entire cell contents, which are double-encoded.

@khirasaki
Copy link

Previous use cases that broke for me with the change, just to confirm that they get fixed as part of this fix (MacOS):

  1. In order to open a cell edit window, you have to first select the cell in the table. If you attempt to copy selected text from the edit window, it doesn't work as expected, because the cell is also selected and that gets added to the clipboard.
  2. If I select a cell (not opening the edit window) and copy it, the string copied to the clipboard includes double quotes around it. So if I have a cell with a UUID, select it, hit copy, and then go to paste it in to an SQL query, it adds in double quotes, which I then have to delete.

@adityatoshniwal
Copy link
Contributor

Previous use cases that broke for me with the change, just to confirm that they get fixed as part of this fix (MacOS):

  1. In order to open a cell edit window, you have to first select the cell in the table. If you attempt to copy selected text from the edit window, it doesn't work as expected, because the cell is also selected and that gets added to the clipboard.

This will be fixed in upcoming release.

  1. If I select a cell (not opening the edit window) and copy it, the string copied to the clipboard includes double quotes around it. So if I have a cell with a UUID, select it, hit copy, and then go to paste it in to an SQL query, it adds in double quotes, which I then have to delete.

This is an expected behaviour. You can customize it from Preferences/Settings -> Query Tool -> Result Grid.

@riuttner
Copy link
Author

riuttner commented Dec 6, 2024

@adityatoshniwal Many thanks for your explanation! Your proposed solution via Preferences/Settings works for me.

I agree that this is "expected behaviour" from developer's point of view, because somebody obviously implemented a general setting for quoting that is totally independent from the type of data to be quoted. But I am sure that no user ever will expect a database tool to quote complex contents of a known type such that the contents is rendered unusable - the questionable idea behind this feature IMHO is that a general quoting rule makes sense at all. If there is really a demanding need for a feature like this, I would at least expect a second setting "apply general quoting rule even against implicit rules for known types", defaulting to false. Of course this would require some internal code changes, because data types might not be available on deep call levels. But on the long run, it might be a better path to not try to overrule logic by settings.

@adityatoshniwal
Copy link
Contributor

adityatoshniwal commented Dec 6, 2024

@adityatoshniwal Many thanks for your explanation! Your proposed solution via Preferences/Settings works for me.

I agree that this is "expected behaviour" from developer's point of view, because somebody obviously implemented a general setting for quoting that is totally independent from the type of data to be quoted. But I am sure that no user ever will expect a database tool to quote complex contents of a known type such that the contents is rendered unusable - the questionable idea behind this feature IMHO is that a general quoting rule makes sense at all. If there is really a demanding need for a feature like this, I would at least expect a second setting "apply general quoting rule even against implicit rules for known types", defaulting to false. Of course this would require some internal code changes, because data types might not be available on deep call levels. But on the long run, it might be a better path to not try to overrule logic by settings.

Hi @riuttner,

You're assumption that all the cells are quoted is incorrect. The default value of "Result copy quoting" is String, which says only string data types will be quoted. For eg - numbers and boolean are not quoted. Secondly, the copied data can be pasted directly on the grid - which should work. Who would want a string with spaces to be not quoted?
You can always choose None if you don't want pgAdmin to use any quoting which I think is same as "apply general quoting rule even against implicit rules for known types".

@Bilge
Copy link

Bilge commented Dec 6, 2024

Why would anyone ever want this behaviour? Moreover, this has changed recently, whether due to a new or changed option, because I haven't reinstalled; just upgraded.

@adityatoshniwal
Copy link
Contributor

Why would anyone ever want this behaviour? Moreover, this has changed recently, whether due to a new or changed option, because I haven't reinstalled; just upgraded.

This particular bug is accepted and fixed. Will be available in release next week.

@khirasaki
Copy link

@adityatoshniwal re: setting to quote all/strings/none

I initially set this to "none," which satisfied the previous behavior before the change had been made. But then based on your comment above about "strings," I turned that on.

However, with that setting, when I copy a uuid, that gets interpreted as a "string" and so gets quotes put around it. Is that the intent of what "String" means in that setting?

Also, if I select an array of UUIDs (the whole cell), then when I copy and paste that, it also has quotes around it, even though I don't think of an array of UUIDs as a "string".

However, if I select a cell that is a boolean, it does not include quotes, which is expected.

@khirasaki
Copy link

Other related issue is that if I make the change in settings (going from "string" to "none"), that functionality does not appear to immediately take effect in Result Tables that are currently open. If I open a new one, it does, but existing tabs keep the previous setting. Ideally this behavior would be global instantly.

@riuttner
Copy link
Author

riuttner commented Dec 8, 2024

@adityatoshniwal Thanks again for your explanations, now I have a better understanding of what is still wrong IMHO.

For testing, I changed back my setting to Result copy quote character to double quote, and Result copy quoting to Strings.

Now I execute a select of a single jsonb object of type json string via:

select '"abcd"'::jsonb;

I am using single quotes to create a plain string, needed by Postgres to express a json(b) literal. The double quotes inside create a jsonb string, which can be easily checked by selecting

select jsonb_typeof('"abcd"'::jsonb);

which yields result text string as expected (saying that we have a json(b) contents of json type string (i.e. not postgres type varchar or text).

Copying this string from the grid with my current setting yields "string" which I can accept. However, if I go back to the first select command:

select '"abcd"'::jsonb;

and copy its result from the grid, I get (as I expect due to the nature of the current implementation) the following copy buffer contents:

""string""

This is simply wrong in my eyes due to the fact that a json(b) contents is not a string, thus should not be quoted due to the choice in my setting, even though there are a lot of transitions where it makes sense to internally transform json(b) to string (or vice versa) such that it can be displayed or that it can be parsed as literal. But the quoting defined by my setting should never affect json(b) contents, as there is no case where double quotes can be applied usefully.

Even single quotes are not that useful as it might look at first glance, as most real-life json data do not contain single quotes. And just for pasting a json contents to a place where it has to be parsed, it is normally enough to first once press the key for single quote, which will create a pair of them. Then you can paste your (unquoted) JSON contents. BTW: This would be the same way as you operate when you paste existing contents copied from a valid json file to pgadmin. In case of existing single quotes in your JSON contents you would anyway run into a syntax error.

Thus I propose to not quote JSON data unless somebody has a setting for Result copy quoting on all data types (which might make sense if you copy for pasting to special formats like CSV, but not in general).

Anyway I can live with the current implementation, just by setting my result copy quote character to single quote.

@adityatoshniwal
Copy link
Contributor

adityatoshniwal commented Dec 9, 2024

Anyway I can live with the current implementation, just by setting my result copy quote character to single quote.

I understand your concern. But, the current implementation is based on CSV format. pgAdmin produces copied data in a format which can be easily imported. Consider a scenario where user need to import a CSV file with JSON rows. If the quotes are not used then it will fail.

What I think we can do is introduce a new option Strings + JSON along with None, All, Strings. As part of Strings JSON will not be quoted. JSON will be quoted when Strings + JSON and it will be the default selected. Does it work?

@riuttner
Copy link
Author

riuttner commented Dec 9, 2024

Your proposal for the additional option is a good idea in my eyes. Please do it that way.

@adityatoshniwal
Copy link
Contributor

Your proposal for the additional option is a good idea in my eyes. Please do it that way.

New feature request added - #8247

@pravesh-sharma
Copy link
Contributor

Issue fixed, verified on snapshot build.

@pravesh-sharma pravesh-sharma moved this from In Testing to ✅ Done in Current Sprint (182) Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

6 participants