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

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)
2. The variables return_ajax_response, only_sql, json_resp as far as I
understood are similar. Can we have same var name everywhere ?
3. Remove commented code -
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
-> get_sql_from_table_diff
4. 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 ?
5. 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 ?
6. Rename .reallyHidden to .really-hidden
7. 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.
8. .slick-group-toggle.collapsed, .slick-group-toggle.expanded - svgs
are not required. Font awesome has the icons. refer - .obj_properties
.collapsed .caret::before.
9. 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));
+ }
+};
10. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
-> fetchData - We should not use async = false.
+ $.ajax({
+ async: false,
+ url: url,
+ })
11. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
Use 'sources/window' - pgWindow.
+ 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');
12. 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;
13. 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
+ });
+
+ }
14. 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() {
15. 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]
16. Comparing objects loader is not attached to DDL Comparison panel.
[image: compare_overlay.png]
17. 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 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.*

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)
> 2. The variables return_ajax_response, only_sql, json_resp as far as I
> understood are similar. Can we have same var name everywhere ?
> 3. Remove commented code -
> web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
> -> get_sql_from_table_diff
> 4. 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 ?
> 5. 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 ?
> 6. Rename .reallyHidden to .really-hidden
> 7. 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.
> 8. .slick-group-toggle.collapsed, .slick-group-toggle.expanded - svgs
> are not required. Font awesome has the icons. refer - .obj_properties
> .collapsed .caret::before.
> 9. 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));
> + }
> +};
> 10. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
> -> fetchData - We should not use async = false.
> + $.ajax({
> + async: false,
> + url: url,
> + })
> 11. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
> Use 'sources/window' - pgWindow.
> + 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');
> 12. 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;
> 13. 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
> + });
> +
> + }
> 14. 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() {
> 15. 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]
> 16. Comparing objects loader is not attached to DDL Comparison panel.
> [image: compare_overlay.png]
> 17. 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 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.*
>
> 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"
>

--
Thanks and Regards,
Aditya Toshniwal
pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune
"Don't Complain about Heat, Plant a TREE"