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

Hi Joao,
On Wed, May 9, 2018 at 8:30 PM, Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> 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.
>
I have already commited the patch - 0001.

I have two review comments for 0002 patch.
*In pgadmin/static/js/tree/tree.js:*
1. In TreeNode, we're keepging the reference of DOMElement, do we really
need it?
i.e.
*export class TreeNode {*
* constructor(id, data, domNode, parent) {*
* this.id <http://this.id> = id;*
* this.data = data;*
* this.setParent(parent);*
* this.children = [];*
* this.domNode = domNode;*
* }*
* ...*
*}*
*....*

2. We're exporting a tree object on line #189.
i.e.
*export let tree = new Tree();*

Are you expecting the tree class to be a singleton class?
I think not. Hence - it should not be exported.

I am currently reviewing 0003 patch.
Will share my comments soon.

We will talk about the architecture change in separate mail chain.

-- Thanks, Ashesh

> @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*
>>
>