Обсуждение: PATCH: Encoding issue with qtLiteral function (pgAdmin4)

Hi

On Fri, Jul 22, 2016 at 3:11 PM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> Hi,
>
> PFA patch to fix the issue when we use qtLiteral function and user enters
> special characters(like emojis) which it fails to handle.
> RM#1307

It still seems to be broken to me. For example, if I try to set a
comment on a table to 😬, I get the following error:

2016-07-22 16:47:49,526: INFO werkzeug: 127.0.0.1 - - [22/Jul/2016
16:47:49] "GET
/browser/table/msql/1/1/17251/2200/17267?id=17267&description=%F0%9F%98%AC+&columns=%7B%22changed%22%3A%5B%7B%22attlen%22%3Anull%7D%2C%7B%22attlen%22%3Anull%7D%2C%7B%22attlen%22%3Anull%7D%2C%7B%22attlen%22%3Anull%7D%2C%7B%22attlen%22%3Anull%7D%5D%7D&_=1469202363116
HTTP/1.1" 500 -
Traceback (most recent call last):
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 2000, in __call__
    return self.wsgi_app(environ, start_response)
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1991, in wsgi_app
    response = self.make_response(self.handle_exception(e))
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1567, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1988, in wsgi_app
    response = self.full_dispatch_request()
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1641, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1544, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1639, in full_dispatch_request
    rv = self.dispatch_request()
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/app.py",
line 1625, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-packages/flask/views.py",
line 84, in view
    return self.dispatch_request(*args, **kwargs)
  File "/Users/dpage/git/pgadmin4/web/pgadmin/browser/utils.py", line
235, in dispatch_request
    return method(*args, **kwargs)
  File "/Users/dpage/git/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py",
line 276, in wrap
    self.conn = self.manager.connection(did=kwargs['did'])
  File "/Users/dpage/git/pgadmin4/web/pgadmin/utils/driver/psycopg2/__init__.py",
line 1278, in connection
    raise Exception(msg_active_conn)
Exception: Server has no active connection. Please connect to the server.


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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi

On Tue, Jul 26, 2016 at 2:34 PM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> Hi Dave,
>
> PFA updated patch to fix encoding issue, I have tested in Python2/3 and
> looks like working in both environment.
>
> Almost all nodes are affected with this change, I tried to test & cover all
> basic operations on nodes but still it requires some testing.
>
> This patch will cover below tickets,
> RM#1307
> RM#1479

I've committed this as-is, as it fixes things nicely as far as I can
see... with one exception: I couldn't create Functions with utf-8
names. Can you fix that please?

Thanks.

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jul 26, 2016 at 3:06 PM, Dave Page <dpage@pgadmin.org> wrote:
> Hi
>
> On Tue, Jul 26, 2016 at 2:34 PM, Murtuza Zabuawala
> <murtuza.zabuawala@enterprisedb.com> wrote:
>> Hi Dave,
>>
>> PFA updated patch to fix encoding issue, I have tested in Python2/3 and
>> looks like working in both environment.
>>
>> Almost all nodes are affected with this change, I tried to test & cover all
>> basic operations on nodes but still it requires some testing.
>>
>> This patch will cover below tickets,
>> RM#1307
>> RM#1479
>
> I've committed this as-is, as it fixes things nicely as far as I can
> see... with one exception: I couldn't create Functions with utf-8
> names. Can you fix that please?

I spoke too soon - Types don't play nicely either - try creating the
following in the UI.

CREATE TYPE public."🍺" AS ENUM
    ('a', 'b');

ALTER TYPE public."🍺"
    OWNER TO postgres;


Isn't there a way we can force everything to UTF-8 without having to
modify individual nodes? That's what we did in pgAdmin 3, then never
worried about the problem again.

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi

On Wed, Jul 27, 2016 at 12:18 PM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> Hi Dave,
>
> 1) Unicode conversion was working properly for function node, It was failing
> due to wrong sql templates.
> - I've fixed them.

I just did a test, and it's still failing - see the attached
screenshot. Aside from the encoding issue, it looks like it's adding
extra quotes around the parameter data type - double-double quoting
the schema and type, and then quoting the whole lot again in the
CREATE part. The ALTER part looks fine.

> 2) Type is also working fine with encoding for me(PFA screenshots), Can you
> please provide error you got?

Hmm, I can't reproduce that now. Weird.

> This patch also makes Provider optional in security label control &
> templates.
>
> -------
> Regarding enforcing to utf-8, In my opinion we can enforce everything to
> utf-8 if we use any of one python version but to make it compatible with
> both python2 and python3 version by forcing utf-8 encoding it will be
> difficult to handle its side effects.

Urgh. Ok, thanks.

> Python2,
>
>>>> txt_sample = 'Hello'
>>>> isinstance(txt_sample, str)
> True
>>>> isinstance(txt_sample, bytes)
> True
>>>> isinstance(txt_sample, unicode)
> False
>>>> txt_sample.encode('utf-8')
> 'Hello'
>>>> txt_sample.decode('utf-8')
> u'Hello'
>
> Forcing to unicode in Python2,
>
>>>> txt_sample = u'Hello'
>>>> isinstance(txt_sample, str)
> False
>>>> isinstance(txt_sample, bytes)
> False
>>>> isinstance(txt_sample, unicode)
> True
>
> Python3,
>
>>>> txt_sample = 'Hello'
>>>> isinstance(txt_sample, str)
> True
>>>> isinstance(txt_sample, bytes)
> False
>>>> txt_sample.encode('utf-8')
> b'Hello'
>>>> txt_sample.decode('utf-8')
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> AttributeError: 'str' object has no attribute 'decode'
>
>
> And also psycopg2 driver returns text as string in Python2 even if that text
> contains unicode characters, In python3 that just works fine because string
> is Unicode by default.
>
>
> Regards,
> Murtuza
>
> --
> Regards,
> Murtuza Zabuawala
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company



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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thanks - applied!

