Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1aQv3A-0006Qk-0j for pgadmin-hackers@arkaria.postgresql.org; Wed, 03 Feb 2016 10:52:40 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84) (envelope-from ) id 1aQv39-000321-C1 for pgadmin-hackers@arkaria.postgresql.org; Wed, 03 Feb 2016 10:52:39 +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_SHA384:256) (Exim 4.84) (envelope-from ) id 1aQv38-00031f-Ks for pgadmin-hackers@postgresql.org; Wed, 03 Feb 2016 10:52:38 +0000 Received: from mail-yk0-x233.google.com ([2607:f8b0:4002:c07::233]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84) (envelope-from ) id 1aQv34-0002HS-Ii for pgadmin-hackers@postgresql.org; Wed, 03 Feb 2016 10:52:38 +0000 Received: by mail-yk0-x233.google.com with SMTP id u9so14487001ykd.1 for ; Wed, 03 Feb 2016 02:52:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=bUD4iVy/r6P6FHgsRnl3xJtKR+EkrOzrF943dDpDS6E=; b=BsWEOFV7FqQc3Vu/SqthHB93E2pAgiBaNa43SSsprJCuu231FOsGBdkLlT5xM35V9Y NIruONLk4spv96Wat1mD4m1ej1FbTNY3V2kZdXh0CGt6HNT1Q0etYBHLVzh9YQB+Sp7n hVD7tGArw5ZucvYH8G266Kd/u/wH1SA7ZZA7NuWO9gcTm7Ca0uojwtcB5aAbKjyjotUx AsCk6Px0H4b1cIutA/lxF8x4sLnR11NYC0GbdQTPAB2Ar8xlelxDt3EqQl+cNWIPnjSN QOPWpa215GuhtUZmw91Ztr15jX8yJp46aajzq6ynNgB9v+OOAH24JTOT307nBE7OPITP T5Lg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=bUD4iVy/r6P6FHgsRnl3xJtKR+EkrOzrF943dDpDS6E=; b=ZducHclibTAglpAui/uvPrb19+e5DjXEGhd4vCeiCFGTVxENXrrGRF55i+7bv3OXhY pGJSdScXj3h7g/YCnnDB4nFqalGKVJetgk47aIm1NsyE4MADhrj1IIzBuTgPOZKifwUV LjU6CoVGGEImdi914wwF3Bjo9pDwUSEViXh0WfpR8oagbb8ZY0DcPo1cJ7IeLHdWqSBS xTFZ/IX7jFRwQpr0UghEwB4ln+p1sFVNMVUxYAXDfTZpemKyzYo7YqDysLSzs49G5UEm 6mI8zPtk1T+rPyUbbvK6YvsF/rGMUJ4THRewAvGrVCIgYq0wDEwCoBZAQUizkmGUVck+ FlwQ== X-Gm-Message-State: AG10YOR1DskXMn9fG9dnguqWB4IWOtdLNqvK/zhTJW5TYGzQVxxz8KvqeSET6QFPhhriybVmKC1eKefWkt1bz4qa MIME-Version: 1.0 X-Received: by 10.37.84.212 with SMTP id i203mr389993ybb.119.1454496752797; Wed, 03 Feb 2016 02:52:32 -0800 (PST) Received: by 10.37.202.75 with HTTP; Wed, 3 Feb 2016 02:52:32 -0800 (PST) In-Reply-To: References: Date: Wed, 3 Feb 2016 16:22:32 +0530 Message-ID: Subject: Re: pgAdmin4 PATCH: Domain Module From: Neel Patel To: Khushboo Vashi Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary=001a11423afc47418f052adb6a33 X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --001a11423afc47418f052adb6a33 Content-Type: text/plain; charset=UTF-8 Hi Khushboo, Please find below review comments. - Reverse engineering SQL generation is not implemented for domain node. - "Length" and "Precision" fields should be enabled/disabled based on the selection of "Base Type" value. - Query is not getting generated properly. Some of the parameters are not reflected in query. As we have provided Length and Precision value in below numeric base type. Also do proper indentation in generated query. * Wrong Query :- * CREATE DOMAIN my_schema.test_123 AS "numeric" DEFAULT 5 ; * Correct Query :- * CREATE DOMAIN my_schema.test_123 AS numeric(22,4) DEFAULT 5 NOT NULL; - After creation of new domain with base type "aclitem" , wrong "Length" field value is getting displayed. - We are getting error saying "*TypeError: the JSON object must be str, not 'dict*'" when we add constraint with "NOT VALID" and NO INHERIT. - We should add property "System Domain?" when we select any domain node. - We think for creation of Security Label, we should include the schema name along with domain name. *Wrong Query :- * SECURITY LABEL FOR pv_label ON DOMAIN test_123 IS 'label_val'; Correct Query :- SECURITY LABEL FOR pv_label ON DOMAIN .test_123 IS 'label_val'; Let us know in case of any issues. Thanks, Neel Patel On Tue, Feb 2, 2016 at 3:51 PM, Khushboo Vashi < khushboo.vashi@enterprisedb.com> wrote: > Hi Neel, > > Thanks for reviewing my patch. > > I have modified the code as per your suggestions and also fixed some of > the issues got while doing unit testing. > Please find attached patch for the same. > > > Thanks, > Khushboo > > > > On Wed, Jan 20, 2016 at 10:56 PM, Neel Patel > wrote: > >> Hi Khushboo, >> >> Please find below review comments. >> >> - While creating new Domain and clicking on SQL tab, python side we are >> getting error saying "*TypeError: 'bool' object is not callable".* >> We are not able to create any domain. Fix this issue so that we can >> test other functionality. >> - Implement the reverse engineering SQL generation for the domain node. >> - As per the checklist, remove the "Use Slony" from Constraints tab, as >> it is not required. >> - No need to pass "*qtIdent=self.qtIdent*" as function argument in >> "create" and "getSQL" function in domains/__init__.py >> - In "Security" tab , provider and security label fields are not editable. >> - In PG version 9.1, when we update the existing domain name then "ALTER >> DOMAIN" is not supported. >> Currently there is no checking for the PG version 9.1 and 9.2_plus. It >> will fail when we connect to database 9.1 >> >> e.g. >> For PG version 9.1 - Update command should be as below. >> ALTER TYPE xyz RENAME TO abc; >> For PG version 9.2 onwards - Update command should be as below. >> ALTER DOMAIN xyz RENAME TO abc; >> >> - Some of the SQL file, qtIdent is not used. Please check all the related >> SQL files. >> e.g. - In update.sql file "data.owner" should be >> "conn|qtIdent(data.owner)" >> >> {% if data.owner %} >> ALTER DOMAIN {{ conn|qtIdent(o_data.basensp, name) }} >> OWNER TO {{ data.owner }}; >> {% endif %} >> >> Let us know for any issues. >> >> Thanks, >> Neel Patel >> >> On Wed, Jan 20, 2016 at 2:50 PM, Khushboo Vashi < >> khushboo.vashi@enterprisedb.com> wrote: >> >>> Hi Neel, >>> >>> Please find updated patch. >>> >>> Thanks, >>> Khushboo >>> >>> On Wed, Jan 20, 2016 at 12:50 PM, Neel Patel < >>> neel.patel@enterprisedb.com> wrote: >>> >>>> Hi Khushboo, >>>> >>>> While applying the patch file, we are getting below warnings. >>>> >>>> ######################################### >>>> domains (1).patch:1340: trailing whitespace. >>>> oid: undefined, >>>> domains (1).patch:1483: trailing whitespace. >>>> (nspname = 'pg_catalog' AND EXISTS >>>> domains (1).patch:1487: trailing whitespace. >>>> OR (nspname = 'information_schema' AND EXISTS >>>> domains (1).patch:1489: trailing whitespace. >>>> OR (nspname LIKE '_%' AND EXISTS >>>> domains (1).patch:1642: trailing whitespace. >>>> (select 1 from pg_class where relnamespace=typnamespace and relname = >>>> typname and relkind != 'c') AND (typname not like '_%' OR NOT EXISTS >>>> (select 1 from pg_class where relnamespace=typnamespace and relname = >>>> substring(typname from 2)::name and relkind != 'c')) >>>> warning: squelched 4 whitespace errors >>>> warning: 9 lines add whitespace errors. >>>> ######################################### >>>> >>>> Can you please remove the whitespace and regenerate the patch ? >>>> >>>> Thanks, >>>> Neel Patel >>>> >>>> On Wed, Jan 20, 2016 at 12:37 PM, Khushboo Vashi < >>>> khushboo.vashi@enterprisedb.com> wrote: >>>> >>>>> Resending patch with binary option. >>>>> >>>>> On Wed, Jan 20, 2016 at 10:18 AM, Khushboo Vashi < >>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> Please find attached patch for the Domain Module. >>>>>> >>>>>> The patch will be modified after Types module implementation as we >>>>>> need to populate Base Type and some Type related validations from the >>>>>> Types module. >>>>>> >>>>>> Please review it and let me know the feedback. >>>>>> >>>>>> Thanks, >>>>>> Khushboo >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) >>>>> To make changes to your subscription: >>>>> http://www.postgresql.org/mailpref/pgadmin-hackers >>>>> >>>>> >>>> >>> >> > --001a11423afc47418f052adb6a33 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Khushboo,

