Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed problem with HashMap caused by Java8 Upgrade #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dngferreira
Copy link

Java 8 HashMap were rewritten and don't respect the insert order when you call .keySet() on the HashMap ( From the documentation, order was never guaranteed from the start, it just worked that way ).
The code uses HashMaps to represent maps where the order of the keys is important.

@gardes
Copy link
Member

gardes commented Apr 29, 2015

Hi - I would never have expected that keySet on a Hashmap guaranteed any order.
And I am not even sure it always did this in openJDK or so.

Is the order really relevant in the code? Or is it just relevant in the tests that now fail?

In particular, I don't see why it would fail anything in the Archetype Validator (one of the bits I know better than others).

If it is only the tests, I would avoid changing the implementation with the overhead of a LinkedHashMap, but rather fix the tests?

@dngferreira
Copy link
Author

Hi,
The function findMatchingRMClass() of class org.openehr.build.RMObjectBuilder seems to care about the order of the key-values of the typeMap HashMap, to decide the class of the RMObject.
The problematic part is also this code:

for (String attr : filteredMap.keySet()) {
                if (!attributes.contains(attr)) {

                    log.debug("unknown attribute: " + attr);

                    matched = false;
                }
            }

Lets use as an example the test that was failling, I have an ItemList valueMap and I want to know the RMClass of this value map. With the old implementation of HashMaps the typeMap map would be read in the order in which it is inserted and ItemList would appear before Section ( the problematic case for this test). In Java 8 the way the values are returned it mixes the order and you end up with Section before ItemList. Since ItemList attributes appear in Section the previous code would never hit the matched = false; part and it would leave the for loop with matched = true.

This would return that the RMClass of the valueMap is Section, when reality is an ItemList.

English is not my main language so I hope the problem is clear.
There might be a better solution for this or it could be that this fix brings problems in other places, I'm too new to OpenEhr to see the big picture of this change.

@gardes
Copy link
Member

gardes commented Jun 20, 2016

I agree, this needs to be changed.

Are the two "Fixed problem in XML where the top element was not defined" commits related to the Java8-HashMap problem in any way? Not sure I completely understand the problem solved / changes made.

If not: can we keep this separate - I can cherry-pick your first commit or just redo your changes or you can provide a Pull Request with this change only, whatever you prefer?

Iago Corbal and Malik Firose have worked in the area of the last two commits about the XML problem I believe and could maybe have a look at these commits separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants