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

Despite this being a work-in-progress. We've provided committable code:
We've ensured it works like before.

For right now, our strategy is looking for calls to `itemData(`. We've
noticed that these surface a lot of calls that are duplicated throughout
the code base. We'd like to reduce the amount of duplication as much as
possible for multiple reasons:
- we've noticed that it makes the code easier to reason about.
- we've noticed that it makes the code much easier to test (there's a lot
of functionality we'd like to have coverage for).
- ultimately will make the goal of decoupling the code from the aci tree
library easier.

For instance, the following lines in the picture below show up in too many
places.
[image: Screen Shot 2018-04-30 at 10.50.24 AM.png]

You can expect in the next patches to see more and more functions extracted
with the goal of decoupling the tree in mind.

Anthony && Joao

On Mon, Apr 30, 2018 at 6:25 AM Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
wrote:

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