Please find below review c= omments.

=C2=A0- Reverse engineering SQL generation is not implemented for domain node.
=C2=A0- "Length" and "Precision" fields sh= ould be enabled/disabled based on the selection of "Base Type" va= lue.
=C2=A0- Query is not getting generated properly. Some of the= parameters are not reflected in query.=C2=A0 As we have provided Length an= d Precision value in=C2=A0
=C2=A0 =C2=A0below numeric base type. = Also do proper indentation in generated query.

= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Wrong Query :-=C2=A0
= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 CREATE DOMAIN my_schema.test_123
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 AS "numeric"
=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 DEFAULT 5
=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 ;

=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0Correct Query :-=C2=A0
=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0CREATE DOMAIN my_schema.test_123
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0AS numeric(22,4)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0DEFAULT 5
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0NOT NULL;

=C2=A0- After creation of new domain with base type = "aclitem" , wrong "Length" field value is getting displ= ayed.
=C2=A0- We are getting error saying "TypeError: the= JSON object must be str, not 'dict'" when we add constrai= nt with "NOT VALID" and NO INHERIT.
=C2=A0- We should a= dd property "System Domain?" when we select any domain node.
=C2=A0- We think for creation of Security Label, we should include th= e schema name along with domain name.
=C2=A0 =C2=A0 Wrong Quer= y :-=C2=A0
=C2=A0 =C2=A0 =C2=A0 =C2=A0SECURITY LABEL FOR pv_l= abel ON DOMAIN test_123 IS 'label_val';
=C2=A0 =C2=A0 Cor= rect Query :-=C2=A0
=C2=A0 =C2=A0 =C2=A0 =C2=A0SECURITY LABEL FOR= =C2=A0pv_label=C2=A0ON DOMAIN <schema_name>.test_123 IS 'label_va= l';


