Commons Digester
  1. Commons Digester
  2. DIGESTER-44

[digester] [PATCH] NodeCreateRule does not correctly handle namespaced attributes

    Details

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

      Operating System: All
      Platform: All

      Description

      Attributes within namespaces do not have their prefix preserved within the
      NodeCreateRule when Digester is namespace aware. This is because
      element.setAttributeNS is called with the incorrect parameters. Fixed by
      passing in URI, QualifiedName and Value instead of URI, LocalName and Value.

      -Changed NodeCreateRule.begin to call setAttributeNS with qualified name
      -added junit test NodeCreateRuleTestCase.testNamespacedAttribute
      -added test xml file: Test10.xml

      Attachment patch in cvs universal diff format.

        Activity

        Hide
        Kurt Zettel II added a comment -

        Created an attachment (id=16490)
        patch in diff -u format

        Show
        Kurt Zettel II added a comment - Created an attachment (id=16490) patch in diff -u format
        Hide
        Simon Kitching added a comment -

        Looks correct to me.

        The only potential issue is that this will change the existing behaviour of
        digester. Formerly, calling getName on the attribute would return the localname
        part. After this fix it would return the qualified name, which is correct. But
        this could break existing applications.

        Does anyone think this breakage is a significant problem? If so, we could add
        some kind of flag on the Digester class to select the old (wrong) or new (right)
        behaviour, at least for a release or two, eg:
        Digester.setRetainAttributePrefixes(boolean)
        which would default to false (the old behaviour). I personally think this would
        be wise; digester usage is fairly widely spread so incompatible changes can
        cause significant grief.

        Regards,

        Simon

        Show
        Simon Kitching added a comment - Looks correct to me. The only potential issue is that this will change the existing behaviour of digester. Formerly, calling getName on the attribute would return the localname part. After this fix it would return the qualified name, which is correct. But this could break existing applications. Does anyone think this breakage is a significant problem? If so, we could add some kind of flag on the Digester class to select the old (wrong) or new (right) behaviour, at least for a release or two, eg: Digester.setRetainAttributePrefixes(boolean) which would default to false (the old behaviour). I personally think this would be wise; digester usage is fairly widely spread so incompatible changes can cause significant grief. Regards, Simon
        Hide
        Kurt Zettel II added a comment -

        I disagree. Although digester usage is widespread, I doubt namespaced
        attributes being digested is used at all since it doesn't work. If anyone was
        trying to use namespaced attributes with the digester in the past, they would
        have abandoned the cause. I can't imagine a scenario where someone would have
        just accepted the behavior as "good enough" and continued to use namespaced
        attributes even though they get lost as they are digested and can't be
        re-serialized as XML. Since that would have caused everyone to use
        non-namespaced attributes, they won't notice a difference because local-name and
        qualified-name are the same in that case.

        Thanks,

        Kurt

        (In reply to comment #2)
        > Looks correct to me.
        >
        > The only potential issue is that this will change the existing behaviour of
        > digester. Formerly, calling getName on the attribute would return the localname
        > part. After this fix it would return the qualified name, which is correct. But
        > this could break existing applications.
        >
        > Does anyone think this breakage is a significant problem? If so, we could add
        > some kind of flag on the Digester class to select the old (wrong) or new (right)
        > behaviour, at least for a release or two, eg:
        > Digester.setRetainAttributePrefixes(boolean)
        > which would default to false (the old behaviour). I personally think this would
        > be wise; digester usage is fairly widely spread so incompatible changes can
        > cause significant grief.
        >
        > Regards,
        >
        > Simon

        Show
        Kurt Zettel II added a comment - I disagree. Although digester usage is widespread, I doubt namespaced attributes being digested is used at all since it doesn't work. If anyone was trying to use namespaced attributes with the digester in the past, they would have abandoned the cause. I can't imagine a scenario where someone would have just accepted the behavior as "good enough" and continued to use namespaced attributes even though they get lost as they are digested and can't be re-serialized as XML. Since that would have caused everyone to use non-namespaced attributes, they won't notice a difference because local-name and qualified-name are the same in that case. Thanks, Kurt (In reply to comment #2) > Looks correct to me. > > The only potential issue is that this will change the existing behaviour of > digester. Formerly, calling getName on the attribute would return the localname > part. After this fix it would return the qualified name, which is correct. But > this could break existing applications. > > Does anyone think this breakage is a significant problem? If so, we could add > some kind of flag on the Digester class to select the old (wrong) or new (right) > behaviour, at least for a release or two, eg: > Digester.setRetainAttributePrefixes(boolean) > which would default to false (the old behaviour). I personally think this would > be wise; digester usage is fairly widely spread so incompatible changes can > cause significant grief. > > Regards, > > Simon
        Hide
        rdonkin@apache.org added a comment -

        Hi Kurt

        Please understand's not quite as simple as that.

        1. Digester is used extensively where loosy binding is perfectly accceptable.
        2. Digester is old and flexible enough to have to worry about maintaining
        compatibility with parsers which are not namespace aware.

        However, in this case I've taken a look at the DOM specification and believe
        that the passage of code in question is just buggy. DOM expects the qualified
        (and not the local) name. I suspect that this is so that it can extract the
        prefix for it's own uses. My reading of the API implies that the Element parses
        the qualified name to extract the local name itself. Therefore, applying this
        patch should not change the existing behaviour and so will do no harm.

        Probably worth creating a unit test for the case where there is no namespace,
        though.

        Opinions?

        Show
        rdonkin@apache.org added a comment - Hi Kurt Please understand's not quite as simple as that. 1. Digester is used extensively where loosy binding is perfectly accceptable. 2. Digester is old and flexible enough to have to worry about maintaining compatibility with parsers which are not namespace aware. However, in this case I've taken a look at the DOM specification and believe that the passage of code in question is just buggy. DOM expects the qualified (and not the local) name. I suspect that this is so that it can extract the prefix for it's own uses. My reading of the API implies that the Element parses the qualified name to extract the local name itself. Therefore, applying this patch should not change the existing behaviour and so will do no harm. Probably worth creating a unit test for the case where there is no namespace, though. Opinions?
        Hide
        Kevin Ross added a comment -

        The fact that the digester allows for loose interpretation of the standard does
        support:
        1. old parsers
        2. developers lacking knowledge about proper use of namespaces.

        If we want to maintain backward support for incorrect behavior, then that is
        fine, make it the exception, not the rule. Incorrect behavior SHOULD break.
        Don't discourage forward progress and use of the digester for modern purposes
        because people want to use what is actually buggy software in terms of the new
        specs or modern parsers. This leads to nothing but confusion, and now stands as
        a barrier to adoption for those who want to start using it but cannot get it to
        work.

        I understand how we got here, and thats ok. We have to move forward. I believe
        the solution is:

        1. commit the code, it is correct
        2. allow the code to be incompatible with previous versions. For those who want
        to stay at a historical point in time, the maven repository has all releases

        There are even more changes that I've run across that I've been hesitant to
        change, and it is to the point that either the digester fully understands
        namespaces or I need to find a new project that will do the same thing. I just
        need something that exhibits behavior that is concurrent with the standards and
        understand namespaces.

        -Kevin

        (In reply to comment #4)
        > Hi Kurt
        >
        > Please understand's not quite as simple as that.
        >
        > 1. Digester is used extensively where loosy binding is perfectly accceptable.
        > 2. Digester is old and flexible enough to have to worry about maintaining
        > compatibility with parsers which are not namespace aware.
        >
        > However, in this case I've taken a look at the DOM specification and believe
        > that the passage of code in question is just buggy. DOM expects the qualified
        > (and not the local) name. I suspect that this is so that it can extract the
        > prefix for it's own uses. My reading of the API implies that the Element parses
        > the qualified name to extract the local name itself. Therefore, applying this
        > patch should not change the existing behaviour and so will do no harm.
        >
        > Probably worth creating a unit test for the case where there is no namespace,
        > though.
        >
        > Opinions?

        Show
        Kevin Ross added a comment - The fact that the digester allows for loose interpretation of the standard does support: 1. old parsers 2. developers lacking knowledge about proper use of namespaces. If we want to maintain backward support for incorrect behavior, then that is fine, make it the exception, not the rule. Incorrect behavior SHOULD break. Don't discourage forward progress and use of the digester for modern purposes because people want to use what is actually buggy software in terms of the new specs or modern parsers. This leads to nothing but confusion, and now stands as a barrier to adoption for those who want to start using it but cannot get it to work. I understand how we got here, and thats ok. We have to move forward. I believe the solution is: 1. commit the code, it is correct 2. allow the code to be incompatible with previous versions. For those who want to stay at a historical point in time, the maven repository has all releases There are even more changes that I've run across that I've been hesitant to change, and it is to the point that either the digester fully understands namespaces or I need to find a new project that will do the same thing. I just need something that exhibits behavior that is concurrent with the standards and understand namespaces. -Kevin (In reply to comment #4) > Hi Kurt > > Please understand's not quite as simple as that. > > 1. Digester is used extensively where loosy binding is perfectly accceptable. > 2. Digester is old and flexible enough to have to worry about maintaining > compatibility with parsers which are not namespace aware. > > However, in this case I've taken a look at the DOM specification and believe > that the passage of code in question is just buggy. DOM expects the qualified > (and not the local) name. I suspect that this is so that it can extract the > prefix for it's own uses. My reading of the API implies that the Element parses > the qualified name to extract the local name itself. Therefore, applying this > patch should not change the existing behaviour and so will do no harm. > > Probably worth creating a unit test for the case where there is no namespace, > though. > > Opinions?
        Hide
        Simon Kitching added a comment -

        You are quite right that Digester's general handling of namespaces is very poor.
        This is acknowledged in the package.html file for digester. However there is no
        way to fix this across the whole app without major binary incompatibilities, and
        we just can't break that many users.

        You can find the "digester2" project in subversion:
        http://svn.apache.org/repos/asf/jakarta/commons/proper/digester/branches/digester2/

        Because this uses the package name "org.apache.commons.digester2", it can
        happily co-exist with digester1. It is intended to fix many issues with the
        digester1 design, including the namespaces one.

        However I don't currently have much time to work on digester2, and there isn't
        anyone else actively moving it forward. So if you need robust namespace support
        right now, unfortunately you will need to use something other than Digester.

        The specific namespace problem you originally raised in this bugrequest is
        somehting we should think about, as Robert indicated. Hopefully Robert or I will
        get around to this soon, but unfortunately we're both very busy people right now
        and as noted this is a little tricky from a backwards-compatibility viewpoint .

        Show
        Simon Kitching added a comment - You are quite right that Digester's general handling of namespaces is very poor. This is acknowledged in the package.html file for digester. However there is no way to fix this across the whole app without major binary incompatibilities, and we just can't break that many users. You can find the "digester2" project in subversion: http://svn.apache.org/repos/asf/jakarta/commons/proper/digester/branches/digester2/ Because this uses the package name "org.apache.commons.digester2", it can happily co-exist with digester1. It is intended to fix many issues with the digester1 design, including the namespaces one. However I don't currently have much time to work on digester2, and there isn't anyone else actively moving it forward. So if you need robust namespace support right now, unfortunately you will need to use something other than Digester. The specific namespace problem you originally raised in this bugrequest is somehting we should think about, as Robert indicated. Hopefully Robert or I will get around to this soon, but unfortunately we're both very busy people right now and as noted this is a little tricky from a backwards-compatibility viewpoint .
        Hide
        Kevin Ross added a comment -

        Ok, who created digester2 it and what state is it in? How can you add me to
        commit for that branch? My apache username is kevinross.

        I just determined that my CLA is not on file so it will be a few days before I
        can begin contributing directly to svn.

        Please let me know, I have several projects that require this functionality that
        are active. I have a team of four working an them, so I feel confident that
        there will be several contributions and I can screen them.

        Thanks,

        Kevin

        Show
        Kevin Ross added a comment - Ok, who created digester2 it and what state is it in? How can you add me to commit for that branch? My apache username is kevinross. I just determined that my CLA is not on file so it will be a few days before I can begin contributing directly to svn. Please let me know, I have several projects that require this functionality that are active. I have a team of four working an them, so I feel confident that there will be several contributions and I can screen them. Thanks, Kevin
        Hide
        Simon Kitching added a comment -

        Digester2 is mostly my work, with some input from others. I'm one of the main
        maintainers of digester1 (along with Robert Donkin). Of about a dozen
        significant changes from digester1 planned, approximately half have been done.
        There's still quite a bit of work to do, though, before it's a functional
        product. If you are interested please start an email thread on the commons-dev
        email list rather than commenting here as this bugreport is not the right venue
        for that discussion.

        Show
        Simon Kitching added a comment - Digester2 is mostly my work, with some input from others. I'm one of the main maintainers of digester1 (along with Robert Donkin). Of about a dozen significant changes from digester1 planned, approximately half have been done. There's still quite a bit of work to do, though, before it's a functional product. If you are interested please start an email thread on the commons-dev email list rather than commenting here as this bugreport is not the right venue for that discussion.
        Hide
        Kurt Zettel II added a comment -

        (In reply to comment #4)
        > Probably worth creating a unit test for the case where there is no namespace,
        > though.

        I took the liberty of creating that test case. I ran it against the previous
        version of the code and the code with my updates. The non-namespaced attribute
        test works with both, proving that the current functionality of handling
        non-namespaced attributes has not changed.

        Let me know if I can do anything else to help get this change commited.

        Thanks,

        Kurt

        Show
        Kurt Zettel II added a comment - (In reply to comment #4) > Probably worth creating a unit test for the case where there is no namespace, > though. I took the liberty of creating that test case. I ran it against the previous version of the code and the code with my updates. The non-namespaced attribute test works with both, proving that the current functionality of handling non-namespaced attributes has not changed. Let me know if I can do anything else to help get this change commited. Thanks, Kurt
        Hide
        Kurt Zettel II added a comment -

        Created an attachment (id=16790)
        Updated Patch with additonal Test Case (Replaces the original patch)

        Show
        Kurt Zettel II added a comment - Created an attachment (id=16790) Updated Patch with additonal Test Case (Replaces the original patch)
        Hide
        Kurt Zettel II added a comment -

        About three months ago I created this patch and a test case. Please let me know
        the status of this bug.

        Thank you,

        Kurt.

        Show
        Kurt Zettel II added a comment - About three months ago I created this patch and a test case. Please let me know the status of this bug. Thank you, Kurt.
        Hide
        Simon Kitching added a comment -

        Hi Kurt,

        Sorry this issue has been open so long. I'll try to get around to addressing
        this in the next couple of days.

        Show
        Simon Kitching added a comment - Hi Kurt, Sorry this issue has been open so long. I'll try to get around to addressing this in the next couple of days.
        Hide
        Simon Kitching added a comment -

        Hi Kurt,

        After further thought, I think you're right. The old behaviour is just a bug, so
        a change in behaviour is ok, not an "incompatibility". I've committed your patch.
        Thanks very much for your contribution - and your patience!

        Show
        Simon Kitching added a comment - Hi Kurt, After further thought, I think you're right. The old behaviour is just a bug, so a change in behaviour is ok, not an "incompatibility". I've committed your patch. Thanks very much for your contribution - and your patience!
        Hide
        Henri Yandell added a comment -

        (Fixing JIRA import error)

        Show
        Henri Yandell added a comment - (Fixing JIRA import error)

          People

          • Assignee:
            Unassigned
            Reporter:
            Kurt Zettel II
          • Votes:
            3 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development