per bz triage (asantos, ccrouch, mfoley. loleary)

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...

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.

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...

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...

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...

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...

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.

setting back to ON_DEV until potentially related test failures are resolved.

master http://git.fedorahosted.org/git/?p=rhq/rhq.git;a=commitdiff;h=2007f75d4be72277f3ac62e81ee626b283c0f800
Author: Lukas Krejci <lkrejci>
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.

With this unit test issue resolved, moving back to ON_QA. See test notes
above.

An additional commit that should have been logged against this issue:

master commit 08827539668357e9b87e55f752ec2d20aeeeb6da
Author: Jay Shaughnessy <jshaughn>
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().

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.

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.

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.