Red Hat Bugzilla – Full Text Bug Listing
|Summary:||Map configuration properties not shown in correct order|
|Product:||[Other] RHQ Project||Reporter:||Heiko W. Rupp <hrupp>|
|Component:||Core UI||Assignee:||Jay Shaughnessy <jshaughn>|
|Status:||CLOSED CURRENTRELEASE||QA Contact:||Mike Foley <mfoley>|
|Version:||4.2||CC:||hrupp, jshaughn, lkrejci|
|Target Release:||RHQ 4.4.0|
|Fixed In Version:||Doc Type:||Bug Fix|
|Doc Text:||Story Points:||---|
|Last Closed:||2013-09-01 05:59:48 EDT||Type:||---|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
|Bug Depends On:|
|Bug Blocks:||707223, 782579|
Description Heiko W. Rupp 2012-02-01 06:51:20 EST
Created attachment 558800 [details] Screenshot I have <c:list-property name="xa-properties" displayName="XA Properties" description="Additional XA Properties (connection url is set above)" required="false"> <c:map-property name="xa-properties"> <c:simple-property name="key" displayName="Key" description="Key of the property"/> <c:simple-property name="value" displayName="Value" description="Value of the property"/> </c:map-property> </c:list-property> in the operations UI those are not show "key" | "value" but reverse which is confusing like hell
Comment 1 Mike Foley 2012-02-06 12:02:33 EST
per bz triage (asantos, ccrouch, mfoley. loleary)
Comment 2 Jay Shaughnessy 2012-03-21 16:13:03 EDT
I can see an example of this issue in my RHQ Server -> JMS XA connection factory -> ResourceConfig -> Config Property field. It looks like the last declared field shows up first. Investgating...
Comment 3 Jay Shaughnessy 2012-03-21 17:01:12 EDT
The issue appears to be that rhq_config_prop_def.order_index is 0 for all of the parent map children prop defs. The rows are parsed and stored in correct order but the order_index is not set. Fetch order is not guaranteed to be in the order of storage. When the order_index is set it is respected, so it seems we need to ensure it is set on initial load of the type def. If we can assume that the ids for the child prop defs are sequential then the following query can repair an existing db: update rhq_config_prop_def cpd set order_index = (id - ( select min(id) from rhq_config_prop_def where parent_map_definition_id = cpd.parent_map_definition_id)) where parent_map_definition_id > 0 and order_index = 0 I believe that is a fair assumption as I think config defs are completely replaced when updated, but I need to verify.
Comment 4 Jay Shaughnessy 2012-03-21 17:32:51 EDT
Hmmm, on further investigation the above is still true but I'm not sure why it's relevant or changes behavior. The map of children is supposed to be ordered by id due to an @OrderBy annotation in the entity model, which should give us the desired behavior. Still investigating...
Comment 5 Jay Shaughnessy 2012-03-21 17:43:09 EDT
Going on in my mazz-like conversation with myself... The problem may be that we've got bad annotations on this entity. I see both @MapKey("name") and @OrderBy. From what I see in the docs the OrderBy doesn't actually do anything for maps and MapKey is used instead. But the MapKey on the property name would be weird for ordering unless it were for some sort of alphabetical display purpose, which would be pretty bad as the name isn't even the displayName. I think that removing @OrderBy and changing to @MapKey (without a param, therefore defaulting to the id) may be what we want. Still investigating...
Comment 6 Jay Shaughnessy 2012-03-21 18:00:15 EDT
Well that didn't work. Although I do think it's right if we were to use the implicit ordering, but in fact we do have the order_index field, and I see now in the GUI's ConfigurationEditor that we use it to determine the order of properties. Which is why when I repair the database with the above query we get what we expect. We may be able to enhance the comparator such that it uses a two-level sort, orderIndex, id. So if orderIdex is not set (all 0s) then the id will take over, which respects the db insert order, which in turn respects the XML ordering in the descriptor. Still investigating...
Comment 7 Jay Shaughnessy 2012-03-22 11:48:14 EDT
I think I was wrong in Comment 5. I now think the annotations are correct. The question now is, I think, what are we really trying to do, or expecting to do with the annotations vs. the use of order_index (aka PropertDefintion.order)? The annotations/impl give us a LinkedHashMap<propDefName,propDefValue> with ordered-iteration on propDefId, which is the insertOrder, which is the ordering of elements in the plugin descriptor. So why have order_index as opposed to just assuming the ordering should be as it is defined in the plugin descriptor? Is it ever overridden? It doesn't seem like it because I see nothing in the configuration XSD that allows for specifying ordering, it seems the element order is the desired order. As it stands, as pointed out above, order_index is not being set for the properties of a Map. We have a few options: 1) We could easily start assigning an order_index to the map properties. The Comparable definitions in the code already seem to assume it is set. We would need to update existing dbs with the query above. If we do this it begs the question of why we have the @OrderBy annotation on PropertyDefinitionMap.map (and maybe in some other places). All that does is force in-memory sorting of the fetched data, slowing it down, for reasons that escape me. (note - we have a significant number of @OrderBy annotations on entities, if we don't use the ordering then these just slow things down. 2) We can leave order_index unset for maps. In this case we need to potentially update the Comparable impls to compare against 'id' if 'order' is equal. But really, since order_index is just incremented along with id, we shouldn't really even need to do compares, iterating the ordered Map (due to the annotations) should be sufficient for propDef ordering, whether it's a Map or not. Need to consult with someone...
Comment 8 Jay Shaughnessy 2012-03-23 17:42:06 EDT
master commit b06ddaf9833b0bcc3ac81f3b8fd71aaca06d7b21 See the above comments for more detail, the thinking here was somewhat involved. In short, the ConfigPropDefs for Map props were not being assigned an order_index when parsed from the descriptor and persisted into the db. But code assumed it was set properly, using that field in comparators to determine ordering. The order in which properties are declared in the plugin descriptor should be honored, and now it is. Additionally, given that we have order_index we shouldn't be relying on the @OrderBy annotation on the entity's map field. It's confusing, and unreliable given the chance of meta-data-update. The order_index field should be used when sorting properties is required and as such, the annotation has been removed. [In fact, we should review the use of OrderBy across the board, another bz will be generated.] While adding tests for this it became clear that metadata update for map properties was itself badly broken. This has also been fixed, and now also sets order_index properly. This fix actually provides the upgrade path for this fix. When updating plugins the proper ordering will get applied automatically during the metadata update. Test Notes: This must also be tested against an upgraded db. The upgrade should fix the problem automatically. Test against oracle and postgres.
Comment 9 Jay Shaughnessy 2012-03-25 20:58:54 EDT
setting back to ON_DEV until potentially related test failures are resolved.
Comment 10 Lukas Krejci 2012-03-27 08:54:29 EDT
master http://git.fedorahosted.org/git/?p=rhq/rhq.git;a=commitdiff;h=2007f75d4be72277f3ac62e81ee626b283c0f800 Author: Lukas Krejci <firstname.lastname@example.org> Date: Tue Mar 27 14:52:59 2012 +0200 [BZ 786416 - Map configuration properties not shown in correct order] Apache configuration mapping now uses the correct method that returns the child property definitions in order.
Comment 11 Jay Shaughnessy 2012-03-27 11:13:35 EDT
With this unit test issue resolved, moving back to ON_QA. See test notes above.
Comment 12 Jay Shaughnessy 2012-10-03 09:32:28 EDT
An additional commit that should have been logged against this issue: master commit 08827539668357e9b87e55f752ec2d20aeeeb6da Author: Jay Shaughnessy <email@example.com> Date: Sun Mar 25 21:08:20 2012 -0400 Another check-in for this. It seems more code is dependent on an ordered list of map properties than originally thought. All of this code was in danger of getting an unexpected ordering of map properties. Refactor PropertyDefinitionMap.getProperties() to return the ordered set of map properties, and provide a new getMap() method to be used as needed. Solidify by making it clear whether the calling code is getting an ordered set of properties or accessing the unordered Map. This hopefully will fix a couple test failures and also prevent other potential runtime issues. Note that this commit changed the return type for PropertyDefinitionMap.getProperties(), which unwittingly changed the public API because Domain classes are accessible to client apps and plugin code. The change was necessary to provide ordered results, thus fixing several issues, but to maintain the API we should have kept the existing method as is. At this point the best workaround is to update the client code by either using PropertyDefinitionMap.getMap(), keeping the return type the same, or to change the resturn type for calls to PropertyDefinitionMap.getProperties().
Comment 13 Jay Shaughnessy 2012-11-15 14:03:57 EST
Setting back to ON_DEV. The last commit changed the public API in PropertyDefinitionMap. After some discussion we've decide that we need to change it back to maintain the documented public API. The new solution wil be to maintain the Map return value but to have the underlying implementation be a SortedMap, such that keyset() and values() will return in the order expected.
Comment 14 Jay Shaughnessy 2012-11-16 10:11:09 EST
On second thought, setting back to ON_QA since it was never verified, and opening new BZ for the revert work. Note that the revert work in bug 877179 requires the same testing as this BZ.
Comment 15 Heiko W. Rupp 2013-09-01 05:59:48 EDT
Bulk closing of items that are on_qa and in old RHQ releases, which are out for a long time and where the issue has not been re-opened since.