Project

Profile

Help

Bug #5389

closed

Test case attribute-1301 fails

Added by Michael Kay over 2 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Low
Assignee:
Category:
XSLT Conformance
Sprint/Milestone:
Start date:
2022-03-13
Due date:
% Done:

100%

Estimated time:
Applies to JS Branch:
2, Trunk
Fix Committed on JS Branch:
2, Trunk
Fixed in JS Release:
SEF Generated with:
Platforms:
Company:
-
Contact person:
-
Additional contact persons:
-

Description

This test is doing something pretty weird with namespaces and gives the wrong results for SaxonJS, with either the XJ or XX compilers. I don't think it's a new problem, just one that we didn't consider important to fix. Adding it to the exceptions list for now.

Actions #1

Updated by Michael Kay over 2 years ago

Let's look at the test. It's doing

<xsl:template match="/">
  <xsl:variable name="temp">
  <out>
    <jam>
      <xsl:attribute name="{docs/b}" namespace="http://xyz.com">jaminben</xsl:attribute>
    </jam>
  </out>
  </xsl:variable>
  <ans bdd="{namespace-uri-for-prefix('bdd', $temp/out/jam)}" attr-ns="{namespace-uri($temp/out/jam/@*:attr)}"/>
</xsl:template>

The source document is

<docs><b>bdd:attr</b></docs>

The expected output is:

<ans xmlns:bdd="http://bdd.test.com" bdd="http://bdd.test.com" attr-ns="http://xyz.com"/>

but it's failing:

Transformation failure: Error FORG0001 at attribute-1301.xsl#13
    Invalid local name: '' (prefix='ns0', uri='http://xyz.com')

Actions #2

Updated by Michael Kay over 2 years ago

The rules for literal result elements say, firstly, that within the temporary tree $temp, the constructed jam element should have a namespace binding xmlns:bdd="http://bdd.test.com", copied from the xsl:stylesheet element.

The jam element should also have an attribute with local name attr, namespace URI http://xyz.com. The prefix for this attribute would normally be bdd. but this would create a conflict, so a different prefix must be allocated, and the appearance of ns0 in the error message suggests that it is attempting to allocate a prefix.

The error is coming from the XdmQName constructor (XdmAtomic.js line 754). This is being called from computedQNameEvaluator() (Push.js line 1123). The value of the variable name (line 1128) is a zero-length string.

Hmm.. I suspect that this error is genuine, docs/b isn't selecting anything, probably because we've selected the wrong source document. Try again.

Actions #3

Updated by Michael Kay over 2 years ago

That's better. Now running with the correct source document, and getting output

<ans xmlns:bdd="http://bdd.test.com" bdd="http://xyz.com" attr-ns="http://xyz.com"/>

compared with the expected

<ans xmlns:bdd="http://bdd.test.com" bdd="http://bdd.test.com" attr-ns="http://xyz.com"/>

So the problem is that when we created the attribute, the prefix proposed in xsl:attribute/@name was used, despite the existence of a conflict with the binding of that prefix inherited by the literal result element.

Actions #4

Updated by Michael Kay over 2 years ago

And looking at the code of computedQNameEvaluator(), at Push line 1165, we see

// TODO: handle conflicts where the prefix needs to be changed - see stylesheetResolver

But actually, that's probably not where it should be done. It's only when we get to the ComplexContentOutputter that we have enough information to know that there's a conflict.

The logic that attempts this is at Push.js line 177

                    const p2 = this.ensureBinding(name.prefix, name.uri, false);
                    if (p2 !== name.prefix) {
                        name = name.withPrefix(p2);
                    }

But when we call ensureBinding("bdd", "http://xyz.com", false), the map pendingNamespaces is empty, so no conflict is detected.

The problem seems to be that we are only looking in pendingNamespaces for an existing binding, we are not looking in namespaceBindings. Because the binding ("bdd", "http://bdd.test.com") is effectively inherited from the parent element (out), it's present in namespaceBindings but not in pendingNamespaces.

Actions #5

Updated by Michael Kay over 2 years ago

Frustrating progress. With minor tweaks to this code I can get this test case working easily enough, but it always breaks something else.

Actions #6

Updated by Michael Kay over 2 years ago

I seem to have found the right magic to make this test pass without breaking anything else. The code, however, feels fragile.

Actions #7

Updated by Michael Kay over 2 years ago

  • Status changed from New to Resolved
  • Applies to JS Branch 2, Trunk added
  • Fix Committed on JS Branch 2, Trunk added
Actions #8

Updated by Debbie Lockett over 2 years ago

  • Status changed from Resolved to Closed
  • % Done changed from 0 to 100
  • Fixed in JS Release set to SaxonJS 2.4

Bug fix applied in the SaxonJS 2.4 maintenance release.

Actions #9

Updated by Debbie Lockett over 2 years ago

  • Status changed from Closed to In Progress
  • Fixed in JS Release deleted (SaxonJS 2.4)

Reopening this bug, because the test attribute-1301 is failing again. In fact, it looks like the bug was not fixed in the SaxonJS 2.4 release as previously stated, since the test results show the test was failing then too.

I have reinstated the test exception in the exceptions list.

Actions #10

Updated by Debbie Lockett over 2 years ago

Before finding this existing bug and the notes, I spent some time trying to debug the attribute-1301 test failure, but don't really feel like I've got anywhere. I don't feel like I've got a grasp on what's going on in the ComplexContentOutputter. I made some attempts to fix the code, but these only resulted in causing other test failures (much like Mike previously found, see #note-5).

Actions #11

Updated by Norm Tovey-Walsh over 2 years ago

  • Sprint/Milestone set to SaxonJS 2.5
Actions #12

Updated by Michael Kay about 2 years ago

Now fixed. The fix was indeed in the area identified in comment #4 - ComplexContentOutputter.ensureBinding().

The rules for literal result elements result in a binding for namespace prefix bdd on the jam element, but we are largely ignoring this because the prefix is already correctly bound on the parent element. This means that when we come to write the attribute node to the bdd element, we feel free to add a namespace binding for bdd to a different URI.

The solution is to exploit the fact that we have recorded the presence of the original namespace binding in the forcedBindings object. We extend this object to hold the actual URI to which the prefix is bound; and when the attribute is processed, we detect that forcedBindings contains a binding of bdd to a different URI, and under this condition we allocate a different prefix to the attribute (with the side-effect of adding a namespace binding for the new prefix.)

Actions #13

Updated by Debbie Lockett about 2 years ago

  • Status changed from In Progress to Resolved

Code fix committed on saxonjs2 and main branches. (Plus attribute-1301 removed again from the xslt30 exceptions file.)

Actions #14

Updated by Norm Tovey-Walsh about 2 years ago

  • Status changed from Resolved to Closed
  • Fixed in JS Release set to SaxonJS 2.5

Fixed in SaxonJS 2.5.

Please register to edit this issue

Also available in: Atom PDF Tracking page