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

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
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"
>