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

Thanks, patch applied.

On Wed, Jan 8, 2020 at 11:37 AM Khushboo Vashi <
khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:

> Hi,
>
> Please find the attached updated patch.
>
> - The Generate Icon has been changed to
> https://fontawesome.com/v4.7.0/icon/file-code-o .
>
> Thanks,
> Khushboo
>
> On Thu, Jan 2, 2020 at 3:08 PM Khushboo Vashi <
> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>>
>>
>> On Thu, Jan 2, 2020 at 2:43 PM Aditya Toshniwal <
>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>
>>> 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.
>>>>
>>> The return value can be taken and tweaked to fit your purpose. If let's
>>> say a new version of Postgres is released then that also need to be added
>>> in schema diff along with get_version_mapping_directories. That would
>>> not be the case if you use get_version_mapping_directories.
>>>
>> I can do one thing, will take the versions dict at one place and both the
>> functions get_version_mapping_directories and schema_diff can use that
>> version dict.
>> As, I am not in favour of tweaking get_version_mapping_directories
>> function as the name suggests something else.
>>
> I have changed the logic for the version comparison, so now no need to
> hard code the version values and no need to change the existing code.
>
>>
>>>>> 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?
>>>>
>>> Obviously the extra append operations, which can be avoided.
>>>
>> Yeah, right but I don't think it is going to be a performance issue.
>>
>>>
>>>>> 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,
>>>>> + })
>>>>>
>>>>> This is pending.
>>>
>> It's not pending, I left it as it is, as I have copied this code from
>> backform.pgadmin.js and tweaked it accordingly, as we already cleaned up
>> the async: false in the past but this has not been changed. So, before
>> changing it, we need to analyse why we have not changed it.
>>
>>>
>>>>> 1. 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.
>>>>
>>> I'm not comparing "map" and "for" here. I'm trying to avoid
>>> sel_rows_data.push statement here which is directly proportional to
>>> the number of selected rows.
>>>
>> At the end, it will return the new array according to the condition in
>> both the cases.
>>
>>>
>>>>> 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?
>>>>
>>> The fetch_objects_to_compare function fetches all the nodes first, and
>>> then in a loop the properties for each node is fetched. Instead of that,
>>> all the nodes along with their properties can be fetched in one go from the
>>> database. Although this is no stopper, but it can be an improvement done in
>>> future.
>>>
>> Sure, will look into it in the second phase.
>>
>>
>>>
>>>> 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"
>>>>>
>>>>
>>>
>>> --
>>> Thanks and Regards,
>>> Aditya Toshniwal
>>> pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune
>>> "Don't Complain about Heat, Plant a TREE"
>>>
>>

--
*Thanks & Regards*
*Akshay Joshi*

*Sr. Software Architect*
*EnterpriseDB Software India Private Limited*
*Mobile: +91 976-788-8246*