Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

On Mon, Apr 30, 2018 at 3:51 PM, Anthony Emengo <aemengo(at)pivotal(dot)io> wrote:

> Hi there,
>
> Yes, there's a lot of TODO that we intend for this effort - to be
> submitted. We'd like to remove as much *direct* invocations on the ACI
> Tree library, as it causes a lot of coupling to the library. It is not the
> final patch, but we cannot come up with a definitive list of the things we
> intend to do, at this time.
>
Is there any known TODO list?
So that - I can help you figure out (if any more).

-- Thanks, Ashesh

>
> On Mon, Apr 30, 2018 at 2:16 AM, Ashesh Vashi <
> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>>
>>
>> On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira <
>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>
>>> Hi Hackers,
>>> As you are aware we kept on working on the patch, so we are attaching to
>>> this email a new version of the patch.
>>> This new version contains all the changes in the previous one plus more
>>> extractions of functions and refactoring of code.
>>>
>>> The objective of this patch is to create a separation between pgAdmin
>>> and the ACI Tree. We are doing this because we realized that at this point
>>> in time we have the ACI Tree all over the code of pgAdmin. I found a very
>>> interesting article that really talks about this:
>>> https://medium.freecodecamp.org/code-dependencies-are-the-de
>>> vil-35ed28b556d
>>>
>>> In this patch there are some visions and ideas about the location of the
>>> code, the way to organize it and also try to pave the future for a
>>> application that is stable, easy to develop on and that can be release at a
>>> times notice.
>>>
>>> We are investing a big chunk of our time in doing this refactoring, but
>>> while doing that we also try to respond to the patches sent to the mailing
>>> list. We would like the feedback from the community because we believe this
>>> is a changing point for the application. The idea is to change the way we
>>> develop this application, instead of only correcting a bug of developing a
>>> feature, with every commit we should correct the bug or develop a feature
>>> but leave the code a little better than we found it (Refactoring,
>>> refactoring, refactoring). This is hard work but that is what the users
>>> from pgAdmin expect from this community of developers.
>>>
>>>
>>> ======
>>>
>>>
>>>
>>> It is a huge patch
>>> 86 files changed, 5492 inserts, 1840 deletions
>>> and we would like to get your feedback as soon as possible, because we
>>> are continuing to work on it which means it is going to grow in size.
>>>
>>>
>>> At this point in time we still have 124 of 176 calls to the function
>>> itemData from ACITree.
>>>
>>> What does each patch contain:
>>> 0001:
>>> Very simple patch, we found out that the linter was not looking into
>>> all the javascript test files, so this patch will ensure it is
>>>
>>> 0002:
>>> New Tree abstraction. This patch contains the new Tree that works as
>>> an adaptor for ACI Tree and is going to be used on all the extractions that
>>> we are doing
>>>
>>> 0003:
>>> Code that extracts, wrap with tests and replace ACI Tree invocations.
>>> We start creating new pattern for the location of Javascript files and
>>> their structure.
>>> Create patterns for creation of dialogs (backup and restore)
>>>
>>
>> Do you have some TODO left for the same?
>> Or, is this the final one? Because - it gives us the better understanding
>> during reviewing the patch.
>>
>> -- Thanks, Ashesh
>>
>>>
>>>
>>> Thanks
>>> Joao
>>>
>>>
>>> On Fri, Apr 27, 2018 at 5:34 AM Ashesh Vashi <
>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>> I have quite a few comments for the patch.
>>>> I will send them soon.
>>>>
>>>> On Fri, Apr 27, 2018, 14:45 Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> How is your work on this going Ashesh? Will you be committing today?
>>>>>
>>>>> On Wed, Apr 25, 2018 at 8:52 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Ashesh; you had agreed to work on this early this week. Please ensure
>>>>>> you do so today.
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>> On Tue, Apr 24, 2018 at 8:13 PM, Joao De Almeida Pereira <
>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>
>>>>>>> Hi Hackers,
>>>>>>>
>>>>>>> Can someone review and merge this patch?
>>>>>>>
>>>>>>> Thanks
>>>>>>> Joao
>>>>>>>
>>>>>>> On Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereira <
>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>
>>>>>>>> Hi Hackers,
>>>>>>>> Any other comment about this patch?
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Joao
>>>>>>>>
>>>>>>>> On Tue, Apr 10, 2018 at 12:00 PM Joao De Almeida Pereira <
>>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>>
>>>>>>>>> Hello Khushboo
>>>>>>>>>
>>>>>>>>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi <
>>>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Joao,
>>>>>>>>>>
>>>>>>>>>> I have reviewed your patch and have some suggestions.
>>>>>>>>>>
>>>>>>>>>> On Sat, Apr 7, 2018 at 12:43 AM, Joao De Almeida Pereira <
>>>>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hello Murtuza/Dave,
>>>>>>>>>>> Yes now the extracted functions are spread into different files.
>>>>>>>>>>> The intent would be to make the files as small as possible, and also to
>>>>>>>>>>> group and name them in a way that would be easy to understand what each
>>>>>>>>>>> file is doing without the need of opening it.
>>>>>>>>>>> As a example:
>>>>>>>>>>> static/js/backup will contain all the backup related
>>>>>>>>>>> functionality inside of this folder we can see the file:
>>>>>>>>>>>
>>>>>>>>>> menu_utils.js At this moment in time we decided to group all the
>>>>>>>>>>> functions that are related to the menu, but we can split that also if we
>>>>>>>>>>> believe it is easier to see.
>>>>>>>>>>>
>>>>>>>>>> It's really very good to see the separated code for backup
>>>>>>>>>> module. As we have done for backup, we would like do it for other PG
>>>>>>>>>> utilities like restore, maintenance etc.
>>>>>>>>>> Considering this, we should separate the code in a way that some
>>>>>>>>>> of the common functionalities can be used for other modules like menu (as
>>>>>>>>>> you have mentioned above), dialogue factory etc.
>>>>>>>>>> Also, I think these functionalities should be in their respective
>>>>>>>>>> static folder instead of pgadmin/static.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> About the location of the files. The move of the files to
>>>>>>>>> pgadmin/static/js was made on purpose in order to clearly separate
>>>>>>>>> Javascript from python code.
>>>>>>>>> The rational behind it was
>>>>>>>>> - Create a clear separation between the backend and frontend
>>>>>>>>> - Having Javascript code concentrated in a single place,
>>>>>>>>> hopefully, will encourage to developers to look for a functionality, that
>>>>>>>>> is already implemented in another modules, because they are right there.
>>>>>>>>> (When we started this journey we realized that the 'nodes' have a big
>>>>>>>>> groups of code that could be shared, but because the Javascript is spread
>>>>>>>>> everywhere it is much harder to look for it)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> There are some drawbacks of this separation:
>>>>>>>>> - When creating a new module we will need to put the javascript in
>>>>>>>>> a separate location from the backend code
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> static/js/datagrid folder contains all the datagrid related
>>>>>>>>>>> functionality
>>>>>>>>>>>
>>>>>>>>>> Same as backup module, this should be in it's respective
>>>>>>>>>> static/js folder.
>>>>>>>>>>
>>>>>>>>>>> Inside of the folder we can see the files:
>>>>>>>>>>> get_panel_title.js is responsible for retrieving the name of
>>>>>>>>>>> the panel
>>>>>>>>>>> show_data.js is responsible for showing the datagrid
>>>>>>>>>>> show_query_tool.js is responsible for showing the query tool
>>>>>>>>>>>
>>>>>>>>>>> Does this structure make sense?
>>>>>>>>>>> Can you give an example of a comment that you think is missing
>>>>>>>>>>> and that could help?
>>>>>>>>>>>
>>>>>>>>>>> As a personal note, unless the algorithm is very obscure or very
>>>>>>>>>>> complicated, I believe that if the code needs comments it is a signal that
>>>>>>>>>>> something needs to change in terms of naming, structure of the part in
>>>>>>>>>>> question. This being said, I am open to add some comments that might help
>>>>>>>>>>> people.
>>>>>>>>>>>
>>>>>>>>>> You are right, with the help of naming convention and structure
>>>>>>>>>> of the code, any one can get the idea about the code. But it is very useful
>>>>>>>>>> to understand the code
>>>>>>>>>> very easily with the proper comments especially when there are
>>>>>>>>>> multiple developers working on a single project.
>>>>>>>>>>
>>>>>>>>>> I found some of the places where it would be great to have
>>>>>>>>>> comments.
>>>>>>>>>>
>>>>>>>>>> - treeMenu: new tree.Tree() in a browser.js
>>>>>>>>>> - tree.js (especially Tree class)
>>>>>>>>>>
>>>>>>>>> About the comment point I need a more clear understanding on what
>>>>>>>>> kind of comments you are looking for. Because when you read the function
>>>>>>>>> names you understand the intent, what they are doing. The parameters also
>>>>>>>>> explain what you need to pass into them.
>>>>>>>>>
>>>>>>>>> If what you are looking for in these comments is the reasoning
>>>>>>>>> being the change itself, then that should be present in the commit message.
>>>>>>>>> Specially because this is going to be a very big patch with a very big
>>>>>>>>> number of changes.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>>> Joao
>>>>>>>>>>> ​
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>> Khushboo
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Apr 6, 2018 at 4:48 AM Murtuza Zabuawala <
>>>>>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Joao,
>>>>>>>>>>>>
>>>>>>>>>>>> Patch looks good and working as expected.
>>>>>>>>>>>>
>>>>>>>>>>>> I also agree with Dave, Can we please add some comments in each
>>>>>>>>>>>> file which can help us to understand the flow, I'm saying because now the
>>>>>>>>>>>> code is segregated in so many separate files it will be hard to keep track
>>>>>>>>>>>> of the flow from one file to another when debugging.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Murtuza Zabuawala
>>>>>>>>>>>> EnterpriseDB: http://www.enterprisedb.com
>>>>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Apr 5, 2018 at 7:08 PM, Joao De Almeida Pereira <
>>>>>>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Khushboo,
>>>>>>>>>>>>> Attached you can find both patches rebased
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi <
>>>>>>>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Joao,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Can you please rebase the second patch?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Khushboo
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida Pereira <
>>>>>>>>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Hackers,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Attached you can find the patch that will start to decouple
>>>>>>>>>>>>>>> pgAdmin from ACITree library.
>>>>>>>>>>>>>>> This patch is intended to be merged after 3.0, because we do
>>>>>>>>>>>>>>> not want to cause any entropy or delay the release, but we want to start
>>>>>>>>>>>>>>> the discussion and show some code.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This job that we started is a massive tech debt chore that
>>>>>>>>>>>>>>> will take some time to finalize and we would love the help of the community
>>>>>>>>>>>>>>> to do it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *Summary of the patch:*
>>>>>>>>>>>>>>> 0001 patch:
>>>>>>>>>>>>>>> - Creates a new tree that will allow us to create a
>>>>>>>>>>>>>>> separation between the application and ACI Tree
>>>>>>>>>>>>>>> - Creates a Fake Tree (Test double, for reference on the
>>>>>>>>>>>>>>> available test doubles: https://martinfowler.
>>>>>>>>>>>>>>> com/bliki/TestDouble.html) that can be used to inplace to
>>>>>>>>>>>>>>> replace the ACITree and also encapsulate the new tree behavior on our tests
>>>>>>>>>>>>>>> - Adds tests for all the tree functionalities
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 0002 patch:
>>>>>>>>>>>>>>> - Extracts, refactors, adds tests and remove dependency
>>>>>>>>>>>>>>> from ACI Tree on:
>>>>>>>>>>>>>>> - getTreeNodeHierarchy
>>>>>>>>>>>>>>> - on backup.js: menu_enabled, menu_enabled_server,
>>>>>>>>>>>>>>> start_backup_global_server, backup_objects
>>>>>>>>>>>>>>> - on datagrid.js: show_data_grid, get_panel_title,
>>>>>>>>>>>>>>> show_query_tool
>>>>>>>>>>>>>>> - Start using sprintf-js as Underscore.String is deprecating
>>>>>>>>>>>>>>> sprintf function
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This patch represents only 10 calls to ACITree.itemData out
>>>>>>>>>>>>>>> of 176 that are spread around our code
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *In Depth look on the process behind the patch:*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> We started writing this patch with the idea that we need to
>>>>>>>>>>>>>>> decouple pgAdmin4 from ACITree, because ACITree is no longer supported, the
>>>>>>>>>>>>>>> documentation is non existent and ACITree is no longer being actively
>>>>>>>>>>>>>>> developed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Our process:
>>>>>>>>>>>>>>> 1. We "randomly" selected a function that is part of the
>>>>>>>>>>>>>>> ACITree. From this point we decided to replace that function with our own
>>>>>>>>>>>>>>> version. The function that we choose was "itemData".
>>>>>>>>>>>>>>> The function gives us all the "data" that a specific node of
>>>>>>>>>>>>>>> the tree find.
>>>>>>>>>>>>>>> Given in order to replace the tree we would need to have a
>>>>>>>>>>>>>>> function that would give us the same information. We had 2 options:
>>>>>>>>>>>>>>> a) Create a tree with a function called itemData
>>>>>>>>>>>>>>> Pros:
>>>>>>>>>>>>>>> - At first view this was the simpler solution
>>>>>>>>>>>>>>> - Would keep the status quo
>>>>>>>>>>>>>>> Cons:
>>>>>>>>>>>>>>> - Not a OOP approach
>>>>>>>>>>>>>>> - Not very flexible
>>>>>>>>>>>>>>> b) Create a tree that would return a node given an ID and
>>>>>>>>>>>>>>> then the node would be responsible for giving it's data.
>>>>>>>>>>>>>>> Pros:
>>>>>>>>>>>>>>> - OOP Approach
>>>>>>>>>>>>>>> - More flexible and we do not need to bring the tree
>>>>>>>>>>>>>>> around, just a node
>>>>>>>>>>>>>>> Cons:
>>>>>>>>>>>>>>> - Break the current status quo
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Given these 2 options we decided to go for a more OOP
>>>>>>>>>>>>>>> approach creating a Tree and a TreeNode classes, that in the future will be
>>>>>>>>>>>>>>> renamed to ACITreeWrapper and TreeNode.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 2. After we decided on the starting point we searched for
>>>>>>>>>>>>>>> occurrences of the function "itemData" and we found out that there were 303
>>>>>>>>>>>>>>> occurrences of "itemData" in the code and roughly 176 calls to the function
>>>>>>>>>>>>>>> itself (some of the hits were variable names).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 3. We selected the first file on the search and found the
>>>>>>>>>>>>>>> function that was responsible for calling the itemData function.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 4. Extracted the function to a separate file
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 5. Wrap this function with tests
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 6. Refactor the function to ES6, give more declarative names
>>>>>>>>>>>>>>> to variables and break the functions into smaller chunks
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 7. When all the tests were passing we replaced ACITree with
>>>>>>>>>>>>>>> our Tree
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 8. We ensured that all tests were passing
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 9. Remove function from the original file and use the new
>>>>>>>>>>>>>>> function
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 10. Ensure everything still works
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 11. Find the next function and execute from step 4 until all
>>>>>>>>>>>>>>> the functions are replaced, refactored and tested.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> As you can see by the process this is a pretty huge
>>>>>>>>>>>>>>> undertake, because of the number of calls to the function. This is just the
>>>>>>>>>>>>>>> first step on the direction of completely isolating the ACITree so that we
>>>>>>>>>>>>>>> can solve the problem with a large number of elements on the tree.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *What is on our radar that we need to address:*
>>>>>>>>>>>>>>> - Finish the complete decoupling of the ACITree
>>>>>>>>>>>>>>> - Performance of the current tree implementation
>>>>>>>>>>>>>>> - Tweak the naming of the Tree class to explicitly tell us
>>>>>>>>>>>>>>> this is to use only with ACITree.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>> Joao
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>> The Enterprise PostgreSQL Company
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: http://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>
>>
>