Re: [Feature-3452] Schema diff tool with Table, View, Materialized View, Functions and Procedures

Hi Khushboo,

On Thu, Jan 2, 2020 at 12:04 PM Khushboo Vashi <
khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:

> Hi Aditya,
>
> Thanks for the review. Please find the inline response.
> Also, the updated patch attached.
>
> On Fri, Dec 27, 2019 at 4:55 PM Aditya Toshniwal <
> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>
>> Hi Akshay/Khushboo,
>>
>> I have few suggestions/questions for the attached patch:
>>
>> 1. Code like SchemaDiffRegistry('server', ServerNode) should be
>> replaced with SSchemaDiffRegistry(ServerModule.NODE_TYPE, ServerNode)
>>
>> Done
>
>>
>> 1. The variables return_ajax_response, only_sql, json_resp as far as
>> I understood are similar. Can we have same var name everywhere ?
>>
>> only_sql is used when I need only SQL but not to be executed in the
> backend. json_resp is used to have the plain text/json response.
>
>>
>> 1. Remove commented code -
>> web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
>> -> get_sql_from_table_diff
>>
>> Done
>
>>
>> 1. In
>> web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
>> -> fetch_tables - keys_to_remove is passed. How is it different from
>> keys_to_ignore used at other places ?
>>
>> keys_to_remove used to remove the table properties before comparison as
> we combine all child nodes of the table while comparing and keys_to_ignore
> is to ignore the keys while comparing. We have also used the keys_to_ignore
> in the table node itself.
>
>>
>> 1. web/pgadmin/tools/schema_diff/__init__.py
>> -> check_version_compatibility has hardcoded version numbers. Can we
>> use get_version_mapping_directories from
>> web/pgadmin/utils/versioned_template_loader.py ?
>>
>> I have checked the possibilities before using it in the schema diff. The
> * purpose and the return values * are different for both the files.
>
>>
>> 1. Rename .reallyHidden to .really-hidden
>>
>> Done
>
>>
>> 1. CSS class #schema-diff-grid -> background: white; - hardcoded
>> color can be changed to use $color-bg instead. Also use rem or px for -
>> font-size: 9pt.
>>
>> Done
>
>>
>> 1.
>> 2. .slick-group-toggle.collapsed, .slick-group-toggle.expanded - svgs
>> are not required. Font awesome has the icons. refer - .obj_properties
>> .collapsed .caret::before.
>>
>> Done
>
>>
>> 1.
>> 2. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
>> -> formatNode - Appends can be avoided and formed in a single statement.
>> + } else {
>> + return $('<span></span>').append(
>> + $('<span></span>', {
>> + class: 'wcTabIcon ' + optimage,
>> + })
>> + ).append($('<span></span>').text(opt.text));
>> + }
>> +};
>>
>> Any harm in this approach?
>
>>
>> 1.
>> 2. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
>> -> fetchData - We should not use async = false.
>> + $.ajax({
>> + async: false,
>> + url: url,
>> + })
>> 3. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
>> Use 'sources/window' - pgWindow.
>>
>>
>> 1. + let preferences = (window.opener !== null) ?
>> window.opener.pgAdmin.Browser.get_preferences_for_module('schema_diff') :
>> window.top.pgAdmin.Browser.get_preferences_for_module('schema_diff');
>>
>> Done
>
>>
>> 1. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js - Use
>> map instead of for loop. It will also remove the sel_rows_data.push. will
>> be helpfull in large data.
>> + for (var row = 0; row < sel_rows.length; row++) {
>> + let data = self.grid.getData().getItem(sel_rows[row]);
>> +
>> + if (data.type) {
>> + let tmp_data = {
>> + 'node_type': data.type,
>> + 'source_oid': parseInt(data.oid, 10),
>> + 'target_oid': parseInt(data.oid, 10),
>> + 'comp_status': data.status,
>> + };
>> +
>> + if(data.status && (data.status.toLowerCase() ==
>> 'different' || data.status.toLowerCase() == 'identical')) {
>> + tmp_data['target_oid'] = data.target_oid;
>> + }
>> + sel_rows_data.push(tmp_data);
>> + }
>> + }
>> +
>> + url_params['sel_rows'] = sel_rows_data;
>>
>> This is a debatable topic as there are pros and cons of both map and for
> loop. Like, it's more readable if we use map and in case of for loop,
> chrome and firefox will be more happy in terms of performance.
>
>>
>> 1.
>> 2. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -Are
>> we doing anything to handle failure.
>> + connect_database(server_id, db_id, callback) {
>> + var url = url_for('schema_diff.connect_database', {'sid':
>> server_id, 'did': db_id});
>> + $.post(url)
>> + .done(function(res) {
>> + if (res.success && res.data) {
>> + callback(res.data);
>> + }
>> + })
>> + .fail(function() {
>> + // Fail
>> + });
>> +
>> + }
>>
>> Forgot to handle, now added.
>
>>
>> 1.
>> 2. As you've added a completely different function for
>> connect_server, I would suggest to rename dlgServerPass to a different name
>> to avoid conflict with existing dlgServerPass in server.js
>> + if (!Alertify.dlgServerPass) {
>> + Alertify.dialog('dlgServerPass', function factory() {
>>
>> As we open the schema diff in different frame, I think, this should not
> be the issue.
>
>>
>> 1.
>> 2. Generate script does not work if pgAdmin opened in iframe. Iframes
>> are used by tools like Katacoda.
>> [image: Screenshot 2019-12-27 at 16.40.47.png]
>>
>> Fixed, good catch.
>
>>
>> 1.
>> 2. Comparing objects loader is not attached to DDL Comparison panel.
>> [image: compare_overlay.png]
>>
>> Fixed.
>
>>
>> 1.
>> 2. Filter icon and Generate script icon size are different. Also
>> change icons CSS to use font-icon. You can refer icons from sqleditor.
>> [image: Screenshot 2019-12-27 at 12.18.00.png]
>>
>> The problem is, for the generate script icon, I have used the svg (as no
> similar icon in font-awesome) whereas for Filter, font-awesome is used. I
> can replace the Generate Script icon from font-awesome (can search for some
> similar icon) if Chetana agrees.
> @Chethana Kumar <chethana(dot)kumar(at)enterprisedb(dot)com> , please have a look.
>
> https://fontawesome.com/v4.7.0/icon/file-code-o
>

We can use the above one as it looks appropriate to me.

Thanks,
Chethana kumar

>
> https://fontawesome.com/v4.7.0/icon/file-text-o
>
>>
>> 1.
>>
>> *The fetch_objects_to_compare function used in each node uses loop to
>> fetch data. Although it is working for now, but I would suggest using bulk
>> fetch nodes instead of looping through all the nodes one by one.*
>>
>
> Can you please elaborate the approach which you are suggesting?
>
> Thanks,
> Khushboo
>
>>
>> On Fri, Dec 20, 2019 at 6:59 PM Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Hackers,
>>>
>>> Attached is the implementation of the new feature Schema Diff Tool.
>>> Initial work(backend code to compare the objects) has been done by me and
>>> then most of the task has been completed by *Khushboo Vashi. *Sending
>>> the patch on behalf of her*.*
>>>
>>> Currently, this tool only supports Tables, Views, Materialized Views,
>>> Functions and Procedures node.
>>>
>>> Please review and test it thoroughly. Suggestions are welcome to improve
>>> the tool.
>>>
>>> --
>>> *Thanks & Regards*
>>> *Akshay Joshi*
>>>
>>> *Sr. Software Architect*
>>> *EnterpriseDB Software India Private Limited*
>>> *Mobile: +91 976-788-8246*
>>>
>>
>>
>> --
>> Thanks and Regards,
>> Aditya Toshniwal
>> pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune
>> "Don't Complain about Heat, Plant a TREE"
>>
>

--
Chethana Kumar
Principal UI/UX Designer
EnterpriseDB Corporation

The Postgres Database Company

P: +91 86981 57146
www.enterprisedb.com