Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree - Mailing list pgadmin-hackers

Hi Hackers

There were very good points raised. Do you think that these are blockers for 0001, 0002, and 0003 getting committed? In general, we felt like there were points made about very specific issues, were there any broader concerns about the patches? We don't need an agreement on architecture yet for patches 0001, 0002, and 0003.

@Khushboo Vashi 

Patch 0003 - I have some concerns regarding the approach taken to separate out the PG utilities. The dialogue factory is responsible for creating the dialogue which uses dialogue wrapper. So, any module should call the dialogue factory to create a new dialogue instance. Here dialogue itself call the dialogue factory for creating it.

We did not fully understand what your concern is on this point.

If you are concerned that we have a dialogWrapper and a dialog class and don't understand the difference between the two?

We don't feel too strongly about this decision. It seemed easier at the time (to test) because the wrapper represents what will be passed into the alertify library. We're certainly open to having this implementation change in the future.

In the enable_disable_triggers.js file, can we just import alertifyjs instead of passing in all the functions?

 We made this decision to make testing of the functionality easier to test, by using a principle called dependency injection. https://martinfowler.com/articles/injection.html. If we import alertifyjs directly, then we can't stub out its functionality.

 

Patch 0004 - Right now with the new proposed architecture only JS files are re-arranged. To maintain the pluggability I would also like to have some discussion on the back end side. Right now, the code maintains everything. Let say I want to add any node under schema I will just drop the related folder and files in that hierarchy and it will work. But with the proposed architecture I can not visualize the backend architecture and this should not be ended with only JS restructuring.

This patch in fact only moves Javascript files from one folder structure to a different folder structure. The process we are following is an iterative process and will be built upon itself. The end vision that we have for the product is as depicted in the previous email an Intent Driven Structure and in this structure each folder will contain a Feature that will be self contained (Backend + Frontend). The major difference between what we have now and what we envision this application to look like is that these folders can live at the same level so that we know at a glance what this application is capable of doing. This structure will also make it easier to onboard someone that is looking to help building any feature because they will have access to all of the features  in the "same screen" instead of having to do a deep dive into the application code in order to find the place to start.

As of right now we haven't though very deep on the backend side of things. But looking at the current structure moving from a hierarchical file structure into a horizontal one, doesn't look like a lot of plumbing. Nevertheless this is something that we should address when we get there and we believe we are not there yet.

 @Akshay Joshi  

In menu/can_create.js, "parentCatalogOfTableChild" function name is unable to understand. 

The intent behind this function is to retrieve all the tree child node that matches a given type argument and has a direct parent of the catalog type. The name was in a hurry and could certainly be improved. Any suggestions are welcome. How about **matchesTypeAndHasCatalogParent(self, arg)**?

Below code is bit confusing in terms of naming convention canCreate.canCreate inside "canCreate".

That sounds great. Let's do that.

Why we have "menu_utils.js" for Backup, Maintenance, Restore and Grant Wizard? Can we have one single file with all the supported node for respective module?

It was for expressing developer intent. The menu_utils.js that were placed in discrete packages have functionality that pertain only to its package. If we thought that the functionality were shared, we wouldn't have put them in the shared folder `pgadmin4/web/pgadmin/static/js/menu/...`

 

Also I have seen that the folder hierarchy for grant wizard is "/web/pgadmin/static/js/grant/wizard/menu_utils.js" why there are two separate folders grant and wizard (sub folder of grant)?

It makes sense to think of the wizard as separate entity from grants. You could imagine that when we add additional functionality for grants, that we'd want it alongside the wizard functionality. You could call it premature at this point. 

@pgadmin-hackers

We understand that our vision for the application is quite bold and if we, as a community, decide that this is the way to go it is going to take some time to accomplish and will involve effort. But  we believe this will take the code into a better place.

Patches 1,2 and 3 are our priority at this point, because there are features that we want to implement that depend on the separation from ACI Tree, and we would like to see these in master as soon as possible. 

Joao and Anthony

On Wed, May 9, 2018 at 8:38 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Joao

Below are my review comments: 

• In menu/can_create.js, "parentCatalogOfTableChild" function name is unable to understand. 
  
• Below code is bit confusing in terms of naming convention canCreate.canCreate inside "canCreate". 

canCreate: function(itemData, item, data) {

  return canCreate.canCreate(pgBrowser, 'coll-collation', item, data);

}

Can we have something like "menuCreator.canCreate"?

• Why we have "menu_utils.js" for Backup, Maintenance, Restore and Grant Wizard? Can we have one single file with all the supported node for respective module? Also I have seen that the folder hierarchy for grant wizard is "/web/pgadmin/static/js/grant/wizard/menu_utils.js" why there are two separate folders grant and wizard (sub folder of grant)?

