Fix duplicate bf:relation from a 490 with a $v or $x - #274
Open
edsu wants to merge 1 commit into
Open
Conversation
A 490 carrying an enumeration or ISSN subfield alongside its $a came out as two identical bf:relation values on the Work -- under libxslt, but not under Saxon. The group numbers of the grouped 490 fragment sit on its top-level children, so preceding-sibling:: reaches all of them, and says what the code means more directly than walking preceding:: from an attribute. It also avoids a libxml2 bug: over a node-set made with exsl:node-set, the preceding:: axis omits the fragment's first top-level element. The group number being compared against was therefore never in the node-set, nothing was filtered, and every child of the fragment built its own bf:Relation. Saxon walks the axis correctly, which is why the XSpec suite has always passed and this only ever reached users on libxslt -- lxml, Nokogiri, PHP's XSL extension, XML::LibXSLT and xsltproc. test/data/ConvSpec-Process6-Series/dup-relations.xml and a new scenario expect one relation from each of 490 $a, $a $v and $a $x, following the dup-properties.xml pattern in ConvSpec-001-007. They pass under Saxon with or without the fix, so they guard the behaviour rather than demonstrate the bug. test/compare-processors.sh is what demonstrates it. It converts the same records with both processors and requires the results to match, after rendering both through test/normalize.xsl so that indentation, attribute order and the placement of namespace declarations are not differences. CI installs xsltproc, which the config already anticipated in a commented-out line, and runs the script after the XSpec suites. Its default record set is deliberately small so the check is green from the start; pointing it at other paths is how it widens. No change under Saxon: output is byte for byte identical over test/data/marc.xml, test/data/collection.xml and test/data/ConvSpec-Process6-Series/marc.xml, and the full XSpec suite still passes 1378/1378. Under libxslt the relation counts now match Saxon's exactly. Fixes lcnetdev#273
edsu
commented
Sep 10, 2026
| <!-- Find the group numbers. --> | ||
| <xsl:variable name="tThisDF" select="."/> | ||
| <xsl:variable name="tGroupNums" select="$grouped490Info//@groupNum[not(.=preceding::bf:*/@groupNum[1])]" /> | ||
| <xsl:variable name="tGroupNums" select="$grouped490Info/*[not(@groupNum = preceding-sibling::*/@groupNum)]/@groupNum" /> |
Author
There was a problem hiding this comment.
This is the one line fix! Everything else is just testing with xsltproc and docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full disclosure: these edits were suggested by Claude Code. Really there's just a one line change that's needed for things to work correctly under Saxon and libxslt. If you want to zoom in on that, I've highlighted it below with a comment. But if you want the additional testing bits, that's where the majority of the PR is. If you'd rather have the PR trimmed back to the bare essentials let me know.
A 490 carrying an enumeration or ISSN subfield alongside its $a came out as two identical bf:relation values on the Work -- under libxslt, but not under Saxon.
The group numbers of the grouped 490 fragment sit on its top-level children, so
preceding-sibling::reaches all of them, and says what the code means more directly than walkingpreceding::from an attribute. It also avoids a libxml2 bug: over a node-set made withexsl:node-set, thepreceding::axis omits the fragment's first top-level element. The group number being compared against was therefore never in the node-set, nothing was filtered, and every child of the fragment built its own bf:Relation. Saxon walks the axis correctly, which is why the XSpec suite has always passed and this only ever reached users on libxslt: lxml, Nokogiri, PHP's XSL extension, XML::LibXSLT and xsltproc.test/data/ConvSpec-Process6-Series/dup-relations.xmland a new scenario expect one relation from each of 490 $a, $a $v and $a $x, following the dup-properties.xmlpattern inConvSpec-001-007. They pass under Saxon with or without the fix, so they guard the behavior rather than demonstrate the bug.test/compare-processors.shis what demonstrates it. It converts the same records with both processors and requires the results to match, after rendering both throughtest/normalize.xslso that indentation, attribute order and the placement of namespace declarations are not differences. CI installs xsltproc, which the config already anticipated in a commented-out line, and runs the script after the XSpec suites. Its default record set is deliberately small so the check is green from the start; pointing it at other paths is how it widens.No change under Saxon: output is byte for byte identical over
test/data/marc.xml,test/data/collection.xmlandtest/data/ConvSpec-Process6-Series/marc.xml, and the full XSpec suite still passes 1378/1378. Under libxslt the relation counts now match Saxon's exactly.Fixes #273