Re: [pgAdmin4][Patch] - RM 3780 pgAdmin4 lacks ability to specify NULL values in CSV export

Hi Dave

On Thu, Dec 20, 2018 at 5:12 PM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

>
>
> On Thu, Dec 20, 2018 at 4:48 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Hi
>>
>> On Thu, Dec 20, 2018 at 10:09 AM Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Dave
>>>
>>> On Thu, Dec 20, 2018 at 3:08 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>
>>>> Hi
>>>>
>>>> When testing with quoting set to None, quote = " and delimiter = , I
>>>> get the following exception when I try to download:
>>>>
>>>> 2018-12-20 09:34:02,547: SQL pgadmin: Execute (with server cursor) for
>>>> server #2 - CONN:354106 (Query-id: 4121147):
>>>> SELECT NULL::text, 1234::int, 'Foo bar'::text, E'Foo\nBar'::text
>>>> 2018-12-20 09:34:02,570: INFO werkzeug: 127.0.0.1 - - [20/Dec/2018
>>>> 09:34:02] "GET
>>>> /sqleditor/query_tool/download/5610522?query=SELECT%20NULL%3A%3Atext%2C%201234%3A%3Aint%2C%20%27Foo%20bar%27%3A%3Atext%2C%20E%27Foo%5CnBar%27%3A%3Atext&filename=data-1545298442530.csv
>>>> HTTP/1.1" 500 -
>>>> 2018-12-20 09:34:02,572: ERROR werkzeug: Error on request:
>>>> Traceback (most recent call last):
>>>> File
>>>> "/Users/dpage/.virtualenvs/pgadmin4/lib/python3.6/site-packages/werkzeug/serving.py",
>>>> line 270, in run_wsgi
>>>> execute(self.server.app)
>>>> File
>>>> "/Users/dpage/.virtualenvs/pgadmin4/lib/python3.6/site-packages/werkzeug/serving.py",
>>>> line 260, in execute
>>>> for data in application_iter:
>>>> File
>>>> "/Users/dpage/.virtualenvs/pgadmin4/lib/python3.6/site-packages/werkzeug/wsgi.py",
>>>> line 870, in __next__
>>>> return self._next()
>>>> File
>>>> "/Users/dpage/.virtualenvs/pgadmin4/lib/python3.6/site-packages/werkzeug/wrappers.py",
>>>> line 82, in _iter_encoded
>>>> for item in iterable:
>>>> File
>>>> "/Users/dpage/git/pgadmin4/web/pgadmin/utils/driver/psycopg2/connection.py",
>>>> line 820, in gen
>>>> csv_writer.writerows(results)
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/csv.py", line 748,
>>>> in writerows
>>>> return self.writer.writerows(map(self._dict_to_list, rowdicts))
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/csv.py", line 256,
>>>> in writerows
>>>> self.writerow(row)
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/csv.py", line 249,
>>>> in writerow
>>>> row = [self.strategy.prepare(field, only=only) for field in row]
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/csv.py", line 249,
>>>> in <listcomp>
>>>> row = [self.strategy.prepare(field, only=only) for field in row]
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/csv.py", line 136,
>>>> in prepare
>>>> raise Error('No escapechar is set')
>>>> _csv.Error: No escapechar is set
>>>>
>>>
>>> Not able to reproduce the above issue. I have tested it with the
>>> same setting as you mentioned. Please refer all the attached screenshots.
>>> Please specify the steps if they are different.
>>>
>>>>
>>>> When I have quoting set to All, the first column is returned as ""
>>>>
>>>> dpage(at)hal:*~/Downloads*$ more data-1545298598112.csv
>>>>
>>>> "text","int4","text-2","text-3"
>>>>
>>>> "","1234","Foo bar","Foo
>>>>
>>>> Bar"
>>>>
>>>> Isn't the point for it to be NULL?
>>>>
>>>
>>> while quoting is set to ALL, all the data types has been quoted, so
>>> I thought null values should be replaced by "" instead of blank. But if you
>>> think null values shouldn't be quoted even if user select quote ALL, I'll
>>> fix it and resend the patch.
>>>
>>
>> So how would you distinguish NULL from an empty string? Isn't that
>> exactly what the bug is about?
>>
>> I still think we need a "Replace NULLs with" config option, and
>> regardless of quoting settings we always replace NULL values with whatever
>> that is set to - for which the user could then choose options like:
>>
>> NULL
>> "NULL"
>> ""
>> ''
>> <empty string>
>>
>> We would never quote the NULL replacement value - if the user wanted it
>> to be quoted, they would include the quotes in the configured string.
>>
>
>
> OK, Will work on it and send the modified patch again.
>

Attached is the modified patch as per your suggestion.

>
>>
>>>
>>>> On Tue, Dec 18, 2018 at 11:13 AM Akshay Joshi <
>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Dave
>>>>>
>>>>> Attached is the modified patch to fix review comments.
>>>>>
>>>>> On Tue, Dec 18, 2018 at 3:00 PM Akshay Joshi <
>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Dec 18, 2018 at 2:49 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> On Tue, Dec 18, 2018 at 3:45 AM Akshay Joshi <
>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Hackers,
>>>>>>>>
>>>>>>>> Attached is the patch to fix RM #3780 pgAdmin4 lacks ability to
>>>>>>>> specify NULL values in CSV export.
>>>>>>>>
>>>>>>>> Please review it.
>>>>>>>>
>>>>>>>
>>>>>>> A few points;
>>>>>>>
>>>>>>> - You've included code from backports.csv, but per the licence you
>>>>>>> need to include a description of the changes made.
>>>>>>>
>>>>>>
>>>>>> Sure. In that case I'll copy the complete file and will do my
>>>>>> changes which is of two lines only. With my patch I have remove all the
>>>>>> unwanted code from backport.csv.
>>>>>>
>>>>>>>
>>>>>>> - Shouldn't backports.csv be removed from requirements.txt, or is it
>>>>>>> used elsewhere?
>>>>>>>
>>>>>>
>>>>>> Yes. Will do that.
>>>>>>
>>>>>>>
>>>>>>> - If the previous point is true, then I'm fairly sure there is code
>>>>>>> in one or more of the many package build scripts that adds an __init__.py
>>>>>>> file to backports.csv in the venv that's created.
>>>>>>>
>>>>>>
>>>>>> I'll remove that code as well.
>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Dave Page
>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>> Twitter: @pgsnake
>>>>>>>
>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> *Akshay Joshi*
>>>>>>
>>>>>> *Sr. Software Architect *
>>>>>>
>>>>>>
>>>>>>
>>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> *Akshay Joshi*
>>>>>
>>>>> *Sr. Software Architect *
>>>>>
>>>>>
>>>>>
>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>>>
>>>>
>>>>
>>>> --
>>>> Dave Page
>>>> Blog: http://pgsnake.blogspot.com
>>>> Twitter: @pgsnake
>>>>
>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>
>>>
>>> --
>>> *Akshay Joshi*
>>>
>>> *Sr. Software Architect *
>>>
>>>
>>>
>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
> --
> *Akshay Joshi*
>
> *Sr. Software Architect *
>
>
>
> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>

--
*Akshay Joshi*

*Sr. Software Architect *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*