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

From: Joao De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>
To: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: [pgadmin4][patch] Initial patch to decouple from ACI Tree
Date: 2018-04-02 18:45:32
Message-ID: CAE+jja=Gdd032H7tpoZD2C0m2R7SnTZpHX_oPx2K2zGbaaW9yg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

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

Attachment Content-Type Size
0001-Tree-of-information-on-the-current-status-of-the-app.patch application/octet-stream 14.1 KB
0002-Extractions-of-multiples-locations-where-we-used-the.patch application/octet-stream 142.9 KB

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Joao De Almeida Pereira 2018-04-02 19:41:52 Re: [pgAdmin4][RM#3155] Allow user to lock the Layout
Previous Message Murtuza Zabuawala 2018-04-02 14:07:01 Re: [pgAdmin4][RM#3155] Allow user to lock the Layout