Let us know in case of any = issues.

Thanks,
Neel Patel

On Tue, Feb 2, 2016= at 3:51 PM, Khushboo Vashi <khushboo.vashi@enterprisedb.com= > wrote:
<= div>
Hi Neel,

Thanks for reviewing my patch= .

I have modified the code as per your suggestions and also fi= xed some of the issues got while doing unit testing.
Please find a= ttached patch for the same.


Thanks,
Khushboo
<= div>



On Wed= , Jan 20, 2016 at 10:56 PM, Neel Patel <neel.patel@enterprisedb.= com> wrote:
Hi Khushboo,

Please find below review comments.
<= div>
- While creating new Domain and clicking on SQL tab, pyt= hon side we are getting error saying "TypeError: 'bool' obj= ect is not callable".
=C2=A0 We are not able to c= reate any domain. Fix this issue so that we can test other functionality.
- Implement the reverse engineering SQL generation for the domain = node.
- As per the checklist, remove the "Use Slony" fr= om Constraints tab, as it is not required.
- No need to pass &quo= t;qtIdent=3Dself.qtIdent" as function argument in "create&= quot; and "getSQL" function in domains/__init__.py
- In= "Security" tab , provider and security label fields are not edit= able.
- In PG version 9.1, when we update the existing domain nam= e then "ALTER DOMAIN" is not supported.=C2=A0
=C2=A0 Cu= rrently there is no checking for the PG version 9.1 and 9.2_plus. It will f= ail when we connect to database 9.1

