Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1gZwjD-0002ag-Gg for pgadmin-hackers@arkaria.postgresql.org; Thu, 20 Dec 2018 11:42:59 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1gZwjC-0001bK-6W for pgadmin-hackers@arkaria.postgresql.org; Thu, 20 Dec 2018 11:42:58 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1gZwjB-0001Zr-Vl for pgadmin-hackers@lists.postgresql.org; Thu, 20 Dec 2018 11:42:58 +0000 Received: from mail-qk1-x72a.google.com ([2607:f8b0:4864:20::72a]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1gZwj8-000483-4U for pgadmin-hackers@postgresql.org; Thu, 20 Dec 2018 11:42:57 +0000 Received: by mail-qk1-x72a.google.com with SMTP id 189so746105qkj.8 for ; Thu, 20 Dec 2018 03:42:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=hJ8LlkXMUPC7Gb7VKsNsWZeXuiOgP33dO6zGWvKFM1k=; b=m0b5lqc61R9Uon5QUQrZ5zAr3QilU9+gOIeKpXwYVXh1NoN0/EzIFUxboblK9qmV3X a+0rC7zZtAEdnlaZddo85Up5XzcDtRKZRLvtsB+mAfIxfJJhnVKXaZp3aFZ3PJUBK7mq JlSZIesbVyu4WF+UH5Bt8n8y/Q4Ew9YIZxH0zzSySaIt9JVp9eMu8SPpuwu7jMjzu+Ua ostDf8zi3cpkjJY4I6jxG8KZXvlCKDbrOJdsPT8fuYsr9YwOa0uXdAb9JO4QUCEGEmUF 2HEzATyY5NmBuQzeI1VuKOtMqP2JXXSD3VtGP4Hrkv/mp/EJBW1fVKKXnjX46naollZ9 j+4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=hJ8LlkXMUPC7Gb7VKsNsWZeXuiOgP33dO6zGWvKFM1k=; b=D9oQhf0UJoPV6yqrcb/M/eQG31tXqpLwtuU8h3SMjNdIWBZlwMKL0PzIfqmRSKPFaZ jhs3+dgnMBsRzgTNxe75YB4hgheLTaLyOl+HJFIBj/tHvqAgRVM+prh5JoH0S9pZfpLj h44SuDlF9772MoybUH+AYQXle9V1TNkUBGhNHmpNW1a7We96PJjrQI2i9h27Z1TPy3cO l5l/g6RVp09lJ+TAQBRcT6j2uk99e/YNvO8MjXtqY+14dAnAHukqo6upMV6boRIY7FQJ 58yRsP36XorlVX5i45TGFwLM9aDNRqbpZazqkxDA04sAI8LroBAcacRtWSvpB8/Yz8z6 H1ug== X-Gm-Message-State: AA+aEWYTqT9HZEmjf9/caoS70to/92BysrT+sanEXyPNMOIj5eEI0tGx clndDoCmCrPcRLjKgvF26BeyFyOR5QwRAShxKbCspA== X-Google-Smtp-Source: AFSGD/Uu/sI+KryOI4TA5Csh2IWk4KbZ/h31FkTXRMytWQJx4hRB+1rpdG5dcDXfbHeaxbvZYDzV65ZNTeLJkh6s3vc= X-Received: by 2002:a37:90c3:: with SMTP id s186mr23888900qkd.339.1545306171692; Thu, 20 Dec 2018 03:42:51 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Thu, 20 Dec 2018 17:12:40 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch] - RM 3780 pgAdmin4 lacks ability to specify NULL values in CSV export To: Dave Page Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000006ef390057d72a108" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000006ef390057d72a108 Content-Type: text/plain; charset="UTF-8" On Thu, Dec 20, 2018 at 4:48 PM Dave Page wrote: > Hi > > On Thu, Dec 20, 2018 at 10:09 AM Akshay Joshi < > akshay.joshi@enterprisedb.com> wrote: > >> Hi Dave >> >> On Thu, Dec 20, 2018 at 3:08 PM Dave Page 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 >>> 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@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" > "" > '' > > > 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. > > >> >>> On Tue, Dec 18, 2018 at 11:13 AM Akshay Joshi < >>> akshay.joshi@enterprisedb.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.joshi@enterprisedb.com> wrote: >>>> >>>>> >>>>> >>>>> On Tue, Dec 18, 2018 at 2:49 PM Dave Page wrote: >>>>> >>>>>> Hi >>>>>> >>>>>> On Tue, Dec 18, 2018 at 3:45 AM Akshay Joshi < >>>>>> akshay.joshi@enterprisedb.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* --0000000000006ef390057d72a108 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Dec 20, 2018 at 4:48 PM Dave Page <dpage@pgadmin.org> wrote:
Hi

On Thu, Dec 20, 2018 at 10:09 AM Aksh= ay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi Dave=

On Thu, Dec 20, 2018 = at 3:08 PM Dave Page <dpage@pgadmin.org> wrote:
Hi

When testing with quoting set to None, quote =3D " and delimiter = =3D , I get the following exception when I try to download:

