[KULRICE-11529] Consolidate and simplify ID generation logic Created: 23/Dec/13  Updated: 21/May/15  Resolved: 31/Jan/14

Status: Closed
Project: Kuali Rice Development
Component/s: Development
Affects Version/s: None
Fix Version/s: Not version specific
Security Level: Public (Public: Anyone can view)

Type: Task Priority: Major
Reporter: Jerry Neal (Inactive) Assignee: Mark Fyffe (Inactive)
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: 0 minutes
Time Spent: 1 day
Original Estimate: 4 hours

Similar issues:
KULRICE-5323Dataset Consolidation
KULRICE-111Simplify UI control pluggability
KULRICE-1306Data/DDL consolidation and bootstrap process
KULRICE-6321Database script consolidation for release
KULRICE-10878Simplify ObjectPropertyUtils
KULRICE-11597Generated lookup/inquiry ids contain invalid characters
KULRICE-13283Grouping classes and ids bad for row grouping in collection
KULRICE-8424Consolidate database scripts for release
KULRICE-2177Consolidate all Rice Struts modules into a single struts module
KULRICE-7871Checkbox and Radio Group ids are not correct
Rice Module:
KRAD
KRAD Feature Area:
UIF Component
KAI Review Status: Not Required
KTI Review Status: Not Required
Code Review Status: Not Required
Include in Release Notes?:
Yes

 Description   

Cleanup item moved from KULRICE-9997:

