Apostrophe handling in CpsPathParser 74/135474/7
authordanielhanrahan <daniel.hanrahan@est.tech>
Wed, 28 Jun 2023 11:55:20 +0000 (12:55 +0100)
committerdanielhanrahan <daniel.hanrahan@est.tech>
Thu, 20 Jul 2023 09:08:50 +0000 (10:08 +0100)
Apostrophe is not currently handled correctly, and having apostrophe in
the xpath will lead to various errors.
For example, normalizing this xpath works:
  /path[@name="I'm quoted"] -> /path[@name='I\'m quoted']
However the resulting xpath will throw a PathParsingException if parsed!
(Thus path normalization is not idempotent.)

- Use '' for escaping apostrophe in single quoted leaf value,
  to comply with XPath standard (and use "" for escaping in ").
- Use Liquibase to make existing data comply with new rules.
- Leaf values in data leaves are now unescaped, e.g. "I'm quoted"
- Quoting is now consistent for leaf/text/contains conditions.

Issue-ID: CPS-1769
Signed-off-by: danielhanrahan <daniel.hanrahan@est.tech>
Change-Id: Iafc287f738254d7f99706c6bc548091c0ecd5aa0

15 files changed:
cps-path-parser/src/main/antlr4/org/onap/cps/cpspath/parser/antlr4/CpsPath.g4
cps-path-parser/src/main/java/org/onap/cps/cpspath/parser/CpsPathBuilder.java
cps-path-parser/src/test/groovy/org/onap/cps/cpspath/parser/CpsPathQuerySpec.groovy
cps-ri/src/main/java/org/onap/cps/spi/repository/FragmentQueryBuilder.java
cps-ri/src/main/java/org/onap/cps/spi/repository/TempTableCreator.java
cps-ri/src/main/java/org/onap/cps/spi/utils/EscapeUtils.java
cps-ri/src/main/resources/changelog/changelog-master.yaml
cps-ri/src/main/resources/changelog/db/changes/21-escape-quotes-in-xpath-forward.sql [new file with mode: 0644]
cps-ri/src/main/resources/changelog/db/changes/21-escape-quotes-in-xpath-rollback.sql [new file with mode: 0644]
cps-ri/src/main/resources/changelog/db/changes/21-escape-quotes-in-xpath.yaml [new file with mode: 0644]
cps-ri/src/test/groovy/org/onap/cps/spi/utils/EscapeUtilsSpec.groovy
cps-service/src/main/java/org/onap/cps/utils/YangUtils.java
docs/cps-path.rst
docs/release-notes.rst
integration-test/src/test/groovy/org/onap/cps/integration/functional/CpsQueryServiceIntegrationSpec.groovy

index c88a822..3aef120 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  ============LICENSE_START=======================================================
- *  Copyright (C) 2021-2022 Nordix Foundation
+ *  Copyright (C) 2021-2023 Nordix Foundation
  *  Modifications Copyright (C) 2023 TechMahindra Ltd
  *  ================================================================================
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  ============LICENSE_END=========================================================
  */
 
+/*
+ * Parser Rules
+ * Some of the parser rules below are inspired by
+ * https://github.com/antlr/grammars-v4/blob/master/xpath/xpath31/XPath31Parser.g4
+ */
+
 grammar CpsPath ;
 
 cpsPath : ( prefix | descendant | incorrectPrefix ) multipleLeafConditions? textFunctionCondition? containsFunctionCondition? ancestorAxis? invalidPostFix?;
@@ -60,7 +66,7 @@ invalidPostFix : (AT | CB | COLONCOLON | comparativeOperators ).+ ;
 /*
  * Lexer Rules
  * Most of the lexer rules below are inspired by
- * https://raw.githubusercontent.com/antlr/grammars-v4/master/xpath/xpath31/XPath31.g4
+ * https://github.com/antlr/grammars-v4/blob/master/xpath/xpath31/XPath31Lexer.g4
  */
 
 AT : '@' ;
@@ -89,9 +95,9 @@ IntegerLiteral : FragDigits ;
 // Add below type definitions for leafvalue comparision in https://jira.onap.org/browse/CPS-440
 DecimalLiteral : ('.' FragDigits) | (FragDigits '.' [0-9]*) ;
 DoubleLiteral : (('.' FragDigits) | (FragDigits ('.' [0-9]*)?)) [eE] [+-]? FragDigits ;
-StringLiteral : ('"' (FragEscapeQuot | ~[^"])*? '"') | ('\'' (FragEscapeApos | ~['])*? '\'') ;
+StringLiteral : '"' (~["] | FragEscapeQuot)* '"' | '\'' (~['] | FragEscapeApos)* '\'' ;
 fragment FragEscapeQuot : '""' ;
-fragment FragEscapeApos : '\';
+fragment FragEscapeApos : '\'\'';
 fragment FragDigits : [0-9]+ ;
 
 QName  : FragQName ;
index 9913596..de261e6 100644 (file)
@@ -79,18 +79,13 @@ public class CpsPathBuilder extends CpsPathBaseListener {
 
     @Override
     public void exitLeafCondition(final LeafConditionContext ctx) {
-        Object comparisonValue;
+        final Object comparisonValue;
         if (ctx.IntegerLiteral() != null) {
             comparisonValue = Integer.valueOf(ctx.IntegerLiteral().getText());
         } else if (ctx.StringLiteral() != null) {
-            final boolean wasWrappedInDoubleQuote = ctx.StringLiteral().getText().startsWith("\"");
-            comparisonValue = stripFirstAndLastCharacter(ctx.StringLiteral().getText());
-            if (wasWrappedInDoubleQuote) {
-                comparisonValue = String.valueOf(comparisonValue).replace("'", "\\'");
-            }
+            comparisonValue = unwrapQuotedString(ctx.StringLiteral().getText());
         } else {
-            throw new PathParsingException(
-                    "Unsupported comparison value encountered in expression" + ctx.getText());
+            throw new PathParsingException("Unsupported comparison value encountered in expression" + ctx.getText());
         }
         leafContext(ctx.leafName(), comparisonValue);
     }
@@ -140,13 +135,13 @@ public class CpsPathBuilder extends CpsPathBaseListener {
     @Override
     public void exitTextFunctionCondition(final TextFunctionConditionContext ctx) {
         cpsPathQuery.setTextFunctionConditionLeafName(ctx.leafName().getText());
-        cpsPathQuery.setTextFunctionConditionValue(stripFirstAndLastCharacter(ctx.StringLiteral().getText()));
+        cpsPathQuery.setTextFunctionConditionValue(unwrapQuotedString(ctx.StringLiteral().getText()));
     }
 
     @Override
     public void exitContainsFunctionCondition(final CpsPathParser.ContainsFunctionConditionContext ctx) {
         cpsPathQuery.setContainsFunctionConditionLeafName(ctx.leafName().getText());
-        cpsPathQuery.setContainsFunctionConditionValue(stripFirstAndLastCharacter(ctx.StringLiteral().getText()));
+        cpsPathQuery.setContainsFunctionConditionValue(unwrapQuotedString(ctx.StringLiteral().getText()));
     }
 
     @Override
@@ -173,10 +168,6 @@ public class CpsPathBuilder extends CpsPathBaseListener {
         return cpsPathQuery;
     }
 
-    private static String stripFirstAndLastCharacter(final String wrappedString) {
-        return wrappedString.substring(1, wrappedString.length() - 1);
-    }
-
     @Override
     public void exitContainerName(final CpsPathParser.ContainerNameContext ctx) {
         final String containerName = ctx.getText();
@@ -207,11 +198,25 @@ public class CpsPathBuilder extends CpsPathBaseListener {
                                     .append(name)
                                     .append(getLastElement(comparativeOperators))
                                     .append("'")
-                                    .append(value)
+                                    .append(value.toString().replace("'", "''"))
                                     .append("'");
     }
 
-    private String getLastElement(final List<String> listOfStrings) {
+    private static String getLastElement(final List<String> listOfStrings) {
         return listOfStrings.get(listOfStrings.size() - 1);
     }
+
+    private static String unwrapQuotedString(final String wrappedString) {
+        final boolean wasWrappedInSingleQuote = wrappedString.startsWith("'");
+        final String value = stripFirstAndLastCharacter(wrappedString);
+        if (wasWrappedInSingleQuote) {
+            return value.replace("''", "'");
+        } else {
+            return value.replace("\"\"", "\"");
+        }
+    }
+
+    private static String stripFirstAndLastCharacter(final String wrappedString) {
+        return wrappedString.substring(1, wrappedString.length() - 1);
+    }
 }
index 7896303..0017242 100644 (file)
@@ -46,6 +46,10 @@ class CpsPathQuerySpec extends Specification {
             'spaces around ='      | '/parent/child[@common-leaf-name-int = 5]'                 || '/parent/child'                                 | 'common-leaf-name-int' | 5
             'key in top container' | '/parent[@common-leaf-name-int=5]'                         || '/parent'                                       | 'common-leaf-name-int' | 5
             'parent list'          | '/shops/shop[@id=1]/categories[@id=1]/book[@title="Dune"]' || "/shops/shop[@id='1']/categories[@id='1']/book" | 'title'                | 'Dune'
+            "' in double quote"    | '/parent[@common-leaf-name="leaf\'value"]'                 || '/parent'                                       | 'common-leaf-name'     | "leaf'value"
+            "' in single quote"    | "/parent[@common-leaf-name='leaf''value']"                 || '/parent'                                       | 'common-leaf-name'     | "leaf'value"
+            '" in double quote'    | '/parent[@common-leaf-name="leaf""value"]'                 || '/parent'                                       | 'common-leaf-name'     | 'leaf"value'
+            '" in single quote'    | '/parent[@common-leaf-name=\'leaf"value\']'                || '/parent'                                       | 'common-leaf-name'     | 'leaf"value'
     }
 
     def 'Parse cps path of type ends with a #scenario.'() {
@@ -80,8 +84,8 @@ class CpsPathQuerySpec extends Specification {
             'parent leaf of type Integer & child'                         | '/parent/child[@code=1]/child2'                                || "/parent/child[@code='1']/child2"
             'parent leaf with double quotes'                              | '/parent/child[@code="1"]/child2'                              || "/parent/child[@code='1']/child2"
             'parent leaf with double quotes inside single quotes'         | '/parent/child[@code=\'"1"\']/child2'                          || "/parent/child[@code='\"1\"']/child2"
-            'parent leaf with single quotes inside double quotes'         | '/parent/child[@code="\'1\'"]/child2'                          || "/parent/child[@code='\\\'1\\\'']/child2"
-            'leaf with single quotes inside double quotes'                | '/parent/child[@code="\'1\'"]'                                 || "/parent/child[@code='\\\'1\\\'']"
+            'parent leaf with single quotes inside double quotes'         | '/parent/child[@code="\'1\'"]/child2'                          || "/parent/child[@code='''1''']/child2"
+            'leaf with single quotes inside double quotes'                | '/parent/child[@code="\'1\'"]'                                 || "/parent/child[@code='''1''']"
             'leaf with more than one attribute'                           | '/parent/child[@key1=1 and @key2="abc"]'                       || "/parent/child[@key1='1' and @key2='abc']"
             'parent & child with more than one attribute'                 | '/parent/child[@key1=1 and @key2="abc"]/child2'                || "/parent/child[@key1='1' and @key2='abc']/child2"
             'leaf with more than one attribute has OR operator'           | '/parent/child[@key1=1 or @key2="abc"]'                        || "/parent/child[@key1='1' or @key2='abc']"
index be06eba..e371035 100644 (file)
@@ -136,7 +136,7 @@ public class FragmentQueryBuilder {
                     final String leafValueAsText = leaf.getValue().toString();
                     sqlStringBuilder.append("attributes ->> '").append(leaf.getName()).append("'");
                     sqlStringBuilder.append(" = '");
-                    sqlStringBuilder.append(leafValueAsText);
+                    sqlStringBuilder.append(EscapeUtils.escapeForSqlStringLiteral(leafValueAsText));
                     sqlStringBuilder.append("'");
                 } else {
                     throw new CpsPathException(" can use only " + nextComparativeOperator + " with integer ");
index 139a8b3..4c7971e 100644 (file)
@@ -31,6 +31,7 @@ import javax.persistence.EntityManager;
 import javax.persistence.PersistenceContext;
 import lombok.AllArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
+import org.onap.cps.spi.utils.EscapeUtils;
 import org.springframework.stereotype.Component;
 import org.springframework.transaction.annotation.Transactional;
 
@@ -86,7 +87,7 @@ public class TempTableCreator {
         final Collection<String> sqlInserts = new HashSet<>(sqlData.size());
         for (final Collection<String> rowValues : sqlData) {
             final Collection<String> escapedValues =
-                rowValues.stream().map(it -> escapeSingleQuotesByDoublingThem(it)).collect(Collectors.toList());
+                rowValues.stream().map(EscapeUtils::escapeForSqlStringLiteral).collect(Collectors.toList());
             sqlInserts.add("('" + String.join("','", escapedValues) + "')");
         }
         sqlStringBuilder.append("INSERT INTO ");
@@ -98,8 +99,4 @@ public class TempTableCreator {
         sqlStringBuilder.append(";");
     }
 
-    private static String escapeSingleQuotesByDoublingThem(final String value) {
-        return value.replace("'", "''");
-    }
-
 }
index 3092b79..2b61d39 100644 (file)
@@ -26,8 +26,12 @@ import lombok.NoArgsConstructor;
 @NoArgsConstructor(access = AccessLevel.PRIVATE)
 public class EscapeUtils {
 
-    public static String escapeForSqlLike(final String text) {
-        return text.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_");
+    public static String escapeForSqlLike(final String value) {
+        return value.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_");
+    }
+
+    public static String escapeForSqlStringLiteral(final String value) {
+        return value.replace("'", "''");
     }
 
 }
index 4e6986e..f76c5ba 100644 (file)
@@ -56,3 +56,5 @@ databaseChangeLog:
       file: changelog/db/changes/19-delete-not-required-dataspace-id-from-fragment.yaml
   - include:
       file: changelog/db/changes/20-change-foreign-key-id-types-to-integer.yaml
+  - include:
+      file: changelog/db/changes/21-escape-quotes-in-xpath.yaml
diff --git a/cps-ri/src/main/resources/changelog/db/changes/21-escape-quotes-in-xpath-forward.sql b/cps-ri/src/main/resources/changelog/db/changes/21-escape-quotes-in-xpath-forward.sql
new file mode 100644 (file)
index 0000000..9bf7f9a
--- /dev/null
@@ -0,0 +1,19 @@
+/*
+   ============LICENSE_START=======================================================
+    Copyright (C) 2023 Nordix Foundation.
+   ================================================================================
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+        http://www.apache.org/licenses/LICENSE-2.0
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+   SPDX-License-Identifier: Apache-2.0
+   ============LICENSE_END=========================================================
+*/
+
+-- replace \' with '' and "" with "
+UPDATE fragment SET xpath = replace(replace(xpath, $$\'$$, $$''$$), '""', '"');
\ No newline at end of file
diff --git a/cps-ri/src/main/resources/changelog/db/changes/21-escape-quotes-in-xpath-rollback.sql b/cps-ri/src/main/resources/changelog/db/changes/21-escape-quotes-in-xpath-rollback.sql
new file mode 100644 (file)
index 0000000..0fd1633
--- /dev/null
@@ -0,0 +1,19 @@
+/*
+   ============LICENSE_START=======================================================
+    Copyright (C) 2023 Nordix Foundation.
+   ================================================================================
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+        http://www.apache.org/licenses/LICENSE-2.0
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+   SPDX-License-Identifier: Apache-2.0
+   ============LICENSE_END=========================================================
+*/
+
+-- replace '' with \' and " with ""
+UPDATE fragment SET xpath = replace(replace(xpath, $$''$$, $$\'$$), '"', '""');
\ No newline at end of file
diff --git a/cps-ri/src/main/resources/changelog/db/changes/21-escape-quotes-in-xpath.yaml b/cps-ri/src/main/resources/changelog/db/changes/21-escape-quotes-in-xpath.yaml
new file mode 100644 (file)
index 0000000..7b5b1db
--- /dev/null
@@ -0,0 +1,29 @@
+# ============LICENSE_START=======================================================
+# Copyright (C) 2023 Nordix Foundation.
+# ================================================================================
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#       http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# SPDX-License-Identifier: Apache-2.0
+# ============LICENSE_END=========================================================
+
+databaseChangeLog:
+
+  - changeSet:
+      id: 21
+      author: cps
+      changes:
+        - sqlFile:
+            path: changelog/db/changes/21-escape-quotes-in-xpath-forward.sql
+      rollback:
+        - sqlFile:
+            path: changelog/db/changes/21-escape-quotes-in-xpath-rollback.sql
index 7de9b97..52330e6 100644 (file)
@@ -24,7 +24,7 @@ import spock.lang.Specification
 
 class EscapeUtilsSpec extends Specification {
 
-    def 'Escape text for using in SQL LIKE operation'() {
+    def 'Escape text for use in SQL LIKE operation.'() {
         expect: 'SQL LIKE special characters to be escaped with forward-slash'
             assert EscapeUtils.escapeForSqlLike(unescapedText) == escapedText
         where:
@@ -33,4 +33,9 @@ class EscapeUtilsSpec extends Specification {
             'Others (./?$) are not special' || 'Others (./?$) are not special'
     }
 
+    def 'Escape text for use in SQL string literal.'() {
+        expect: 'single quotes to be doubled'
+            assert EscapeUtils.escapeForSqlStringLiteral("I'm escaping!") == "I''m escaping!"
+    }
+
 }
index 7da4024..f00f944 100644 (file)
@@ -253,7 +253,7 @@ public class YangUtils {
         final List<String> keyAttributes = nodeIdentifier.entrySet().stream().map(
                 entry -> {
                     final String name = entry.getKey().getLocalName();
-                    final String value = String.valueOf(entry.getValue()).replace("'", "\\'");
+                    final String value = String.valueOf(entry.getValue()).replace("'", "''");
                     return String.format("@%s='%s'", name, value);
                 }
         ).collect(Collectors.toList());
index 796eb7f..6611789 100644 (file)
@@ -177,6 +177,7 @@ General Notes
 =============
 
 - String values must be wrapped in quotation marks ``"`` (U+0022) or apostrophes ``'`` (U+0027).
+- Quotations marks and apostrophes can be escaped by doubling them in their respective quotes, for example ``'CPS ''Path'' Query' -> CPS 'Path' Query``
 - String comparisons are case sensitive.
 
 Query Syntax
@@ -247,7 +248,6 @@ leaf-conditions
   - The key should be supplied with correct data type for it to be queried from DB. In the last example above the attribute code is of type
     Integer so the cps query will not work if the value is passed as string.
     eg: ``//categories[@code="1"]`` or ``//categories[@code='1']`` will not work because the key attribute code is treated a string.
-  - Having '[' token in any index in any list will have a negative impact on this function.
 
 **Notes**
   - For performance reasons it does not make sense to query using key leaf as attribute. If the key value is known it is better to execute a get request with the complete xpath.
@@ -272,7 +272,6 @@ The text()-condition  can be added to any CPS path query.
   - Only string and integer values are supported, boolean and float values are not supported.
   - Since CPS cannot return individual leaves it will always return the container with all its leaves. Ancestor-axis can be used to specify a parent higher up the tree.
   - When querying a leaf value (instead of leaf-list) it is better, more performant to use a text value condition use @<leaf-name> as described above.
-  - Having '[' token in any index in any list will have a negative impact on this function.
 
 contains()-condition
 --------------------
index f56b34a..25f6d22 100755 (executable)
@@ -42,6 +42,7 @@ Bug Fixes
 
 Features
 --------
+    - `CPS-1760 <https://jira.onap.org/browse/CPS-1760>`_ Improve handling of special characters in Cps Paths
 
 Version: 3.3.4
 ==============
index 53737fb..74496d3 100644 (file)
@@ -356,4 +356,23 @@ class CpsQueryServiceIntegrationSpec extends FunctionalSpecBase {
             'text-condition'     || "/bookstore/categories[@code='1']/books/title[text()='[@hello=world]']"
             'contains-condition' || "/bookstore/categories[@code='1']/books[contains(@title, '[@hello=world]')]"
     }
+
+    def 'Cps Path get and query can handle apostrophe inside #quotes.'() {
+        given: 'a book with special characters in title'
+            cpsDataService.saveData(FUNCTIONAL_TEST_DATASPACE_1, BOOKSTORE_ANCHOR_1, "/bookstore/categories[@code='1']",
+                    '{"books": [ {"title":"I\'m escaping"} ] }', OffsetDateTime.now())
+        when: 'a query is executed'
+            def result = objectUnderTest.queryDataNodes(FUNCTIONAL_TEST_DATASPACE_1, BOOKSTORE_ANCHOR_1, cpsPath, OMIT_DESCENDANTS)
+        then: 'the node is returned'
+            assert result.size() == 1
+            assert result[0].xpath == "/bookstore/categories[@code='1']/books[@title='I''m escaping']"
+        cleanup: 'the new datanode'
+            cpsDataService.deleteDataNode(FUNCTIONAL_TEST_DATASPACE_1, BOOKSTORE_ANCHOR_1, "/bookstore/categories[@code='1']/books[@title='I''m escaping']", OffsetDateTime.now())
+        where:
+            quotes               || cpsPath
+            'single quotes'      || "/bookstore/categories[@code='1']/books[@title='I''m escaping']"
+            'double quotes'      || '/bookstore/categories[@code="1"]/books[@title="I\'m escaping"]'
+            'text-condition'     || "/bookstore/categories[@code='1']/books/title[text()='I''m escaping']"
+            'contains-condition' || "/bookstore/categories[@code='1']/books[contains(@title, 'I''m escaping')]"
+    }
 }