On Wed, Jul 27, 2016 at 1:37 PM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> Okay, Let me check on that.
>
> Meanwhile I'm attaching patch for making Provider optional in security
> labels control.
>
> Regards,
> Murtuza
>
> --
> Regards,
> Murtuza Zabuawala
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> On Wed, Jul 27, 2016 at 5:04 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Hi
>>
>> On Wed, Jul 27, 2016 at 12:18 PM, Murtuza Zabuawala
>> <murtuza.zabuawala@enterprisedb.com> wrote:
>> > Hi Dave,
>> >
>> > 1) Unicode conversion was working properly for function node, It was
>> > failing
>> > due to wrong sql templates.
>> > - I've fixed them.
>>
>> I just did a test, and it's still failing - see the attached
>> screenshot. Aside from the encoding issue, it looks like it's adding
>> extra quotes around the parameter data type - double-double quoting
>> the schema and type, and then quoting the whole lot again in the
>> CREATE part. The ALTER part looks fine.
>>
>> > 2) Type is also working fine with encoding for me(PFA screenshots), Can
>> > you
>> > please provide error you got?
>>
>> Hmm, I can't reproduce that now. Weird.
>>
>> > This patch also makes Provider optional in security label control &
>> > templates.
>> >
>> > -------
>> > Regarding enforcing to utf-8, In my opinion we can enforce everything to
>> > utf-8 if we use any of one python version but to make it compatible with
>> > both python2 and python3 version by forcing utf-8 encoding it will be
>> > difficult to handle its side effects.
>>
>> Urgh. Ok, thanks.
>>
>> > Python2,
>> >
>> >>>> txt_sample = 'Hello'
>> >>>> isinstance(txt_sample, str)
>> > True
>> >>>> isinstance(txt_sample, bytes)
>> > True
>> >>>> isinstance(txt_sample, unicode)
>> > False
>> >>>> txt_sample.encode('utf-8')
>> > 'Hello'
>> >>>> txt_sample.decode('utf-8')
>> > u'Hello'
>> >
>> > Forcing to unicode in Python2,
>> >
>> >>>> txt_sample = u'Hello'
>> >>>> isinstance(txt_sample, str)
>> > False
>> >>>> isinstance(txt_sample, bytes)
>> > False
>> >>>> isinstance(txt_sample, unicode)
>> > True
>> >
>> > Python3,
>> >
>> >>>> txt_sample = 'Hello'
>> >>>> isinstance(txt_sample, str)
>> > True
>> >>>> isinstance(txt_sample, bytes)
>> > False
>> >>>> txt_sample.encode('utf-8')
>> > b'Hello'
>> >>>> txt_sample.decode('utf-8')
>> > Traceback (most recent call last):
>> >   File "<stdin>", line 1, in <module>
>> > AttributeError: 'str' object has no attribute 'decode'
>> >
>> >
>> > And also psycopg2 driver returns text as string in Python2 even if that
>> > text
>> > contains unicode characters, In python3 that just works fine because
>> > string
>> > is Unicode by default.
>> >
>> >
>> > Regards,
>> > Murtuza
>> >
>> > --
>> > Regards,
>> > Murtuza Zabuawala
>> > EnterpriseDB: http://www.enterprisedb.com
>> > The Enterprise PostgreSQL Company
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi

On Thu, Jul 28, 2016 at 10:56 AM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> Hi Dave,
>
> PFA updated patch to fix issue in functions which was due to improper
> handing of nested quoting of types in templates.
> I also fixed it in Table/Column node.
>
> Please review.

This doesn't seem right to me:

                # If schema is not pg_catalog & then add schema
                if row['nspname'] != 'pg_catalog' \
                        and row['nspname'] == 'public':
                    rtn_type = row['nspname'] + "." + row['typname']
                else:
                    rtn_type = row['typname']

Shouldn't the "and row['nspname'] == 'public'" be removed?

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Aug 1, 2016 at 2:33 PM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> Hi Dave,
>
> We have to added it because postgres format_type() function do not provide
> us type with 'public' schema prefix to it, due to which wrong SQL's are
> getting generated.
> Rest other types are pre fixed with their respective schema as required.

OK, so:

                # If schema is not pg_catalog & then add schema
                if row['nspname'] == 'public':
                    rtn_type = row['nspname'] + "." + row['typname']
                else:
                    rtn_type = row['typname']

?

!= pg_catalog is redundant with == public.

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Aug 1, 2016 at 2:53 PM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> Yes... :-)

OK, committed with the following:

                # If schema is public, prefix it. Otherwise, we should
already have it.
                if row['nspname'] == 'public':
                    rtn_type = "public." + row['typname']
                else:
                    rtn_type = row['typname']

> On Mon, Aug 1, 2016 at 7:16 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> On Mon, Aug 1, 2016 at 2:33 PM, Murtuza Zabuawala
>> <murtuza.zabuawala@enterprisedb.com> wrote:
>> > Hi Dave,
>> >
>> > We have to added it because postgres format_type() function do not
>> > provide
>> > us type with 'public' schema prefix to it, due to which wrong SQL's are
>> > getting generated.
>> > Rest other types are pre fixed with their respective schema as required.
>>
>> OK, so:
>>
>>                 # If schema is not pg_catalog & then add schema
>>                 if row['nspname'] == 'public':
>>                     rtn_type = row['nspname'] + "." + row['typname']
>>                 else:
>>                     rtn_type = row['typname']
>>
>> ?
>>
>> != pg_catalog is redundant with == public.
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>



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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company