XalanJ2
  1. XalanJ2
  2. XALANJ-1886

Multiple problems with collecting xsl:attributes with over-ridden values and prefix/URI maps

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.7.1
    • Fix Version/s: 2.7
    • Component/s: Serialization
    • Security Level: No security risk; visible to anyone (Ordinary problems in Xalan projects. Anybody can view the issue.)
    • Labels:
      None
    • Environment:
      Operating System: Other
      Platform: Other
    • Xalan info:
      PatchAvailable
    • Fix priority:
      fp1

      Description

      The serializer has a problem that it identifies attributes by their qname
      (prefix:localName) note by their expanded qname (URI and localName). It also
      sometimes replaces existing namespace mappings while processing xsl:attribute
      elements.

      Other bugs are already opened for this, namely bug 23897 and bug 23955. This
      needs a bit of a design change because currently there is no way to tell the
      serializer that attributes are from an xsl:attribute or from those in the
      opening element tag can not be distinguished. See the failing examples below:

      <elem3>
      <xsl:attribute name="pre1:nattr" xmlns:pre1="uri1">value1</xsl:attribute>
      <xsl:attribute name="pre1:nattr" xmlns:pre1="uri2">value1</xsl:attribute>
      </elem3>
      is serialized as:
      <elem3 xmlns:pre1="uri2" pre1:nattr="value1"/>
      this is wrong because the URI's of the attributes are different ("uri1"
      and "uri2") but the serializer identifies attributes by prefix:localName and
      both xsl:attribute elements seem to be setting the same attribute (wrong!).

      This:
      <elem4>
      <xsl:attribute name="pre1:nattr" xmlns:pre1="uri1">value1</xsl:attribute>
      <xsl:attribute name="pre2:nattr" xmlns:pre2="uri1">value1</xsl:attribute>
      </elem4>
      is serialized as:
      <elem4 xmlns:pre1="uri1" pre1:nattr="value1"
      xmlns:pre2="uri1" pre2:nattr="value1"/>

      this is wrong because although the prefices differ the URI's are the same so
      there should only be one attribute when serialized.

      <elem5>
      <xsl:attribute name="pre1:nattr" xmlns:pre1="uri1">value1</xsl:attribute>
      <xsl:attribute name="pre1:nattr" xmlns:pre1="uri2">value2</xsl:attribute>
      </elem5>
      is serialized as:
      <elem5 xmlns:pre1="uri2" pre1:nattr="value2"/>
      this is wrong because there should be two attributes when serialized (different
      URIs)

      <elem9 pre1:otherattr="other" pre1:nattr="value1" xmlns:pre1="uri1">
      <xsl:attribute name="pre1:nattr" xmlns:pre1="uri2">value2</xsl:attribute>
      </elem9>
      is serialized as:
      <elem9 xmlns:pre1="uri2" pre1:nattr="value2" pre1:otherattr="other"/>
      this is wrong because the URI associated with "otherattr" is no longer "uri1"
      as it should be.

      <elem11 pre1:nattr="value1" xmlns:pre1="uri1">
      <xsl:attribute name="pre2:nattr" xmlns:pre2="uri1">value2</xsl:attribute>
      </elem11>
      is serialized as:
      <elem11 xmlns:pre1="uri1" pre1:nattr="value1"
      xmlns:pre2="uri1" pre2:nattr="value2">
      this is wrong because both attributes have the same URI ("uri1") and localName
      so there should only be one attribute when serialized.

      Conclusion: There are multiple problems with processing xsl:attribute elements.
      Part of the problem is in the serializer, part of the problem is that there is
      no internal API to tell the serializer that the attributes (names, values,
      prefix/uri mapping is due to an xsl:attribute). Some changes will be needed in
      XSLTC too.

      1. serializer.patch5.txt
        3 kB
        Brian Minchau
      2. patch4.txt
        40 kB
        Brian Minchau
      3. ASF.LICENSE.NOT.GRANTED--patch2.txt
        40 kB
        Brian Minchau

        Activity

        Hide
        Brian Minchau added a comment -

        serializer.patch5 is applied to Apache CVS HEAD branch.

        Show
        Brian Minchau added a comment - serializer.patch5 is applied to Apache CVS HEAD branch.
        Hide
        Henry Zongaro added a comment -

        I have reviewed Brian's "serializer.patch5" patch, and think it is correct.

        Show
        Henry Zongaro added a comment - I have reviewed Brian's "serializer.patch5" patch, and think it is correct.
        Hide
        Brian Minchau added a comment -

        Attaching serializer.patch5.txt which is a patch that puts back the old:
        addAttribute(uri,localname,qname,type,value);
        into the ExtendedContentHandler interface. This method was removed by the
        previous patch.
        The two implementations of this method are in EmptySerializer, where the body is essentially empty, and in SerialzerBase where it delegates, but with a flag set to false indicating that it is not coming from an xsl:attribute child element.

        Show
        Brian Minchau added a comment - Attaching serializer.patch5.txt which is a patch that puts back the old: addAttribute(uri,localname,qname,type,value); into the ExtendedContentHandler interface. This method was removed by the previous patch. The two implementations of this method are in EmptySerializer, where the body is essentially empty, and in SerialzerBase where it delegates, but with a flag set to false indicating that it is not coming from an xsl:attribute child element.
        Hide
        Brian Minchau added a comment -

        There is nothing wrong with the patch, but
        since ExtendedContentHandler added a parameter to
        an addAttribute() method, it essentially deleted
        an old method on the interface.

        This can cause some migration problems, so the method needs to be added. The patch is already committed, so I'll add a patch to current CVS HEAD.

        Show
        Brian Minchau added a comment - There is nothing wrong with the patch, but since ExtendedContentHandler added a parameter to an addAttribute() method, it essentially deleted an old method on the interface. This can cause some migration problems, so the method needs to be added. The patch is already committed, so I'll add a patch to current CVS HEAD.
        Hide
        Brian Minchau added a comment -

        Patch applied to Apache CVS HEAD branch.

        Show
        Brian Minchau added a comment - Patch applied to Apache CVS HEAD branch.
        Hide
        Brian Minchau added a comment -

        Attaching patch4.txt in which I'm Granting license to ASF. I wrote the original patch, so I can do this.
        Also the code has evolved a bit since the patch2.txt was attached, so not all of its changes "stuck" anymore. Some manual intervention was needed to get a clean patch.

        Show
        Brian Minchau added a comment - Attaching patch4.txt in which I'm Granting license to ASF. I wrote the original patch, so I can do this. Also the code has evolved a bit since the patch2.txt was attached, so not all of its changes "stuck" anymore. Some manual intervention was needed to get a clean patch.
        Hide
        Brian Minchau added a comment -

        Morris Kwan has reviewed my patch with the comment:
        "I have reviewed your patch. It looks good.".
        The patch is ready to be committed.

        Show
        Brian Minchau added a comment - Morris Kwan has reviewed my patch with the comment: "I have reviewed your patch. It looks good.". The patch is ready to be committed.
        Hide
        Brian Minchau added a comment -
            • Bug 23955 has been marked as a duplicate of this bug. ***
        Show
        Brian Minchau added a comment - Bug 23955 has been marked as a duplicate of this bug. ***
        Hide
        Brian Minchau added a comment -
            • Bug 23897 has been marked as a duplicate of this bug. ***
        Show
        Brian Minchau added a comment - Bug 23897 has been marked as a duplicate of this bug. ***
        Hide
        Brian Minchau added a comment -

        I've just attached a rather extensive patch which essentially does the
        following:
        1) Additional flag when adding attributes to indicate if they are from an
        xsl:attribute or elsewhere
        2) Added support in NamespaceMappings to correctly find a mapping using the uri
        and localName
        3) Fixed bugs where attributes were created with the wrong URI, but it didn't
        show up before because attributes were not found using the URI
        4) Additional code to correctly merge xsl:attributes with other attributes,
        with attention to prefix/uri/value.

        The changes to NamespaceMappings were done with performance in mind, but there
        is a chance of some degredation because of the additional functionality
        (finding prefix/uri mappings by uri/localname rather than the
        prefix/localname). Some fields and methods in NamespaceMappings are
        made "final" in the patch in an attempt to mitigate any performance degredation.

        • Brian Minchau
        Show
        Brian Minchau added a comment - I've just attached a rather extensive patch which essentially does the following: 1) Additional flag when adding attributes to indicate if they are from an xsl:attribute or elsewhere 2) Added support in NamespaceMappings to correctly find a mapping using the uri and localName 3) Fixed bugs where attributes were created with the wrong URI, but it didn't show up before because attributes were not found using the URI 4) Additional code to correctly merge xsl:attributes with other attributes, with attention to prefix/uri/value. The changes to NamespaceMappings were done with performance in mind, but there is a chance of some degredation because of the additional functionality (finding prefix/uri mappings by uri/localname rather than the prefix/localname). Some fields and methods in NamespaceMappings are made "final" in the patch in an attempt to mitigate any performance degredation. Brian Minchau
        Hide
        Brian Minchau added a comment -

        Created an attachment (id=11823)
        Patch to Xalan-J interpretive and serializer

        Show
        Brian Minchau added a comment - Created an attachment (id=11823) Patch to Xalan-J interpretive and serializer

          People

          • Assignee:
            Brian Minchau
            Reporter:
            Brian Minchau
            Reviewer:
            Morris Kwan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development