=C2=A0 e.g.
=C2=A0 For PG version 9.1 - Update command should be as below.
=C2=A0 ALTER TYPE xyz RENAME TO abc;
=C2=A0 For PG version = 9.2 onwards - Update command should be as below.
=C2=A0 ALTER DOM= AIN xyz RENAME TO abc;

- Some of the SQL file, qtI= dent is not used. Please check all the related SQL files.
=C2=A0 = e.g. =C2=A0- In update.sql file "data.owner" should be "conn= |qtIdent(data.owner)"

=C2=A0 =C2=A0 {% if dat= a.owner %}
=C2=A0 =C2=A0 ALTER DOMAIN {{ conn|qtIdent(o_data.base= nsp, name) }}
=C2=A0 =C2=A0 =C2=A0 OWNER TO {{ data.owner }};
=C2=A0 {% endif %}

Let us know for any issu= es.

Thanks,
Neel Patel
<= div>

On Wed, Jan 2= 0, 2016 at 2:50 PM, Khushboo Vashi <khushboo.vashi@enterpris= edb.com> wrote:
Hi Neel,

Please find updated patch.
Thanks,
Khushboo

On Wed, Jan 20, 2016 at 12:50 PM, Neel = Patel <neel.patel@enterprisedb.com> wrote:
Hi Khushboo,

While applying the patch file, we are getting below warnings.
#########################################
domai= ns (1).patch:1340: trailing whitespace.
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 oid: undefined,=C2=A0
domains (1).patch:1483: trailing= whitespace.
=C2=A0 =C2=A0 (nspname =3D 'pg_catalog' AND = EXISTS=C2=A0
domains (1).patch:1487: trailing whitespace.
=C2=A0 =C2=A0 OR (nspname =3D 'information_schema' AND EXISTS=C2= =A0
domains (1).patch:1489: trailing whitespace.
=C2=A0= =C2=A0 OR (nspname LIKE '_%' AND EXISTS=C2=A0
domains (1= ).patch:1642: trailing whitespace.
=C2=A0(select 1 from pg_class = where relnamespace=3Dtypnamespace and relname =3D typname and relkind !=3D = 'c') AND (typname not like '_%' OR NOT EXISTS (select 1 fro= m pg_class where relnamespace=3Dtypnamespace and relname =3D substring(typn= ame from 2)::name and relkind !=3D 'c'))=C2=A0
warning: s= quelched 4 whitespace errors
warning: 9 lines add whitespace erro= rs.
#########################################

Can you please remove the whitespace and regenerate the pat= ch ?

Thanks,
Neel Patel

On Wed, Jan = 20, 2016 at 12:37 PM, Khushboo Vashi <khushboo.vashi@enterpr= isedb.com> wrote:
Resending patch with binary option.
=

On Wed,= Jan 20, 2016 at 10:18 AM, Khushboo Vashi <khushboo.vashi@en= terprisedb.com> wrote:
Hi,

Please find attached patc= h for the Domain Module.

The patch will be modified after Type= s module implementation as we need to populate Base Type=C2=A0 and some Typ= e related validations from the Types module.

Please revie= w it and let me know the feedback.

Thanks,
Khushboo



--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers=






--001a11423afc47418f052adb6a33--