Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: All

      Description

      When using the JDK1.2, "ant test.matching.extended" reports a failure in method
      "testAncesterMatch".

      I have added logging to the ExtendedBaseRules class, and the problem appears not
      be with jdk1.2, but rather with the order objects are returned from the HashMap
      rule cache. It seems that iterating over the entryset of elements in the cache
      returns the elements in different order in the two JVMs, and somehow this seems
      to break ExtendedBaseRules. It shouldn't, of course.

      I'll continue to look into it, but would appreciate some help from those who are
      familiar with this class, as the matching rules are very complex.

        Activity

        Simon Kitching created issue -
        Hide
        Simon Kitching added a comment -

        Ok, I think I've got it pinned down.

        The test itself was a very good one. It asked:
        given the pattern "a/b/c/d"
        is the preferred match:
        */b/c/d
        or /b/c/

        The specs for the ExtendedBaseRules class say that:
        [quote]
        /a/b/" matches any elements whose parentage path contains an element 'a'
        followed by an element 'b'. The longest matching still applies but the length
        excludes the '*' at the end.
        [/quote]

        The code currently tries to handle this by subtracting 1 from *new candidate
        keys* when the length if (parentMatched || ancestorMatched). However:
        (a) it should subtract 2 for patterns that both begin and end with wildcards, and
        (b) it doesn't apply any corrections to the "previously longest key" it is
        comparing against.

        And when 2 patterns of the same "length" exist in the cache, and they both
        match, only the first matching pattern is kept. So the current code passes the
        test only if the cache returns the "correct" match first. Which it appears to do
        with jvm 1.4 but not 1.2!

        I will attach a patch soon. I would appreciate review of this, as it has the
        potential to break a lot of stuff if it is wrong.

        Show
        Simon Kitching added a comment - Ok, I think I've got it pinned down. The test itself was a very good one. It asked: given the pattern "a/b/c/d" is the preferred match: */b/c/d or /b/c/ The specs for the ExtendedBaseRules class say that: [quote] /a/b/ " matches any elements whose parentage path contains an element 'a' followed by an element 'b'. The longest matching still applies but the length excludes the '*' at the end. [/quote] The code currently tries to handle this by subtracting 1 from *new candidate keys* when the length if (parentMatched || ancestorMatched). However: (a) it should subtract 2 for patterns that both begin and end with wildcards, and (b) it doesn't apply any corrections to the "previously longest key" it is comparing against. And when 2 patterns of the same "length" exist in the cache, and they both match, only the first matching pattern is kept. So the current code passes the test only if the cache returns the "correct" match first. Which it appears to do with jvm 1.4 but not 1.2! I will attach a patch soon. I would appreciate review of this, as it has the potential to break a lot of stuff if it is wrong.
        Hide
        Simon Kitching added a comment -

        Created an attachment (id=11287)
        patch for bug

        Show
        Simon Kitching added a comment - Created an attachment (id=11287) patch for bug
        Hide
        rdonkin@apache.org added a comment -

        hi

        sorry that i've been a bit busy with other stuff recently but i'll try to find some time tomorrow to take a
        look at this.

        Show
        rdonkin@apache.org added a comment - hi sorry that i've been a bit busy with other stuff recently but i'll try to find some time tomorrow to take a look at this.
        Hide
        rdonkin@apache.org added a comment -

        i've taken a look at this but i really need to finish off configuring my (recently re-installed) linux box so
        i can run the test againts jdk1.2 and see the behaviour for myself.

        i wonder whether it's actually an ordering problem rather than simply a code problem (but i need to get
        a version running on the older jdk first).

        i've taken a look at trying to write some unit tests that fail in my usual JRE but i haven't hit on one yet.
        the patch looks ok(ish) but i'd be much happier if i had a clear unit test that demonstrated the bug
        (which should be replicatable if it's fixed by the patch) before applying it.

        i'll continue looking for a unit test tomorrow unless you beat me to it

        Show
        rdonkin@apache.org added a comment - i've taken a look at this but i really need to finish off configuring my (recently re-installed) linux box so i can run the test againts jdk1.2 and see the behaviour for myself. i wonder whether it's actually an ordering problem rather than simply a code problem (but i need to get a version running on the older jdk first). i've taken a look at trying to write some unit tests that fail in my usual JRE but i haven't hit on one yet. the patch looks ok(ish) but i'd be much happier if i had a clear unit test that demonstrated the bug (which should be replicatable if it's fixed by the patch) before applying it. i'll continue looking for a unit test tomorrow unless you beat me to it
        Hide
        Simon Kitching added a comment -

        It's going to be a tricky one to duplicate with a unit test (which is why I
        didn't create one .

        I can definitely say that exactly the same code passes with jdk1.4 and fails
        with jdk1.2 (returns different sets of matches). I can't see any other
        reasonable cause except the order rules are returned by map.entrySet().

        I'm not sure what you mean by "actually an ordering problem rather than simply a
        code problem". I think it's both: a bug in the code causes two rules which
        should have different "priorities" to actually have the same "priority". And in
        the case of a tie, the first rule found wins. So if the correct rule is
        encountered first, the test works and if the wrong rule is encountered first,
        the test fails. And which is encountered first depends upon the internal
        implementation of the HashMap class (or the String hashcode method), which is
        jdk-specific.

        A unit test for this would presumably involve subclassing the ExtendedBaseRules
        class and using a custom map class, such as one of the OrderedMap classes from
        collections. By then adding the rules to the digester in different orders, the
        order they are returned from the map.entrySet could be controlled. That all
        sounds like a lot of work, unfortunately!

        I might have a go at this over the weekend if the rain continues....unless you
        beat me to it

        Show
        Simon Kitching added a comment - It's going to be a tricky one to duplicate with a unit test (which is why I didn't create one . I can definitely say that exactly the same code passes with jdk1.4 and fails with jdk1.2 (returns different sets of matches). I can't see any other reasonable cause except the order rules are returned by map.entrySet(). I'm not sure what you mean by "actually an ordering problem rather than simply a code problem". I think it's both: a bug in the code causes two rules which should have different "priorities" to actually have the same "priority". And in the case of a tie, the first rule found wins. So if the correct rule is encountered first, the test works and if the wrong rule is encountered first, the test fails. And which is encountered first depends upon the internal implementation of the HashMap class (or the String hashcode method), which is jdk-specific. A unit test for this would presumably involve subclassing the ExtendedBaseRules class and using a custom map class, such as one of the OrderedMap classes from collections. By then adding the rules to the digester in different orders, the order they are returned from the map.entrySet could be controlled. That all sounds like a lot of work, unfortunately! I might have a go at this over the weekend if the rain continues....unless you beat me to it
        Hide
        rdonkin@apache.org added a comment -

        i've taken a look and i now agree with you about the fix and the basic cause: it's a simple bug which can
        be fixed just by assigning the key length to a variable. i'd be happy to see your patch committed.

        i also agree with you that the peculiar ordering of the entries in a post-1.2 map appears to mask this
        problem. i'm no longer sure that it'll be easy to create a simple unit test for it.

        the method could be improved by preforming each startsWith and endsWith just once and assigning to
        a boolean variable. this should make things a little more readible as well as improve performance a
        little.

        Show
        rdonkin@apache.org added a comment - i've taken a look and i now agree with you about the fix and the basic cause: it's a simple bug which can be fixed just by assigning the key length to a variable. i'd be happy to see your patch committed. i also agree with you that the peculiar ordering of the entries in a post-1.2 map appears to mask this problem. i'm no longer sure that it'll be easy to create a simple unit test for it. the method could be improved by preforming each startsWith and endsWith just once and assigning to a boolean variable. this should make things a little more readible as well as improve performance a little.
        Hide
        Simon Kitching added a comment -

        Patch committed. Also used booleans to hold startsWith and endsWith results, as
        suggested.

        Show
        Simon Kitching added a comment - Patch committed. Also used booleans to hold startsWith and endsWith results, as suggested.
        Henri Yandell made changes -
        Field Original Value New Value
        issue.field.bugzillaimportkey 28486 12341413
        Henri Yandell made changes -
        Affects Version/s 1.5 Final [ 12311664 ]
        Assignee Jakarta Commons Developers Mailing List [ commons-dev@jakarta.apache.org ]
        Component/s Digester [ 12311111 ]
        Project Commons [ 12310458 ] Commons Digester [ 12310471 ]
        Key COM-1261 DIGESTER-18
        Henri Yandell made changes -
        Affects Version/s 1.5 Final [ 12311691 ]
        Henri Yandell made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Simon Kitching
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development