<= /div>
2018-12-20 09:34:02,547: SQL pgadmin: Execute (= with server cursor) for server #2 - CONN:354106 (Query-id: 4121147):
<= div>SELECT NULL::text, 1234::int, 'Foo bar'::text, E'Foo\nBar&#= 39;::text
2018-12-20 09:34:02,570: INFO werkzeug: 12= 7.0.0.1 - - [20/Dec/2018 09:34:02] "GET /sqleditor/query_tool/download= /5610522?query=3DSELECT%20NULL%3A%3Atext%2C%201234%3A%3Aint%2C%20%27Foo%20b= ar%27%3A%3Atext%2C%20E%27Foo%5CnBar%27%3A%3Atext&filename=3Ddata-154529= 8442530.csv HTTP/1.1" 500 -
2018-12-20 09:34:02,572: ERROR werkzeug: Error on request:
Traceback (most recent ca= ll last):
=C2=A0 File "/Users/dpage/.virtualenvs/pgadmin4/li= b/python3.6/site-packages/werkzeug/serving.py", line 270, in run_wsgi<= /div>
=C2=A0 =C2=A0 execute(self.server.app)
=C2=A0 File &quo= t;/Users/dpage/.virtualenvs/pgadmin4/lib/python3.6/site-packages/werkzeug/s= erving.py", line 260, in execute
=C2=A0 =C2=A0 for data in a= pplication_iter:
=C2=A0 File "/Users/dpage/.virtualenvs/pgad= min4/lib/python3.6/site-packages/werkzeug/wsgi.py", line 870, in __nex= t__
=C2=A0 =C2=A0 return self._next()
=C2=A0 File "= ;/Users/dpage/.virtualenvs/pgadmin4/lib/python3.6/site-packages/werkzeug/wr= appers.py", line 82, in _iter_encoded
=C2=A0 =C2=A0 for item= in iterable:
=C2=A0 File "/Users/dpage/git/pgadmin4/web/pga= dmin/utils/driver/psycopg2/connection.py", line 820, in gen
= =C2=A0 =C2=A0 csv_writer.writerows(results)
=C2=A0 File "/Us= ers/dpage/git/pgadmin4/web/pgadmin/utils/csv.py", line 748, in writero= ws
=C2=A0 =C2=A0 return self.writer.writerows(map(self._dict_to_l= ist, rowdicts))
=C2=A0 File "/Users/dpage/git/pgadmin4/web/p= gadmin/utils/csv.py", line 256, in writerows
=C2=A0 =C2=A0 s= elf.writerow(row)
=C2=A0 File "/Users/dpage/git/pgadmin4/web= /pgadmin/utils/csv.py", line 249, in writerow
=C2=A0 =C2=A0 = row =3D [self.strategy.prepare(field, only=3Donly) for field in row]
<= div>=C2=A0 File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/csv.py&qu= ot;, line 249, in <listcomp>
=C2=A0 =C2=A0 row =3D [self.st= rategy.prepare(field, only=3Donly) for field in row]
=C2=A0 File = "/Users/dpage/git/pgadmin4/web/pgadmin/utils/csv.py", line 136, i= n prepare
=C2=A0 =C2=A0 raise Error('No escapechar is set'= ;)
_csv.Error: No escapechar is set

=C2=A0 =C2=A0 Not able to reproduce the above is= sue. I have tested it with the same setting as you mentioned. Please refer = all the attached screenshots. Please specify the steps if they are differen= t.
=

When I have quoting set to All, t= he first column is returned as ""

dpage@hal:= ~/Downloads$ more data-1545298598112.csv=C2=A0

"text","int4","text-2",&= quot;text-3"

"","1234","Foo bar",&quo= t;Foo

Bar"

<= div dir=3D"ltr">
Isn't the point for it to be NULL?
=

=C2=A0 =C2=A0 whil= e quoting is set to ALL, all the data types has been quoted, so I thought n= ull 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&= #39;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 thin= k we need a "Replace NULLs with" config option, and regardless of= quoting settings we always replace NULL values with whatever that is set t= o=C2=A0 - for which the user could then choose options like:

=
NULL
"NULL"
""
''
<empty string>

We woul= d never quote the NULL replacement value - if the user wanted it to be quot= ed, they would include the quotes in the configured string.


=C2=A0 =C2=A0OK, Will wor= k on it and send the modified patch again.=C2=A0
=C2=A0

On Tue, Dec 18, 2018 at 11:13= AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi=C2=A0Dave
Attached is the modified patch to fix review comments.

On Tue, Dec 18, 2018= at 3:00 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
=


On Tue, = Dec 18, 2018 at 2:49 PM Dave Page <dpage@pgadmin.org> wrote:
Hi

On Tue, Dec 18, 2018 at 3:45 AM Akshay Joshi= <aks= hay.joshi@enterprisedb.com> wrote:
Hi Hackers,
Attached is the patch to fix RM #3780=C2=A0pgAdmin4 lacks a= bility to specify NULL values in CSV export.

Please revi= ew 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.

=C2=A0 =C2=A0 =C2=A0 Sure. In th= at 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 back= port.csv.=C2=A0 =C2=A0 =C2=A0=C2=A0

=
- Shouldn't backports.csv be removed from requirements.txt, or is = it used elsewhere?

=C2=A0= =C2=A0 =C2=A0Yes. Will do that.=C2=A0

- 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.
=

=C2=A0 =C2=A0 I'll remove that code as= well.=C2=A0=C2=A0
<= div dir=3D"ltr">
=C2=A0
--
Dave Page
Blog: <= a href=3D"http://pgsnake.blogspot.com" target=3D"_blank">http://pgsnake.blo= gspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.comThe Enterprise PostgreSQL Company


--
Akshay Joshi
Sr. Software Archit= ect

=


--
Akshay Joshi
<= b>Sr. Software Architect
<= /div>

=

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


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twit= ter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Post= greSQL Company


--
<= b>Akshay Joshi
Sr. Software Architect

<= div>
Phone= : +91 20-3058-9517
Mobile: +91 976-788-8246


--
Dave Page
Blog: <= a href=3D"http://pgsnake.blogspot.com" target=3D"_blank">http://pgsnake.blo= gspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.comThe Enterprise PostgreSQL Company


--
Akshay Joshi
Sr. S= oftware Architect
=

Phone: +91 20-3058-9517
Mobile: +91 976-788-8246=
--0000000000006ef390057d72a108--