[Freeipa-devel] [PATCH] 023 Circular entity dependency

[Petr Vobornik]

Comments in text. Also attached git diff for convenience.

On 10/17/2011 10:39 PM, Endi Sukma Dewata wrote:
 > On 10/10/2011 10:13 AM, Petr Vobornik wrote:
 >> https://fedorahosted.org/freeipa/ticket/1531
 >>
 >> (3.0 Core Effort Iteration 01 September Y11 Release)
 >>
 >> Implemented solution:
 >> * all entities are created on application start
 >> * dependant objects (facets and dialogs) are created at once on their
 >> first use in entity.
 >>
 >> Note(patch naming): patch 022 was second part of 021, but the file name
 >> was wrong(021-1)
 >
 > Some comments/issues:
 >
 > 1. One of the goals of this bug is to remove the temporary workaround in
 > IPA.search_facet.create_content(). We should now be able to call the
 > initialize_table_columns() during facet initialization.

Fixed

 > 2. Using lazy-loading to create entities, facets, and dialogs makes
 > object creations a little bit unpredictable. This is probably fine for
 > now, but if there's a problem the other option is to create all objects
 > during application initialization. We can use a loop to create all
 > entities first, then use another loop to create all dependent objects in
 > each entity.

I don't think it's necessary  but if it becomes a problem, we can use 
the initialization loop.

 > 3. Another goal is to replace entity names used in spec (see
 > other_entity & nested_entity spec properties) with the actual entity
 > objects. In this case it might be better to use the loops described in
 > #2. This can be done separately.

Wouldn't it lead to the circular dependancy problem again? I think using 
entity names and calling IPA.get_entity at the time when it is needed is 
fine. But we should make some naming conversions of function params or 
object properties to distinguish when we are working with just name or 
entity itself.

 > 4. In the original code, when creating a facet for indirect association
 > it will try to find the corresponding direct facet and use it instead of
 > creating a new one. In the new code, the indirect facet will always be
 > created, but since there is no indirect facet group the facet will never
 > appear. It would be better if we can avoid unnecessary creation of
 > indirect facets.

Fixed

 > 5. In entity.js:201, the use of entity.title for the breadcrumb tooltip
 > might not be appropriate because usually the title is plural whereas the
 > breadcrumb points to a single object. It would be better to use the
 > entity.metadata.label_singular.

Fixed

 > 6. Invoking a method by concatenating the method name dynamically such
 > as prepare_<facet type>_spec will work, but it's more error prone and
 > will clutter up the namespace. It would be better to store the methods
 > in a map like this:
 >
 > that.map.put('search', function(spec) {
 > ...
 > });
 >
 > and use it like this:
 >
 > var method = that.map.get('search');
 > method(spec);
 >
 > This can be done separately.
Reworked. Used object as a map, ordered map isn't necessary.

 >
 > 7. The code in entity.js:474,998,1000 should have a deeper indentation
 > because it's a continuation of the previous line.

Fixed

 > 8. The facet_specs and dialog_specs lists can be replaced with
 > ordered_map. It already has a method to find an element by its name.
 > This can be done separately.
 >
My intention was to be able specify entity facets and dialogs simply by 
specifying their spec in entity spec (without calling builder function), 
this is possible now. I didn't want to add initialization logic for 
facet_specs (to fill the ordered_map) to entity in order to keep it as 
simple as possible. It would be nice though. Maybe we could make an 
utility object with methods for some simple initialization logic which 
is used on many places - like checking if boolean is set or some more 
complicated like initialization of ordered_map (needs key selector fn as 
parameter).

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0023-1-Circular-entity-dependency.patch
Type: text/x-patch
Size: 21198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20111018/4d45b42a/attachment.bin>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: diff
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20111018/4d45b42a/attachment.ksh>