On Wed, May 9, 2018 at 4:32 PM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi Joao,

Patch 0001 & 0002 (esp the new tree implementation) look good to me.

Patch 0003 - I have some concerns regarding the approach taken to separate out the PG utilities. The dialogue factory is responsible for creating the dialogue which uses dialogue wrapper. So, any module should call the dialogue factory to create a new dialogue instance. Here dialogue itself call the dialogue factory for creating it.

In the enable_disable_triggers.js file, can we just import alertifyjs instead of passing in all the functions?

Patch 0004 - Right now with the new proposed architecture only JS files are re-arranged. To maintain the pluggability I would also like to have some discussion on the back end side. Right now, the code maintains everything. Let say I want to add any node under schema I will just drop the related folder and files in that hierarchy and it will work. But with the proposed architecture I can not visualize the backend architecture and this should not be ended with only JS restructuring.

Thanks,

Khushboo

On Tue, May 8, 2018 at 12:31 AM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:

Hi Hackers,

We noticed that some of the latest changes created some conflicts with this patch. So you can find attached the patch rebased on the new master.

Thanks

Victoria && Joao

On Fri, May 4, 2018 at 6:03 PM Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:

And now the patches......

On Fri, May 4, 2018 at 6:01 PM Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:

Hi Hackers,

Be prepared this is going to be a big email.

Objectives of this patch:

1. Start the work to split ACI from pgAdmin code
2. Add more tests around the front end components
3. Start the discussion on application architecture

1. Start the work to split ACI from pgAdmin code

Why

This journey started on our first attempt to store state of the current tree, so that when a user accessed again pgAdmin
there would be no need to reopen all the nodes and make the flow better. Another problem users brought to us was related
to the UI behavior when the number of objects was too big.

In order to do implement this features we would have to play around with aciTree or some functions from our code that
called and leveraged the aciTree functionalities. To do this we needed to have some confidence that our changes would
implement correctly the new feature and it would not break the current functionality.
The process that we used was first extract the functions that interacted with the tree, wrap them with
tests and then refactor them in some way. While doing this work we realized that the tree started spreading roots
throughout the majority of the application.

How

Options:

1. Rewrite the front end
2. One bang replace ACI Tree without change on the application
3. Create an abstraction layer between ACI Tree and the application

1. Rewrite the front end

Pros	Const
Achieve decoupling by recreating the frontend	Cost is too high
Do not rely on deprecated or unmaintained libraries	Turn around is too long
	One shot change

2. One bang replace ACI Tree

Pros	Const
Achieve decoupling by recreating the frontend	Cost is too high
	Does not achieve the decoupling unless we change the application
	Need to create an adaptor
	No garantee of success
	One shot change

3. Create an abstraction layer between ACI Tree and the application

Pros	Const
Achieve decoupling of the application and the tree	90% of the time on Code archaeology
Lower cost
Ability to change or keep ACI Tree, decision TBD
Can be done in a iterative way

We decided that options 3 looked more attractive specially because we could do it in a iterative way.

The next image depicts the current state of the interactions between the application and the ACI and the place we want
to be in.

for retrieving of data from the ACI Tree that then is used in different ways.

Because we are doing this in a iterative process we need to keep the current tree working and create our adaptor next
to it. In order to do that we created a new property on PGBrowser class called treeMenu and when ACI Tree receives
the node information we also populate the new Tree with all the needed information.

This approach allow us not to worry, for now, with data retrieval, URL system and other issues that should be addressed
in the future when the adaptor is done.

This first patch tries to deal with the low hanging fruit, functions that are triggered by events from ACI Tree.
Cases like registering to events and triggering events need to be handled in the future as well.

2. Add more tests around the front end components

Why

Go Fast Forever: https://builttoadapt.io/why-tdd-489fdcdda05e
I think this is a very good summary of why do we need tests in our applications.

3. Start the discussion on application architecture

Why should we care about location of files inside a our application?

Why is this way better the another?

These are 2 good questions that have very lengthy answers. Trying to more or less summarize the answers we care about
the location of the files, because we want our application to communicate intent and there are always pros and cons
on all the decisions that we make.

At this point the application structure follows our menu, this approach eventually make is easier to follow the code
but at the same time if the menu changes down the line, will we change the structure of our folders?

The proposal that we do with the last diff of this patch is to change to a structure that slices vertically the
application. This way we can understand intent behind the code and more easily find what we are looking for.

In the current structure if you want to see the tables code you need to go to
pgAdmin/browser/server_groups/servers/databases/schemas/tables/ this is a huge path to remember and to get to. What
do we win with this? If we open pgAdmin we know which nodes to click in order to get to tables. But for development
every time that you are looking for a specific functionality you need to run the application, navigate the menu so that
you know where you can find the code. This doesn’t sound very appealing.

