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 1gZwLv-0001MP-Sv for pgadmin-hackers@arkaria.postgresql.org; Thu, 20 Dec 2018 11:18:56 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1gZwLt-0000Al-44 for pgadmin-hackers@arkaria.postgresql.org; Thu, 20 Dec 2018 11:18:53 +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 1gZwLs-0000Ae-TK for pgadmin-hackers@lists.postgresql.org; Thu, 20 Dec 2018 11:18:52 +0000 Received: from mail-wm1-x342.google.com ([2a00:1450:4864:20::342]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1gZwLl-0003Zg-Md for pgadmin-hackers@postgresql.org; Thu, 20 Dec 2018 11:18:52 +0000 Received: by mail-wm1-x342.google.com with SMTP id r24so11673978wmh.0 for ; Thu, 20 Dec 2018 03:18:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=23Mq3ydSoXOPCzynMhiaI0UW8K8xk0NgqDUwrn6eHAI=; b=1+rmGPlADdBYyanh2405Wwd9OiB3PVbnbF3p6Di6zjSduXGnHTonAr7LAyfzsAIjza t0PEKa8Nl5xJjeLCiby6N3/vgIZuxERmuo4eNh2tr5g4nDTTuNgRWBaMEi1w/FNCL6Te VNVBCTgTnuycemc1vYARXOEUke1ictTz8J5JixwgFLiu1SdUtZhzh5Q44aEK9U1BiB+a mDu3wJxgmH71YFNBiku7ZQUEdOLD8Ky3Q2pmSyzVZ4r4fkBMYpGAveRwgq2E8goYPjCU su8RmJxKpV4L9TQQ6BWSeJzBsswn1ZuCrRhCf5KSw4PdXSkfW1hN/yXLoifi0OXI59lV DIzg== 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=23Mq3ydSoXOPCzynMhiaI0UW8K8xk0NgqDUwrn6eHAI=; b=Bg1XWrQEVVh3+gfVD+IhGBdf1dFBn1oANbnqcSrLHGxjvfU+TRxj1o1O5sbPAkqRSg UYKlOmw1Zu3Z5RzeyL77MIfRRlWQW74HWvgZ+ZV4YXq978l5gEqK9i1ftySlxA5+M2AN 5PZ09YAC5bDIfBThvh97hMUozuruVZFGvEiXgWfijP3liVtViNncmU9rwdxtXvhp9LPu fOqkcm8vLVG11avnc945KkeiAAkOYtS3ca1D8VRgS+n958Yiz+gGwMNLzfo2VHZJy0Hi Tbxak0zw3dSXApG/HkZAYpUNMTRyJUpG1hWF19Vb5T1Aqs/RffaDQ0aLlU6Nz6QP2XBq Dxlw== X-Gm-Message-State: AA+aEWZnF3aCnm2Bqtmj+MyE5n3KNrFSAs3TMM1epAUvKUK9OSAhCjaR SguBXm88WcOVwvo2UQFemuPFWkrhgKLWLJjqlfi8ZA== X-Google-Smtp-Source: AFSGD/VEt5G9PwT/R+zPHd+vSQXj01Yg/INgUeSBJLDxd7z2lYiyPvGahHCgZtNifDx9Mn85PtCAO46dDl3ZDHqs1sU= X-Received: by 2002:a1c:a68f:: with SMTP id p137mr5784563wme.64.1545304721793; Thu, 20 Dec 2018 03:18:41 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Thu, 20 Dec 2018 11:18:30 +0000 Message-ID: Subject: Re: [pgAdmin4][Patch] - RM 3780 pgAdmin4 lacks ability to specify NULL values in CSV export To: Akshay Joshi Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000033be3057d724bc6" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000033be3057d724bc6 Content-Type: text/plain; charset="UTF-8" Hi On Thu, Dec 20, 2018 at 10:09 AM Akshay Joshi 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. > >> 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 --000000000000033be3057d724bc6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On T= hu, Dec 20, 2018 at 10:09 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
Hi Dave

On Th= u, Dec 20, 2018 at 3:08 PM Dave Page <dpage@pgadmin.org> wrote:
Hi<= div>
When testing with quoting set to None, quote =3D " = and delimiter =3D , 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: 41= 21147):
SELECT NULL::text, 1234::int, 'Foo bar'::text, E&= #39;Foo\nBar'::text
2018-12-20 09:34:02,570: INFO werkzeug: 127.0.0.1 - - [20/Dec/2018 09:34:02] "GET /sqleditor/quer= y_tool/download/5610522?query=3DSELECT%20NULL%3A%3Atext%2C%201234%3A%3Aint%= 2C%20%27Foo%20bar%27%3A%3Atext%2C%20E%27Foo%5CnBar%27%3A%3Atext&filenam= e=3Ddata-1545298442530.csv HTTP/1.1" 500 -
2018-12-20 09:34:= 02,572: ERROR werkzeug: Error on request:
Traceback = (most recent call last):
=C2=A0 File "/Users/dpage/.virtuale= nvs/pgadmin4/lib/python3.6/site-packages/werkzeug/serving.py", line 27= 0, in run_wsgi
=C2=A0 =C2=A0 execute(self.server.app)
= =C2=A0 File "/Users/dpage/.virtualenvs/pgadmin4/lib/python3.6/site-pac= kages/werkzeug/serving.py", line 260, in execute
=C2=A0 =C2= =A0 for data in application_iter:
=C2=A0 File "/Users/dpage/= .virtualenvs/pgadmin4/lib/python3.6/site-packages/werkzeug/wsgi.py", l= ine 870, in __next__
=C2=A0 =C2=A0 return self._next()
= =C2=A0 File "/Users/dpage/.virtualenvs/pgadmin4/lib/python3.6/site-pac= kages/werkzeug/wrappers.py", line 82, in _iter_encoded
=C2= =A0 =C2=A0 for item in iterable:
=C2=A0 File "/Users/dpage/g= it/pgadmin4/web/pgadmin/utils/driver/psycopg2/connection.py", line 820= , in gen
=C2=A0 =C2=A0 csv_writer.writerows(results)
= =C2=A0 File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/csv.py",= line 748, in writerows
=C2=A0 =C2=A0 return self.writer.writerow= s(map(self._dict_to_list, rowdicts))
=C2=A0 File "/Users/dpa= ge/git/pgadmin4/web/pgadmin/utils/csv.py", line 256, in writerows
=C2=A0 =C2=A0 self.writerow(row)
=C2=A0 File "/Users/d= page/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]
=C2=A0 File "/Users/dpage/git/pgadmin4/web/pg= admin/utils/csv.py", line 249, in <listcomp>
=C2=A0 = =C2=A0 row =3D [self.strategy.prepare(field, only=3Donly) for field in row]=
=C2=A0 File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/cs= v.py", line 136, in prepare
=C2=A0 =C2=A0 raise Error('N= o escapechar is set')
_csv.Error: No escapechar is set
<= /div>

=C2=A0 =C2=A0 Not able to= reproduce the above issue. I have tested it with the same setting as you m= entioned. Please refer all the attached screenshots. Please specify the ste= ps if they are different.

When I hav= e quoting set to All, the first column is returned as ""

dpage<= span class=3D"gmail-m_361423959471310265gmail-m_-1300974772564471919gmail-s= 2" style=3D"font-variant-ligatures:no-common-ligatures">@hal:~/Downloads$ more data-1545298598112.csv=C2=A0<= /span>

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

"","1234","= ;Foo bar","Foo

Bar"


Isn't the point for i= t to be NULL?

=C2=A0 =C2=A0 while quoting is set to ALL, all the data types has been quo= ted, 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 s= elect quote ALL, I'll fix it and resend the patch.

So how would you distinguish NULL from an emp= ty 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 wha= tever that is set to=C2=A0 - for which the user could then choose options l= ike:

NULL
"NULL"
&qu= ot;"
''
<empty string>
We would never quote the NULL replacement value - if the user w= anted it to be quoted, they would include the quotes in the configured stri= ng.
=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 com= ments.

On Tue, D= ec 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 A= kshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi H= ackers,

Attached is the patch to fix RM #3780=C2=A0pgAdm= in4 lacks ability to specify NULL values in CSV export.

Please review it.

A few points;

<= div>- 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 that case I'll = copy the complete file and will do my changes which is of two lines only. W= ith my patch I have remove all the unwanted code from backport.csv.=C2=A0 = =C2=A0 =C2=A0=C2=A0
=

- Shouldn&#= 39;t backports.csv be removed from requirements.txt, or is it used elsewher= e?

=C2=A0 =C2=A0 =C2=A0Ye= s. Will do that.=C2=A0

- If the= previous point is true, then I'm fairly sure there is code in one or m= ore of the many package build scripts that adds an __init__.py file to back= ports.csv in the venv that's created.

=C2=A0 =C2=A0 I'll remove that code as well.=C2=A0=C2= =A0
=C2=A0
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

E= nterpriseDB UK: h= ttp://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Akshay Joshi
Sr. Software Architect

<= /b>
=

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


--
Akshay Joshi=
Sr. Software Architect


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


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

Enterpr= iseDB UK: http://= www.enterprisedb.com
The Enterprise PostgreSQL Company


--
=
Akshay Joshi
Sr. Software Architect

=
Phone: +91 20-3058-9517
Mobil= e: +91 976-788-8246


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @p= gsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL = Company
--000000000000033be3057d724bc6--