[hotfix] serialize item-separator can be empty, skip empty values

fixes #4704

Author: line-o

exist-core/src/main/java/org/exist/xquery/functions/fn/FunSerialize.java

@@ -82,13 +82,16 @@ public Sequence eval(Sequence[] args, Sequence contextSequence) throws XPathExce
         try(final StringWriter writer = new StringWriter()) {
             final XQuerySerializer xqSerializer = new XQuerySerializer(context.getBroker(), outputProperties, writer);
 
-            Sequence seq = args[0];
+            final Sequence input = args[0];
             if (xqSerializer.normalize()) {
                 final String itemSeparator = outputProperties.getProperty(EXistOutputKeys.ITEM_SEPARATOR, DEFAULT_ITEM_SEPARATOR);
-                seq = normalize(this, context, seq, itemSeparator);
+                final Sequence normalized = normalize(this, context, input, itemSeparator);
+                xqSerializer.serialize(normalized);
+            }
+            else {
+                xqSerializer.serialize(input);
             }
 
-            xqSerializer.serialize(seq);
             return new StringValue(this, writer.toString());
         } catch (final IOException | SAXException e) {
             throw new XPathException(this, FnModule.SENR0001, e.getMessage());
@@ -97,9 +100,9 @@ public Sequence eval(Sequence[] args, Sequence contextSequence) throws XPathExce
 
     public static Properties getSerializationProperties(final Expression callingExpr, final Item parametersItem) throws XPathException {
         final Properties outputProperties;
-        if(parametersItem.getType() == Type.MAP) {
+        if (parametersItem.getType() == Type.MAP) {
             outputProperties = SerializerUtils.getSerializationOptions(callingExpr, (AbstractMapType) parametersItem);
-        } else if(isSerializationParametersElement(parametersItem)) {
+        } else if (isSerializationParametersElement(parametersItem)) {
             outputProperties = new Properties();
             SerializerUtils.getSerializationOptions(callingExpr, (NodeValue) parametersItem, outputProperties);
         } else {
@@ -144,20 +147,33 @@ public static Sequence normalize(final Expression callingExpr, final XQueryConte
         if (input.isEmpty())
             // "If the sequence that is input to serialization is empty, create a sequence S1 that consists of a zero-length string."
             {return StringValue.EMPTY_STRING;}
-        final String _separator = itemSeparator == null ? DEFAULT_ITEM_SEPARATOR : itemSeparator;
         final ValueSequence temp = new ValueSequence(input.getItemCount());
         for (final SequenceIterator i = input.iterate(); i.hasNext(); ) {
             final Item next = i.nextItem();
-            if (Type.subTypeOf(next.getType(), Type.NODE)) {
-                if (next.getType() == Type.ATTRIBUTE || next.getType() == Type.NAMESPACE || next.getType() == Type.FUNCTION_REFERENCE)
-                    {throw new XPathException(callingExpr, FnModule.SENR0001,
-                        "It is an error if an item in the sequence to serialize is an attribute node or a namespace node.");}
+            final int itemType = next.getType();
+            if (Type.subTypeOf(itemType, Type.NODE)) {
+                if (itemType == Type.ATTRIBUTE || itemType == Type.NAMESPACE || itemType == Type.FUNCTION_REFERENCE) {
+                    throw new XPathException(callingExpr, FnModule.SENR0001,
+                        "It is an error if an item in the sequence to serialize is an attribute node or a namespace node.");
+                }
+//                if (itemType == Type.TEXT || itemType == Type.COMMENT) {
+//                    final StringValue stringRepresentation = new StringValue(callingExpr, next.getStringValue());
+//                    if (stringRepresentation.getStringValue().isEmpty()) {
+//                        continue;
+//                    }
+//                    temp.add(stringRepresentation);
+//                }
                 temp.add(next);
             } else {
                 // atomic value
                 // "For each item in S1, if the item is atomic, obtain the lexical representation of the item by
                 // casting it to an xs:string and copy the string representation to the new sequence;"
-                temp.add(new StringValue(callingExpr, next.getStringValue()));
+                final StringValue stringRepresentation = new StringValue(callingExpr, next.getStringValue());
+                // skip values that evaluate to an empty string
+                if (stringRepresentation.getStringValue().isEmpty()) {
+                    continue;
+                }
+                temp.add(stringRepresentation);
             }
         }
 
@@ -170,10 +186,12 @@ public static Sequence normalize(final Expression callingExpr, final XQueryConte
                 if (Type.subTypeOf(next.getType(), Type.NODE)) {
                     next.copyTo(context.getBroker(), receiver);
                 } else {
-                    receiver.characters(next.getStringValue());
+                    final String stringValue = next.getStringValue();
+                    receiver.characters(stringValue);
                 }
+                // add itemSeparator if there is a next item and the current item is not an empty string
                 if (i.hasNext()) {
-                    receiver.characters(_separator);
+                    receiver.characters(itemSeparator);
                 }
             }
             return (DocumentImpl)receiver.getDocument();

exist-core/src/main/java/org/exist/xquery/functions/util/Eval.java

@@ -404,7 +404,7 @@ private Sequence doEval(final XQueryContext evalContext, final Sequence contextS
 
                         final Sequence seq;
                         if (xqSerializer.normalize()) {
-                            seq = FunSerialize.normalize(this, context, result, null);
+                            seq = FunSerialize.normalize(this, context, result, "");
                         } else {
                             seq = result;
                         }

exist-core/src/main/java/org/exist/xquery/util/SerializerUtils.java

@@ -450,7 +450,12 @@ private static Sequence getParameterValue(final Expression parent, final Abstrac
         final Sequence providedParameterValue = entries.get(parameterConventionEntryKey);
 
         // should we use the default value
-        if (providedParameterValue == null || providedParameterValue.isEmpty() || (parameterConvention.getType() == Type.STRING && isEmptyStringValue(providedParameterValue))) {
+        if (providedParameterValue == null || providedParameterValue.isEmpty() || (
+                parameterConvention.getType() == Type.STRING && isEmptyStringValue(providedParameterValue) &&
+                        // allow empty separator #4704
+                        parameterConvention.getParameterName() != EXistOutputKeys.ITEM_SEPARATOR
+        )
+        ) {
             // use default value
 
             if (W3CParameterConvention.MEDIA_TYPE == parameterConvention) {

exist-core/src/test/java/xquery/xquery3/XQuery3Tests.java

@@ -28,7 +28,8 @@
 @XSuite.XSuiteFiles({
         "src/test/xquery/xquery3",
         "src/test/xquery/xquery3/transform",
-        //Reminder - add an individual test like this - "src/test/xquery/xquery3/transform/<test-file>.xqm",
+        // To add an individual test or only run a specific set of tests -
+//         "src/test/xquery/xquery3/serialize.xql",
 })
 public class XQuery3Tests {
 }

exist-core/src/test/xquery/xquery3/serialize.xql

@@ -224,6 +224,26 @@ function ser:serialize-atomic() {
         fn:serialize($nodes)
 };
 
+(: test for https://github.com/eXist-db/exist/issues/4704 :)
+declare
+    %test:assertEquals("aaabbb")
+function ser:serialize-atomic-empty-separator() {
+    fn:serialize(("aaa", "bbb"), map {"item-separator": ""})
+};
+
+(: test for https://github.com/eXist-db/exist/issues/4704 :)
+declare
+    %test:assertEquals("aaabbb")
+function ser:serialize-atomic-empty-separator-xml-options() {
+    fn:serialize(("aaa", "bbb"),
+      <output:serialization-parameters
+           xmlns:output="http://www.w3.org/2010/xslt-xquery-serialization">
+        <output:item-separator value=""/>
+      </output:serialization-parameters>
+    )
+};
+
+
 declare
     %test:assertEquals("")
 function ser:serialize-empty-sequence() {
@@ -935,3 +955,27 @@ function ser:sequence-of-nodes() {
     (<a>foo</a>, <b>bar</b>) => serialize()
 };
 
+
+declare
+    %test:assertEquals("foo")
+function ser:skip-empty-no-separator() {
+    (<a>foo</a>, <b></b>)/text() => serialize(map{"item-separator": "!"})
+};
+
+declare
+    %test:assertEquals("")
+function ser:empty-array-serializes-to-empty-string() {
+    [] => serialize()
+};
+
+declare
+    %test:assertEquals("")
+function ser:array-with-members-serializes-to-empty-string() {
+    ["", ()] => serialize()
+};
+
+declare
+    %test:assertEquals("")
+function ser:sequence-of-empty-arrays-serializes-to-empty-string() {
+    ([],[]) => serialize()
+};