What if our structure would look like this:

 - web   - tables     - controller       - get_nodes.py       - get_sql.py       - __init__.py     - frontend        - component            - ddl_component.js        - services            - table-service.js   - schemas   - servers   - ....

This would saves us time because all the information that we need is what are we working on and everything is right there.

Menu driven structure	Intent Driven Structure
Pros:	Pros:
Already in place	Explicitly shows features
Self contained features	Self contained features
Support for drop in features	Support for drop in features
Cons:	Cons:
Follows the menu, and it could change	Need to change current code
Hard to find features	Some additional plumbing might be needed
Drop in features need to be placed in a specific location according to the menu location

What are your thought about this architecture?

Around minute 7 of this video Uncle Bob shows an application written
in rails to talk about architecture. It is a long KeyNote if you are curious I would advise you to see the full video.
His approach to architecture of the application is pretty interesting.

---

Patches

0001 Change the order of the shims on the shim file

Simple change that orders the shims inside the shim file

0002 New tree implementation

This is the first version of our Tree implementation. At this point is a very simple tree without no abstractions and
with code that eventually is not very performant, but this is only the first iteration and we are trying to follow the
Last Responsible Moment Principle.

What can you find in this patch:

• Creation of PGBrowser.treeMenu
• Initial version of the Tree Adaptor web/pgadmin/static/js/tree/tree.js
• TreeFake test double that can replace the Tree for testing purposes
• Tests. As an interesting asside because Fake’s need to behave like the real object you will noticed that there are
  tests for this type of double and they the same as of the real object.

0003 Extract, Test and Refactor Methods

This is the patch that contains the change that we talked a in the objectives. It touches in a subset of the places
where itemData function is called. To have a sense of progress the following image depicts, on the left all places
where this functions can be found in the code and on the right the places where it still remains after this patch.

But this patch is not only related to the ACI Tree, it also creates some abstractions from code that we found repeated.
Some examples of this are the dialogs for Backup and Restore, where the majority of the logic was common, so we created
in web/pgadmin/static/js/alertify/ 3 different objects:
A Factory DialogFactory that is responsible for creating new Dialog and these dialogs will use the DialogWrapper.

This is also a good example of the Last Responsible Moment Principle,
in Dialog there are still some functions that are used in a Recover and Backup scenario. When we have more examples
we will be able to, if we want, to extract that into another function.

While doing some code refactoring we found out that the Right Click Menu as some checks that can be centralized. Examples
of these are checks for Enable/Disable of an option of the display of the Create/Edit/Drop option in some menus.
Check web/pgadmin/static/js/menu/ for more information. This simplified code like:

      canCreate: function(itemData, item, data) {
-        //If check is false then , we will allow create menu
-        if (data && data.check == false)
-          return true;
-
-        var t = pgBrowser.tree, i = item, d = itemData;
-        // To iterate over tree to check parent node
-        while (i) {
-          // If it is schema then allow user to create domain
-          if (_.indexOf(['schema'], d._type) > -1)
-            return true;
-
-          if ('coll-domain' == d._type) {
-            //Check if we are not child of catalog
-            var prev_i = t.hasParent(i) ? t.parent(i) : null,
-              prev_d = prev_i ? t.itemData(prev_i) : null;
-            if( prev_d._type == 'catalog') {
-              return false;
-            } else {
-              return true;
-            }
-          }
-          i = t.hasParent(i) ? t.parent(i) : null;
-          d = i ? t.itemData(i) : null;
-        }
-        // by default we do not want to allow create menu
-        return true;
+        return canCreate.canCreate(pgBrowser, 'coll-domain', item, data);      }

This refactor was made in a couple of places throughout the code.

Another refactor that had to be done was the creation of the functions getTreeNodeHierarchyFromElement and
getTreeNodeHierarchyFromIdentifier that replace the old getTreeNodeHierarchy. This implementation was done
because for the time being we have 2 different types of trees and calls to them.

The rest of the changes resulted from our process of refactoring that we are following:

1. Find a location where itemData is used
2. Extract that into a function outside of the current code
3. Wrap it with tests
4. When we have enough confidence on the tests, we refactor the function to use the new Tree
5. We refactor the function to become more readable
6. Start over

0004 Architecture

The proposed structure is our vision for the application, this is not the state we will be in when this patch lands.
For now the only change we are doing is move the javascript into static/js/FEATURE_NAME. This is a first step in the
direction that we would like to see the product heading.

Even if we decide to keep using the same architecture the move of Javascript to a centralized place will
not break any plugability as webpack can retrieve all the Javascript from any place in the code.

Have a nice weekend
Joao

​

--

Akshay Joshi

Sr. Software Architect

Phone: +91 20-3058-9517
Mobile: +91 976-788-8246