Fix duplicate bf:relation from a 490 with a $v or $x

.circleci/config.yml

@@ -18,8 +18,8 @@ jobs:
       - run:
           name: Install testing prerequisites
           command: |
-            # sudo apt-get update
-            # sudo apt-get install -y xsltproc
+            sudo apt-get update
+            sudo apt-get install -y xsltproc
             mkdir /tmp/saxon/
             mkdir /tmp/saxon/lib/
 
@@ -46,6 +46,9 @@ jobs:
       - run:
           name: Run LoC-specific test suite
           command: "$XSPEC test/ConvSpec-DLC.xspec"
+      - run:
+          name: Compare libxslt and Saxon output
+          command: SAXON_CP=${SAXON_CP:-/tmp/saxon/saxon-he-12.4.jar} ./test/compare-processors.sh
       - store_artifacts:
           path: test/xspec/marc2bibframe2-result.html
           destination: xspec-tests/marc2bibframe2-result.html

README.md

@@ -185,6 +185,22 @@ LoC-specific conversions, run only the ConvSpec-DLC.xspec test suite:
 
     xspec.sh test/ConvSpec-DLC.xspec
 
+### Testing against libxslt
+
+The XSpec suite runs under Saxon. A construct that Saxon evaluates correctly
+and libxslt does not will pass it and still produce wrong output for the many
+downstream users on libxslt -- lxml, Nokogiri, PHP's XSL extension,
+XML::LibXSLT and `xsltproc` itself. To catch that, `test/compare-processors.sh`
+converts the same records with both processors and requires the results to
+match:
+
+    SAXON_CP=/tmp/saxon/saxon-he-12.4.jar ./test/compare-processors.sh
+
+Both results are rendered through `test/normalize.xsl` first, by the same
+processor, so indentation, attribute order and where namespace declarations
+land are not treated as differences. With no arguments the script checks the
+records named in its `DEFAULT_FILES`; pass paths to check others.
+
 ## Active record conversion
 
 Active conversion of records - resolving URIs for elements of the

test/ConvSpec-Process6-Series.xspec

@@ -27,6 +27,15 @@
     <x:expect label="$x of a 490 with ind1=1 and no matching 8XX creates a hasSeries property of the Work" test="//bf:Work[1]/bf:relation[1]/bf:Relation/bf:associatedResource/bf:Hub/bf:Work/bf:identifiedBy/bf:Issn/rdf:value = '1400-1144'"/>-->
   </x:scenario>
 
+  <x:scenario label="490 - DUPLICATE RELATIONS">
+    <x:context href="data/ConvSpec-Process6-Series/dup-relations.xml"/>
+    <x:expect label="A 490 with one $a creates one relation" test="count(//bf:Work[1]/bf:relation) = 1"/>
+    <x:expect label="...still one relation when the 490 also has a $v" test="count(//bf:Work[2]/bf:relation) = 1"/>
+    <x:expect label="...and the $v is the seriesEnumeration of that relation" test="//bf:Work[2]/bf:relation[1]/bf:Relation/bf:seriesEnumeration = 'no. 32'"/>
+    <x:expect label="...still one relation when the 490 also has a $x" test="count(//bf:Work[3]/bf:relation) = 1"/>
+    <x:expect label="...and the $x identifies the series of that relation" test="//bf:Work[3]/bf:relation[1]/bf:Relation/bf:associatedResource/bf:Series/bf:identifiedBy/bf:Issn/rdf:value = '1234-5678'"/>
+  </x:scenario>
+
   <x:scenario label="8XX SERIES STATEMENT and SERIES ADDED ENTRY">
     <x:context href="data/ConvSpec-Process6-Series/marc.xml"/>
     <x:expect label="Each 8XX  creates a hasSeries Hub property of the Work" test="count(//bf:Work[1]/bf:relation/bf:Relation/bf:associatedResource/bf:Hub) = 2"/>

test/compare-processors.sh