Extract ID assignment code (org.kuali.rice.krad.uif.service.impl.ViewHelperServiceImpl#preprocessView) to helper method or class, along with Component State, Create helper methods to reduce nesting and duplicate code

Related discussion:

Mark,

Right. So we could change AssignIdsTasks to use the same code as ViewHelperServiceImpl#preprocessView, that is using the path instead of the index?

I think the concern with testing, even after we get the things stable, is if a developer makes a change that moves the component (say push it down in a list or something of that sort), the scripts would need to be modified. If we had a way of generating the Ids (for example based on name for inputs), that would allow for more stable tests.

Thanks,
Jerry

From: Mark Fyffe mark.w.fyffe@gmail.com
Sent: Thursday, December 12, 2013 8:22 AM
To: 'Jerry Neal'
Subject: RE: ids

This does make sense. Now that path is available by virtue of getComponentsForLifecycle(), it should be feasible to universally assign ID based on path.

There are several cases where components are dynamically created and added to the lifecycle, particularly during the apply model phase. Although it doesn’t do so right now, spawnSubLifecycle can assign the path in these cases making path a viable seed for generating the IDs even for new components. This won’t help the generateAndAssignIds() method used by LightTable and BreadcrumbOptions, however, since the purpose of this method appears to be to discard existing and assign new IDs to all components in the subtree – do you know what purpose this serves?

One reason IDs keep changing is because we are making changes to when they are assigned, and also changes that affect the order in which lifecycle components are created. Once the development effort settles the IDs should stabilize – however, I’d expect that the upcoming focus on DOM and component cleanup will continue to shake up recorded scripts.

Mark

From: Jerry Neal jkneal@iu.edu
Sent: Wednesday, December 11, 2013 2:16 PM
To: 'Mark Fyffe'
Subject: RE: ids

Hi Mark,

Thanks for the information! I found the problem. It is due to the way I am running the lifecycle now for component refresh. The index on the phase was not the same and therefore I was getting different ids.

I do think it would be good to consolidate the ID code. In fact, we might want to add the method to Component. We have been discussing IDs recently due to complaints from testers. The issue is, the IDs we are creating could change if a developer modifies the structure. Recorded scripts will grab these ids (if present), and then not work after updates. For things like inputs, it would be better if we didn’t have the Id on there, and selenium would use the name. However, we need the id in many cases. So we have been talking about how we can make these more static across updates. One approach is to use information from the component. For example, data/input fields have the property path which should be unique in most cases. Actions have the method to call, etc. So we might need different Id generation methods depending on the component. Does that make sense?

Thanks,
Jerry

From: Mark Fyffe mark.w.fyffe@gmail.com
Sent: Tuesday, December 10, 2013 3:43 PM
To: Jerry Neal
Subject: Re: ids

There are three places now that generate IDs - each will produce a different ID but the results should be repeatable given the same set of circumstances. It will probably be best to consolidate these to a single utility - although it is not currently the case, path is available during the lifecycle, and within pre-process phase, so it should be possible to base all IDs on the path.
ViewHelperServiceImpl.preprocessView() - Assigns IDs to all components involved in the initialize phase prior to caching the view. Uses path, class name, and parent component ids to maintain an evolving hash code as it traverses the tree. The results should be the same each time the same view is loaded and cached from the DD.
AssignIdsTask - Assigns IDs during the initialize phase based on the class name and list position for all predecessor phases in the lifecycle phase tree. The results should be the same on each run as long as the components are processed in the same order. IDs are only assigned if not already present. This covers components dynamically created and spawned.
ComponentUtils.generateAndAssignIds() - Used by LightTable and BreadcrumbOptions. IDs use an evolving hash based on class name and previously assigned ID. Current IDs are discarded and replaced with new values. Results should be repeatable as long as the components provided are consistent.
In all cases, the ID generated is checked against a set maintained in ViewIndex to prevent duplicate IDs from being generated for the same view. If the ID has already been generated, it is considered a hash collision, so will be seeded again until an ID that hasn't been used for the current view is generated.
Off the top of my head, I can't think of a reason why a component would get a new ID on refresh. I does seem possible, though, since ID assignment and manipulation takes place during performComponentLifecycle(). Do you have an example of where it is happening?
Best,
Mark

On Tue, Dec 10, 2013 at 2:45 PM, Jerry Neal <jkneal@iu.edu> wrote:
Hi Mark,

The new Id generation strategy you put in is based on the component path correct? So if we refreshed a component, the id that gets generated should be the same correct? For some reason, I am not seeing this happen currently.

Thanks,
Jerry



 Comments   
Comment by Mark Fyffe (Inactive) [ 27/Dec/13 ]

This work is complete on the performance branch. Unit tests have been updated and are passing, and initial regression testing didn't turn over any issues.

  • Added path property to LifecycleElement
  • Removed index from ViewLifecyclePhaseBase.
  • Simplified ID generation to a hash calculated from class name and path.
  • Converted sublifecycle to use a path expression for determining component, rather than passing the component itself.
  • Added strict checks to ensure a component's path refers to the component relative to the view.
  • Set path to populate in pre-process phase, as well as prior to executing tasks on all lifecycle phases.

Note that strict mode is not slower due to path/element verification, so should be disabled in non-development environments (if not already).

Comment by Mark Fyffe (Inactive) [ 28/Dec/13 ]

Reopening for further simplification based on items below from Jerry. To address this, I am converting sub-lifecycle spawning to be automatically and eliminating the need to explicitly spawn when components are dynamically added to the graph.

Mark,

A follow-up on this. I’m trying to think of why we are spawning a sub-lifecycles here in the first place? There could be fields added from the remote fields holder, but in most cases these are just the items from the collection, and the init phase should have been run on these.

Thanks,
Jerry

From: Jerry Neal jkneal@iu.edu
Sent: Friday, December 27, 2013 1:24 PM
To: 'Mark Fyffe'
Subject: lineFields

HI Mark,

I was wondering about the code in CollectionGroupBuilder:

List<Field> lineFields = processAnyRemoteFieldsHolder(collectionGroup, lineItems);
collectionGroup.addLineFields(lineSuffix, lineFields);
ViewLifecycle.spawnSubLifecyle(model, collectionGroup, "lineFields["+lineSuffix+"]", null, null, false);

And the corresponding property lineFields in CollectionGroup. Is it necessary to add the property to CollectionGroup because the lifecycle requires a path? What might be slick, is when a lifecycle phase is being run on a component, if the previous phase has not been run it will trigger that phase first. So in this case, these fields will eventually get attached to the lifecycle through the layout manager, and the apply model phase would get triggered on each. I just worry about the confusion the additional holder properties might bring.

Thanks,
Jerry

Another small item related to this request, since it pertains to AssignIdsTask:

One additional small style thing I just noticed in the id creation code:

// Iteratively take the product of the hash and another large prime
hash *= 4507; // until a unique ID has been generated.
// The use of large primes will minimize collisions, reducing the
// likelihood of race conditions leading to components coming out
// with different IDs on different server instances and/or test runs.

We like to avoid end of line comments. How about this instead?

// Iteratively take the product of the hash and another large prime
// until a unique ID has been generated.
// The use of large primes will minimize collisions, reducing the
// likelihood of race conditions leading to components coming out
// with different IDs on different server instances and/or test runs.
hash *= 4507;

In this case I would almost suggest moving the information about the use of primes to the method doc, and then having the inline comment just be:

// Iteratively take the product of the hash and another large prime
// until a unique ID has been generated.
hash *= 4507;

I tend to dislike large blocks of inline comments.

Also, in general for the inline comments just start with a lower case letter and don’t end with a period:

// Add the element's path to the hash code.

To

// add the element's path to the hash code

I realize this is a little picky, but we are defining our style at this level so we will be consistent.

Comment by Mark Fyffe (Inactive) [ 29/Dec/13 ]

Removed spawnSubLifecycle and completed regression testing in KRAD sampleapp.

Spawning sub-lifecycle phases for components that are not yet at the required lifecycle phase is now automated as part of the initializeSuccessors implementation in InitializeComponentPhase, ApplyModelComponentPhase, and FinalizeComponentPhase.

In regression testing, it was noted that ActionField duplicates properties from Action, which resulted in duplicated lifecycle processing for some pages. Also, it was noted that TreeGroup needed to have bean properties added to support the original getComponentsForLifecycle() and getComponentPrototypes() behavior.

Comment by Mark Fyffe (Inactive) [ 27/Jan/14 ]

Reopened temporarily pending code review during F2F.

Renamed LifecycleElementState#getPath() to #getParentPath()‏
Removed LifecycleElement#getPath()

Double-checked unit tests and KRAD sampleapp

Generated at Sat Jul 11 21:11:45 CDT 2020 using JIRA 6.1.5#6160-sha1:a61a0fc278117a0da0ec9b89167b8f29b6afdab2.