Author: markt Date: Wed Oct 31 21:57:26 2012 New Revision: 1404374 URL: http://svn.apache.org/viewvc?rev=1404374&view=rev Log: Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54068 Re-write the fragment ordering algorithm to over come multiple problems. Expand the unit tests to cover the identified issues.
Modified: tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java tomcat/trunk/test/org/apache/catalina/deploy/TestWebXmlOrdering.java Modified: tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java?rev=1404374&r1=1404373&r2=1404374&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java (original) +++ tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java Wed Oct 31 21:57:26 2012 @@ -26,7 +26,6 @@ import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -2121,75 +2120,137 @@ public class WebXml { } } } else { - List<String> order = new LinkedList<>(); - // Start by adding all fragments - order doesn't matter - order.addAll(fragments.keySet()); - - // Now go through and move elements to start/end depending on if - // they specify others - for (WebXml fragment : fragments.values()) { - String name = fragment.getName(); - if (fragment.getBeforeOrdering().contains(WebXml.ORDER_OTHERS)) { - // Move to beginning - order.remove(name); - order.add(0, name); - } else if (fragment.getAfterOrdering().contains(WebXml.ORDER_OTHERS)) { - // Move to end - order.remove(name); - order.add(name); - } - } - - // Now apply remaining ordering + // Stage 1. Make all dependencies bi-directional - this makes the + // next stage simpler. for (WebXml fragment : fragments.values()) { - String name = fragment.getName(); for (String before : fragment.getBeforeOrdering()) { - if (!before.equals(WebXml.ORDER_OTHERS) && - order.contains(before) && - order.indexOf(before) < order.indexOf(name)) { - order.remove(name); - order.add(order.indexOf(before), name); + if (!before.equals(ORDER_OTHERS)) { + fragments.get(before).addAfterOrdering(fragment.getName()); } } for (String after : fragment.getAfterOrdering()) { - if (!after.equals(WebXml.ORDER_OTHERS) && - order.contains(after) && - order.indexOf(after) > order.indexOf(name)) { - order.remove(name); - order.add(order.indexOf(after) + 1, name); + if (!after.equals(ORDER_OTHERS)) { + fragments.get(after).addBeforeOrdering(fragment.getName()); } } } - // Finally check ordering was applied correctly - if there are - // errors then that indicates circular references + // Stage 2. Make all fragments that are implicitly before/after + // others explicitly so. This is iterative so the next + // stage doesn't have to be. for (WebXml fragment : fragments.values()) { - String name = fragment.getName(); - for (String before : fragment.getBeforeOrdering()) { - if (!before.equals(WebXml.ORDER_OTHERS) && - order.contains(before) && - order.indexOf(before) < order.indexOf(name)) { - throw new IllegalArgumentException( - sm.getString("webXml.mergeConflictOrder")); - } + if (fragment.getBeforeOrdering().contains(ORDER_OTHERS)) { + makeBeforeOthersExplicit(fragment.getAfterOrdering(), fragments); } - for (String after : fragment.getAfterOrdering()) { - if (!after.equals(WebXml.ORDER_OTHERS) && - order.contains(after) && - order.indexOf(after) > order.indexOf(name)) { - throw new IllegalArgumentException( - sm.getString("webXml.mergeConflictOrder")); - } + if (fragment.getAfterOrdering().contains(ORDER_OTHERS)) { + makeAfterOthersExplicit(fragment.getBeforeOrdering(), fragments); } } - // Build the ordered list - for (String name : order) { - orderedFragments.add(fragments.get(name)); + // Stage 3. Separate into three groups + Set<WebXml> beforeSet = new HashSet<>(); + Set<WebXml> othersSet = new HashSet<>(); + Set<WebXml> afterSet = new HashSet<>(); + + for (WebXml fragment : fragments.values()) { + if (fragment.getBeforeOrdering().contains(ORDER_OTHERS)) { + beforeSet.add(fragment); + fragment.getBeforeOrdering().remove(ORDER_OTHERS); + } else if (fragment.getAfterOrdering().contains(ORDER_OTHERS)) { + afterSet.add(fragment); + fragment.getAfterOrdering().remove(ORDER_OTHERS); + } else { + othersSet.add(fragment); + } } + + // Stage 4. Decouple the groups so the ordering requirements for + // each fragment in the group only refer to other fragments + // in the group. Ordering requirements outside the group + // will be handled by processing the groups in order. + // Note: Only after ordering requirements are considered. + // This is OK because of the processing in stage 1. + decoupleOtherGroups(beforeSet); + decoupleOtherGroups(othersSet); + decoupleOtherGroups(afterSet); + + // Stage 5. Order each group + // Note: Only after ordering requirements are considered. + // This is OK because of the processing in stage 1. + orderFragments(orderedFragments, beforeSet); + orderFragments(orderedFragments, othersSet); + orderFragments(orderedFragments, afterSet); } return orderedFragments; } + private static void decoupleOtherGroups(Set<WebXml> group) { + Set<String> names = new HashSet<>(); + for (WebXml fragment : group) { + names.add(fragment.getName()); + } + for (WebXml fragment : group) { + Iterator<String> after = fragment.getAfterOrdering().iterator(); + while (after.hasNext()) { + String entry = after.next(); + if (!names.contains(entry)) { + after.remove(); + } + } + } + } + private static void orderFragments(Set<WebXml> orderedFragments, + Set<WebXml> unordered) { + Set<WebXml> addedThisRound = new HashSet<>(); + Set<WebXml> addedLastRound = new HashSet<>(); + while (unordered.size() > 0) { + Iterator<WebXml> source = unordered.iterator(); + while (source.hasNext()) { + WebXml fragment = source.next(); + for (WebXml toRemove : addedLastRound) { + fragment.getAfterOrdering().remove(toRemove.getName()); + } + if (fragment.getAfterOrdering().isEmpty()) { + addedThisRound.add(fragment); + orderedFragments.add(fragment); + source.remove(); + } + } + if (addedThisRound.size() == 0) { + // Circular + throw new IllegalArgumentException( + sm.getString("webXml.mergeConflictOrder")); + } + addedLastRound.clear(); + addedLastRound.addAll(addedThisRound); + addedThisRound.clear(); + } + } + + private static void makeBeforeOthersExplicit(Set<String> beforeOrdering, + Map<String, WebXml> fragments) { + for (String before : beforeOrdering) { + if (!before.equals(ORDER_OTHERS)) { + WebXml webXml = fragments.get(before); + if (!webXml.getBeforeOrdering().contains(ORDER_OTHERS)) { + webXml.addBeforeOrderingOthers(); + makeBeforeOthersExplicit(webXml.getAfterOrdering(), fragments); + } + } + } + } + + private static void makeAfterOthersExplicit(Set<String> afterOrdering, + Map<String, WebXml> fragments) { + for (String after : afterOrdering) { + if (!after.equals(ORDER_OTHERS)) { + WebXml webXml = fragments.get(after); + if (!webXml.getAfterOrdering().contains(ORDER_OTHERS)) { + webXml.addAfterOrderingOthers(); + makeAfterOthersExplicit(webXml.getBeforeOrdering(), fragments); + } + } + } + } } Modified: tomcat/trunk/test/org/apache/catalina/deploy/TestWebXmlOrdering.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/deploy/TestWebXmlOrdering.java?rev=1404374&r1=1404373&r2=1404374&view=diff ============================================================================== --- tomcat/trunk/test/org/apache/catalina/deploy/TestWebXmlOrdering.java (original) +++ tomcat/trunk/test/org/apache/catalina/deploy/TestWebXmlOrdering.java Wed Oct 31 21:57:26 2012 @@ -17,9 +17,11 @@ package org.apache.catalina.deploy; -import java.util.HashMap; +import java.util.ArrayList; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import java.util.Set; @@ -42,9 +44,15 @@ public class TestWebXmlOrdering { private WebXml e; private WebXml f; private Map<String,WebXml> fragments; + private int posA; + private int posB; + private int posC; + private int posD; + private int posE; + private int posF; @Before - public void setUp() throws Exception { + public void setUp() { app = new WebXml(); a = new WebXml(); a.setName("a"); @@ -58,7 +66,8 @@ public class TestWebXmlOrdering { e.setName("e"); f = new WebXml(); f.setName("f"); - fragments = new HashMap<>(); + // Control the input order + fragments = new LinkedHashMap<>(); fragments.put("a",a); fragments.put("b",b); fragments.put("c",c); @@ -185,83 +194,483 @@ public class TestWebXmlOrdering { assertFalse(iter.hasNext()); } + private void doRelativeOrderingTest(RelativeOrderingTestRunner runner) { + // Confirm we have all 720 possible input orders + // Set<String> orders = new HashSet<>(); + + // Test all possible input orders since some bugs were discovered that + // depended on input order + for (int i = 0; i < 6; i++) { + for (int j = 0; j < 5; j++) { + for (int k = 0; k < 4; k++) { + for (int l = 0; l < 3; l++) { + for (int m = 0; m < 2; m++) { + setUp(); + runner.init(); + ArrayList<WebXml> source = new ArrayList<>(); + source.addAll(fragments.values()); + Map<String,WebXml> input = + new LinkedHashMap<>(); + + WebXml one = source.remove(i); + input.put(one.getName(), one); + + WebXml two = source.remove(j); + input.put(two.getName(), two); + + WebXml three = source.remove(k); + input.put(three.getName(), three); + + WebXml four = source.remove(l); + input.put(four.getName(), four); + + WebXml five = source.remove(m); + input.put(five.getName(), five); + + WebXml six = source.remove(0); + input.put(six.getName(), six); + + /* + String order = one.getName() + two.getName() + + three.getName() + four.getName() + + five.getName() + six.getName(); + orders.add(order); + */ + + Set<WebXml> ordered = + WebXml.orderWebFragments(app, input); + populatePositions(ordered); + + runner.validate(getOrder(ordered)); + } + } + } + } + } + // System.out.println(orders.size()); + } + + private String getOrder(Set<WebXml> ordered) { + StringBuilder sb = new StringBuilder(ordered.size()); + for (WebXml webXml : ordered) { + sb.append(webXml.getName()); + } + return sb.toString(); + } + + private void populatePositions(Set<WebXml> ordered) { + List<WebXml> indexed = new ArrayList<>(); + indexed.addAll(ordered); + + posA = indexed.indexOf(a); + posB = indexed.indexOf(b); + posC = indexed.indexOf(c); + posD = indexed.indexOf(d); + posE = indexed.indexOf(e); + posF = indexed.indexOf(f); + } + @Test public void testOrderWebFragmentsRelative1() { // First example from servlet spec - a.addAfterOrderingOthers(); - a.addAfterOrdering("c"); - b.addBeforeOrderingOthers(); - c.addAfterOrderingOthers(); - f.addBeforeOrderingOthers(); - f.addBeforeOrdering("b"); - - Set<WebXml> ordered = WebXml.orderWebFragments(app, fragments); - - Iterator<WebXml> iter = ordered.iterator(); - assertEquals(f,iter.next()); - assertEquals(b,iter.next()); - assertEquals(d,iter.next()); - assertEquals(e,iter.next()); - assertEquals(c,iter.next()); - assertEquals(a,iter.next()); + doRelativeOrderingTest(new RelativeTestRunner1()); } @Test public void testOrderWebFragmentsRelative2() { // Second example - use fragment a for no-id fragment - a.addAfterOrderingOthers(); - a.addBeforeOrdering("c"); - b.addBeforeOrderingOthers(); - d.addAfterOrderingOthers(); - e.addBeforeOrderingOthers(); + doRelativeOrderingTest(new RelativeTestRunner2()); + } - Set<WebXml> ordered = WebXml.orderWebFragments(app, fragments); + @Test + public void testOrderWebFragmentsRelative3() { + // Third example from spec with e & f added + doRelativeOrderingTest(new RelativeTestRunner3()); + } - Iterator<WebXml> iter = ordered.iterator(); - // A number of orders are possible but the algorithm is deterministic - // and this order is valid. If this fails after a change to the - // algorithm, then check to see if the new order is also valid. - assertEquals(b,iter.next()); - assertEquals(e,iter.next()); - assertEquals(f,iter.next()); - assertEquals(a,iter.next()); - assertEquals(c,iter.next()); - assertEquals(d,iter.next()); + @Test + public void testOrderWebFragmentsRelative4Bug54068() { + // Simple sequence that failed for some inputs + doRelativeOrderingTest(new RelativeTestRunner4()); } @Test - public void testOrderWebFragmentsRelative3() { - // Third example from spec - a.addAfterOrdering("b"); - c.addBeforeOrderingOthers(); - fragments.remove("e"); - fragments.remove("f"); + public void testOrderWebFragmentsRelative5Bug54068() { + // Simple sequence that failed for some inputs + doRelativeOrderingTest(new RelativeTestRunner5()); + } - Set<WebXml> ordered = WebXml.orderWebFragments(app, fragments); + @Test + public void testOrderWebFragmentsRelative6Bug54068() { + // Simple sequence that failed for some inputs + doRelativeOrderingTest(new RelativeTestRunner6()); + } - Iterator<WebXml> iter = ordered.iterator(); - // A number of orders are possible but the algorithm is deterministic - // and this order is valid. If this fails after a change to the - // algorithm, then check to see if the new order is also valid. - assertEquals(c,iter.next()); - assertEquals(d,iter.next()); - assertEquals(b,iter.next()); - assertEquals(a,iter.next()); + @Test + public void testOrderWebFragmentsRelative7() { + // Reference loop (but not circular dependencies) + doRelativeOrderingTest(new RelativeTestRunner7()); } @Test - public void testOrderWebFragmentsrelativeCircular() { + public void testOrderWebFragmentsRelative8() { + // More complex, trying to break the algorithm + doRelativeOrderingTest(new RelativeTestRunner8()); + } + + @Test + public void testOrderWebFragmentsRelative9() { + // Variation on bug 54068 + doRelativeOrderingTest(new RelativeTestRunner9()); + } + + @Test + public void testOrderWebFragmentsRelative10() { + // Variation on bug 54068 + doRelativeOrderingTest(new RelativeTestRunner10()); + } + + + @Test(expected=IllegalArgumentException.class) + public void testOrderWebFragmentsrelativeCircular1() { a.addBeforeOrdering("b"); b.addBeforeOrdering("a"); - Exception exception = null; + WebXml.orderWebFragments(app, fragments); + } + + @Test(expected=IllegalArgumentException.class) + public void testOrderWebFragmentsrelativeCircular2() { + a.addBeforeOrderingOthers(); + b.addAfterOrderingOthers(); + c.addBeforeOrdering("a"); + c.addAfterOrdering("b"); + + WebXml.orderWebFragments(app, fragments); + } + + private interface RelativeOrderingTestRunner { + void init(); + void validate(String order); + } + + private class RelativeTestRunner1 implements RelativeOrderingTestRunner { + + @Override + public void init() { + a.addAfterOrderingOthers(); + a.addAfterOrdering("c"); + b.addBeforeOrderingOthers(); + c.addAfterOrderingOthers(); + f.addBeforeOrderingOthers(); + f.addBeforeOrdering("b"); + } + + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + //a.addAfterOrderingOthers(); + assertTrue(order, posA > posB); + assertTrue(order, posA > posC); + assertTrue(order, posA > posD); + assertTrue(order, posA > posE); + assertTrue(order, posA > posF); + + // a.addAfterOrdering("c"); + assertTrue(order, posA > posC); + + // b.addBeforeOrderingOthers(); + assertTrue(order, posB < posC); + + // c.addAfterOrderingOthers(); + assertTrue(order, posC > posB); + assertTrue(order, posC > posD); + assertTrue(order, posC > posE); + assertTrue(order, posC > posF); + + // f.addBeforeOrderingOthers(); + assertTrue(order, posF < posA); + assertTrue(order, posF < posB); + assertTrue(order, posF < posC); + assertTrue(order, posF < posD); + assertTrue(order, posF < posE); + + // f.addBeforeOrdering("b"); + assertTrue(order, posF < posB); + } + } + + private class RelativeTestRunner2 implements RelativeOrderingTestRunner { - try { - WebXml.orderWebFragments(app, fragments); - } catch (Exception e1) { - exception = e1; + @Override + public void init() { + a.addAfterOrderingOthers(); + a.addBeforeOrdering("c"); + b.addBeforeOrderingOthers(); + d.addAfterOrderingOthers(); + e.addBeforeOrderingOthers(); } - assertTrue(exception instanceof IllegalArgumentException); + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + // a.addAfterOrderingOthers(); + assertTrue(order, posA > posB); + assertTrue(order, posA > posE); + assertTrue(order, posA > posF); + + // a.addBeforeOrdering("c"); + assertTrue(order, posC > posA); + assertTrue(order, posC > posB); + assertTrue(order, posC > posE); + assertTrue(order, posC > posF); + + // b.addBeforeOrderingOthers(); + assertTrue(order, posB < posA); + assertTrue(order, posB < posC); + assertTrue(order, posB < posD); + assertTrue(order, posB < posF); + + // d.addAfterOrderingOthers(); + assertTrue(order, posD > posB); + assertTrue(order, posD > posE); + assertTrue(order, posD > posF); + + // e.addBeforeOrderingOthers(); + assertTrue(order, posE < posA); + assertTrue(order, posE < posC); + assertTrue(order, posE < posD); + assertTrue(order, posE < posF); + } + } + + private class RelativeTestRunner3 implements RelativeOrderingTestRunner { + + @Override + public void init() { + a.addAfterOrdering("b"); + c.addBeforeOrderingOthers(); + } + + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + // a.addAfterOrdering("b"); + assertTrue(order, posA > posB); + + // c.addBeforeOrderingOthers(); + assertTrue(order, posC < posA); + assertTrue(order, posC < posB); + assertTrue(order, posC < posD); + assertTrue(order, posC < posE); + assertTrue(order, posC < posF); + } + } + + private class RelativeTestRunner4 implements RelativeOrderingTestRunner { + + @Override + public void init() { + b.addAfterOrdering("a"); + c.addAfterOrdering("b"); + } + + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + // b.addAfterOrdering("a"); + assertTrue(order, posB > posA); + + // c.addAfterOrdering("b"); + assertTrue(order, posC > posB); + } + } + + private class RelativeTestRunner5 implements RelativeOrderingTestRunner { + + @Override + public void init() { + b.addBeforeOrdering("a"); + c.addBeforeOrdering("b"); + } + + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + // b.addBeforeOrdering("a"); + assertTrue(order, posB < posA); + + // c.addBeforeOrdering("b"); + assertTrue(order, posC < posB); + } + } + + private class RelativeTestRunner6 implements RelativeOrderingTestRunner { + + @Override + public void init() { + b.addBeforeOrdering("a"); + b.addAfterOrdering("c"); + } + + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + // b.addBeforeOrdering("a"); + assertTrue(order, posB < posA); + + //b.addAfterOrdering("c"); + assertTrue(order, posB > posC); + } + } + + private class RelativeTestRunner7 implements RelativeOrderingTestRunner { + + @Override + public void init() { + b.addBeforeOrdering("a"); + c.addBeforeOrdering("b"); + a.addAfterOrdering("c"); + } + + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + // b.addBeforeOrdering("a"); + assertTrue(order, posB < posA); + + // c.addBeforeOrdering("b"); + assertTrue(order, posC < posB); + + // a.addAfterOrdering("c"); + assertTrue(order, posA > posC); + } + } + + private class RelativeTestRunner8 implements RelativeOrderingTestRunner { + + @Override + public void init() { + a.addBeforeOrderingOthers(); + a.addBeforeOrdering("b"); + b.addBeforeOrderingOthers(); + c.addAfterOrdering("b"); + d.addAfterOrdering("c"); + e.addAfterOrderingOthers(); + f.addAfterOrderingOthers(); + f.addAfterOrdering("e"); + } + + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + // a.addBeforeOrderingOthers(); + assertTrue(order, posA < posB); + assertTrue(order, posA < posC); + assertTrue(order, posA < posD); + assertTrue(order, posA < posE); + assertTrue(order, posA < posF); + + // a.addBeforeOrdering("b"); + assertTrue(order, posA < posB); + + // b.addBeforeOrderingOthers(); + assertTrue(order, posB < posC); + assertTrue(order, posB < posD); + assertTrue(order, posB < posE); + assertTrue(order, posB < posF); + + // c.addAfterOrdering("b"); + assertTrue(order, posC > posB); + + // d.addAfterOrdering("c"); + assertTrue(order, posD > posC); + + // e.addAfterOrderingOthers(); + assertTrue(order, posE > posA); + assertTrue(order, posE > posB); + assertTrue(order, posE > posC); + assertTrue(order, posE > posD); + + // f.addAfterOrderingOthers(); + assertTrue(order, posF > posA); + assertTrue(order, posF > posB); + assertTrue(order, posF > posC); + assertTrue(order, posF > posD); + assertTrue(order, posF > posE); + + // f.addAfterOrdering("e"); + assertTrue(order, posF > posE); + } + } + + private class RelativeTestRunner9 implements RelativeOrderingTestRunner { + + @Override + public void init() { + a.addBeforeOrderingOthers(); + b.addBeforeOrdering("a"); + c.addBeforeOrdering("b"); + } + + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + // a.addBeforeOrderingOthers(); + assertTrue(order, posA < posD); + assertTrue(order, posA < posE); + assertTrue(order, posA < posF); + + // b.addBeforeOrdering("a"); + assertTrue(order, posB < posA); + + // c.addBeforeOrdering("b"); + assertTrue(order, posC < posB); + } + } + + private class RelativeTestRunner10 implements RelativeOrderingTestRunner { + + @Override + public void init() { + a.addAfterOrderingOthers(); + b.addAfterOrdering("a"); + c.addAfterOrdering("b"); + } + + @Override + public void validate(String order) { + // There is some duplication in the tests below - it is easier to + // check the tests are complete this way. + + // a.addAfterOrderingOthers(); + assertTrue(order, posA > posD); + assertTrue(order, posA > posE); + assertTrue(order, posA > posF); + + // b.addAfterOrdering("a"); + assertTrue(order, posB > posA); + + // c.addAfterOrdering("b"); + assertTrue(order, posC > posB); + } } } --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org