@@ -0,0 +1,103 @@
+#!/usr/bin/env bash
+#
+# Convert every test record with both Saxon and libxslt and require the two
+# results to agree.
+#
+# The XSpec suite runs under Saxon only, so a construct that Saxon evaluates
+# correctly and libxslt does not passes CI and still produces wrong output for
+# the many downstream users on libxslt -- lxml (Python), Nokogiri (Ruby), PHP's
+# XSL extension, XML::LibXSLT (Perl) and xsltproc itself.
+#
+# Both results are rendered through test/normalize.xsl before comparison, by
+# the same processor, so that indentation, attribute order and the placement of
+# namespace declarations do not count as differences.
+#
+# Usage:
+#   SAXON_CP=/path/to/saxon-he-12.4.jar ./test/compare-processors.sh [file...]
+#
+# With no arguments it checks the records listed in DEFAULT_FILES below. Pass
+# paths to check anything else -- pointing it at all of test/data is a quick
+# way to see where else the two processors part company.
+
+set -u
+
+cd "$(dirname "$0")/.."
+
+: "${SAXON_CP:?set SAXON_CP to the Saxon jar, e.g. /tmp/saxon/saxon-he-12.4.jar}"
+
+command -v xsltproc >/dev/null || { echo "xsltproc not found"; exit 2; }
+
+STYLESHEET=xsl/marc2bibframe2.xsl
+NORMALIZE=test/normalize.xsl
+
+# The transform stamps the current time into the work's admin metadata, which
+# would differ between the two runs.
+DATESTAMP=2020-01-01T00:00:00
+
+work=$(mktemp -d)
+trap 'rm -rf "$work"' EXIT
+
+# The records this check covers. Deliberately a subset for now: it starts with
+# the series data, and should grow as other processor divergences are found and
+# resolved.
+DEFAULT_FILES=(test/data/ConvSpec-Process6-Series/*.xml)
+
+if [ "$#" -gt 0 ]; then
+  files=("$@")
+else
+  files=("${DEFAULT_FILES[@]}")
+fi
+
+failed=0
+checked=0
+
+for file in "${files[@]}"; do
+  checked=$((checked + 1))
+
+  if ! xsltproc --stringparam pGenerationDatestamp "$DATESTAMP" \
+       "$STYLESHEET" "$file" > "$work/libxslt.xml" 2> "$work/libxslt.err"; then
+    echo "FAIL $file"
+    echo "     libxslt could not convert it:"
+    sed 's/^/       /' "$work/libxslt.err"
+    failed=$((failed + 1))
+    continue
+  fi
+
+  if [ ! -s "$work/libxslt.xml" ]; then
+    echo "FAIL $file"
+    echo "     libxslt produced no output"
+    sed 's/^/       /' "$work/libxslt.err"
+    failed=$((failed + 1))
+    continue
+  fi
+
+  java -cp "$SAXON_CP" net.sf.saxon.Transform \
+    -s:"$file" -xsl:"$STYLESHEET" "pGenerationDatestamp=$DATESTAMP" \
+    -o:"$work/saxon.xml" 2> "$work/saxon.err" || {
+      echo "FAIL $file"
+      echo "     Saxon could not convert it:"
+      sed 's/^/       /' "$work/saxon.err"
+      failed=$((failed + 1))
+      continue
+    }
+
+  xsltproc "$NORMALIZE" "$work/libxslt.xml" > "$work/libxslt.txt"
+  xsltproc "$NORMALIZE" "$work/saxon.xml" > "$work/saxon.txt"
+
+  if ! diff -q "$work/libxslt.txt" "$work/saxon.txt" > /dev/null; then
+    echo "FAIL $file"
+    echo "     libxslt and Saxon disagree (< libxslt, > Saxon), first 20 lines:"
+    diff "$work/libxslt.txt" "$work/saxon.txt" | head -20 | sed 's/^/       /'
+    failed=$((failed + 1))
+    continue
+  fi
+
+  echo "ok   $file"
+done
+
+echo
+if [ "$failed" -gt 0 ]; then
+  echo "$failed of $checked records differ between processors"
+  exit 1
+fi
+echo "$checked records convert identically under libxslt and Saxon"

test/data/ConvSpec-Process6-Series/dup-relations.xml

@@ -0,0 +1,22 @@
+<collection xmlns="http://www.loc.gov/MARC21/slim">
+  <record>
+    <controlfield tag="001">1</controlfield>
+    <datafield tag="490" ind1="0" ind2=" ">
+      <subfield code="a">Lund studies in geography</subfield>
+    </datafield>
+  </record>
+  <record>
+    <controlfield tag="001">2</controlfield>
+    <datafield tag="490" ind1="0" ind2=" ">
+      <subfield code="a">Lund studies in geography ;</subfield>
+      <subfield code="v">no. 32</subfield>
+    </datafield>
+  </record>
+  <record>
+    <controlfield tag="001">3</controlfield>
+    <datafield tag="490" ind1="0" ind2=" ">
+      <subfield code="a">Lund studies in geography ;</subfield>
+      <subfield code="x">1234-5678</subfield>
+    </datafield>
+  </record>
+</collection>

test/normalize.xsl

@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  Render a conversion result as one line per node, so that output from two
+  XSLT processors can be compared without their serialization differences
+  (indentation, attribute order, where namespace declarations land) showing up
+  as false differences. Run it with the same processor over both files.
+-->
+<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
+
+  <xsl:output method="text" encoding="UTF-8"/>
+  <xsl:strip-space elements="*"/>
+
+  <xsl:template match="/">
+    <xsl:apply-templates select="*"/>
+  </xsl:template>
+
+  <xsl:template match="*">
+    <xsl:param name="depth" select="0"/>
+    <xsl:call-template name="indent">
+      <xsl:with-param name="n" select="$depth"/>
+    </xsl:call-template>
+    <xsl:value-of select="name()"/>
+    <xsl:for-each select="@*">
+      <xsl:sort select="name()"/>
+      <xsl:value-of select="concat(' ', name(), '=&quot;', normalize-space(.), '&quot;')"/>
+    </xsl:for-each>
+    <xsl:if test="normalize-space(text()) != ''">
+      <xsl:value-of select="concat(' | ', normalize-space(text()))"/>
+    </xsl:if>
+    <xsl:text>&#10;</xsl:text>
+    <xsl:apply-templates select="*">
+      <xsl:with-param name="depth" select="$depth + 1"/>
+    </xsl:apply-templates>
+  </xsl:template>
+
+  <xsl:template name="indent">
+    <xsl:param name="n"/>
+    <xsl:if test="$n &gt; 0">
+      <xsl:text>  </xsl:text>
+      <xsl:call-template name="indent">
+        <xsl:with-param name="n" select="$n - 1"/>
+      </xsl:call-template>
+    </xsl:if>
+  </xsl:template>
+
+</xsl:stylesheet>

xsl/ConvSpec-Process6-Series.xsl

@@ -63,7 +63,7 @@
 
     <!-- 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" />
       <xsl:for-each select="$tGroupNums">
         <xsl:variable name="tGNum" select="."/>    
         <bf:relation>