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

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 <khushboo(dot)vashi(at)enterprisedb(dot)com>

> 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 <akshay(dot)joshi(at)enterprisedb(dot)com>

> 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 <pgadmin-hackers(at)postgresql(dot)org>
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(dot)joshi(at)enterprisedb(dot)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(dot)vashi(at)enterprisedb(dot)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(at)pivotal(dot)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(at)pivotal(dot)io> wrote:
>>>
>>>> And now the patches......
>>>>
>>>>
>>>> On Fri, May 4, 2018 at 6:01 PM Joao De Almeida Pereira <
>>>> jdealmeidapereira(at)pivotal(dot)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.
>>>>>
>>>>> [image: Screen Shot 2018-05-04 at 4.06.17 PM.png]
>>>>>
>>>>> 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
>>>>> <https://www.youtube.com/watch?v=hALFGQNeEnU> 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
>>>>> <https://medium.com/@aidanjcasey/guiding-principles-for-an-evolutionary-software-architecture-b6dc2cb24680>
>>>>> .
>>>>>
>>>>> 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
>>>>> <https://martinfowler.com/bliki/TestDouble.html> 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
>>>>> <https://medium.com/@aidanjcasey/guiding-principles-for-an-evolutionary-software-architecture-b6dc2cb24680>
>>>>> ,
>>>>> 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-9517Mobile: +91 976-788-8246*
>