fix: Json smart substitution breaks when evaluated value contains the character '?' (#7031)

* Minor refactoring

* Partial code change to replace question mark with appsmith placeholder

* Working version

* Removing unnecessary code

* Added test case to assert that when evaluated value contains a "?", the replacements are still correct

* Added test case in Mongo Plugin as well for the same scenario

* Minor change in the language of the comment

app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/DataTypeStringUtils.java

@@ -36,6 +36,7 @@ import java.util.Map;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import static com.appsmith.external.helpers.SmartSubstitutionHelper.APPSMITH_SUBSTITUTION_PLACEHOLDER;
 import static org.apache.commons.lang3.ClassUtils.isPrimitiveOrWrapper;
 
 @Slf4j
@@ -45,6 +46,8 @@ public class DataTypeStringUtils {
 
     private static Pattern questionPattern = Pattern.compile(regexForQuestionMark);
 
+    private static Pattern placeholderPattern = Pattern.compile(APPSMITH_SUBSTITUTION_PLACEHOLDER);
+
     private static ObjectMapper objectMapper = new ObjectMapper();
 
     private static JSONParser parser = new JSONParser(JSONParser.MODE_PERMISSIVE);
@@ -183,7 +186,7 @@ public class DataTypeStringUtils {
         return DataType.STRING;
     }
 
-    public static String jsonSmartReplacementQuestionWithValue(String input,
+    public static String jsonSmartReplacementPlaceholderWithValue(String input,
                                                                   String replacement,
                                                                   List<Map.Entry<String, String>> insertedParams) {
 
@@ -199,12 +202,12 @@ public class DataTypeStringUtils {
             case DOUBLE:
             case NULL:
             case BOOLEAN:
-                input = questionPattern.matcher(input).replaceFirst(String.valueOf(replacement));
+                input = placeholderPattern.matcher(input).replaceFirst(String.valueOf(replacement));
                 break;
             case ARRAY:
                 try {
                     JSONArray jsonArray = (JSONArray) parser.parse(replacement);
-                    input = questionPattern.matcher(input).replaceFirst(String.valueOf(objectMapper.writeValueAsString(jsonArray)));
+                    input = placeholderPattern.matcher(input).replaceFirst(String.valueOf(objectMapper.writeValueAsString(jsonArray)));
                 } catch (net.minidev.json.parser.ParseException | JsonProcessingException e) {
                     throw Exceptions.propagate(
                             new AppsmithPluginException(
@@ -220,7 +223,7 @@ public class DataTypeStringUtils {
                     JSONObject jsonObject = (JSONObject) parser.parse(replacement);
                     String jsonString = String.valueOf(objectMapper.writeValueAsString(jsonObject));
                     // Adding Matcher.quoteReplacement so that "/" and "$" in the string are escaped during replacement
-                    input = questionPattern.matcher(input).replaceFirst(Matcher.quoteReplacement(jsonString));
+                    input = placeholderPattern.matcher(input).replaceFirst(Matcher.quoteReplacement(jsonString));
                 } catch (net.minidev.json.parser.ParseException | JsonProcessingException e) {
                     throw Exceptions.propagate(
                             new AppsmithPluginException(
@@ -232,7 +235,7 @@ public class DataTypeStringUtils {
                 }
                 break;
             case BSON:
-                input = questionPattern.matcher(input).replaceFirst(Matcher.quoteReplacement(replacement));
+                input = placeholderPattern.matcher(input).replaceFirst(Matcher.quoteReplacement(replacement));
                 break;
             case DATE:
             case TIME:
@@ -244,7 +247,7 @@ public class DataTypeStringUtils {
                 try {
                     replacement = escapeSpecialCharacters(replacement);
                     String valueAsString = objectMapper.writeValueAsString(replacement);
-                    input = questionPattern.matcher(input).replaceFirst(Matcher.quoteReplacement(valueAsString));
+                    input = placeholderPattern.matcher(input).replaceFirst(Matcher.quoteReplacement(valueAsString));
                 } catch (JsonProcessingException e) {
                     throw Exceptions.propagate(
                             new AppsmithPluginException(

app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/MustacheHelper.java

@@ -18,13 +18,13 @@ import java.util.List;
 import java.util.Map;
 import java.util.Queue;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Function;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 import static com.appsmith.external.helpers.BeanCopyUtils.isDomainModel;
+import static com.appsmith.external.helpers.SmartSubstitutionHelper.APPSMITH_SUBSTITUTION_PLACEHOLDER;
 
 @Slf4j
 public class MustacheHelper {
@@ -38,12 +38,21 @@ public class MustacheHelper {
     private final static Pattern pattern = Pattern.compile("[a-zA-Z_][a-zA-Z0-9._]*");
     /**
      * Appsmith smart replacement : The regex pattern below looks for '?' or "?". This pattern is later replaced with ?
-     * to fit the requirements of prepared statements/Appsmith's JSON smart replacement.
+     * to fit the requirements of prepared statements.
      */
     private static String regexQuotesTrimming = "([\"']\\?[\"'])";
     private static Pattern quoteQuestionPattern = Pattern.compile(regexQuotesTrimming);
     // The final replacement string of ? for replacing '?' or "?"
     private static String postQuoteTrimmingQuestionMark = "\\?";
+
+    /**
+     * Appsmith smart replacement with placeholder : The regex pattern below looks for `APPSMITH_SUBSTITUTION_PLACEHOLDER`
+     * surrounded by quotes. This pattern is later replaced with just APPSMITH_SUBSTITUTION_PLACEHOLDER to fit the requirements
+     * of JSON smart replacement aka trim the quotes present.
+     */
+    private static String regexPlaceholderTrimming = "([\"']" + APPSMITH_SUBSTITUTION_PLACEHOLDER + "[\"'])";
+    private static Pattern placeholderTrimmingPattern = Pattern.compile(regexPlaceholderTrimming);
+
     private static String laxMustacheBindingRegex = "\\{\\{([\\s\\S]*?)\\}\\}";
     private static Pattern laxMustacheBindingPattern = Pattern.compile(laxMustacheBindingRegex);
 
@@ -297,7 +306,7 @@ public class MustacheHelper {
 
         } else if (object instanceof Map) {
             Map renderedMap = new HashMap();
-            for (Object entry : ((Map)object).entrySet()) {
+            for (Object entry : ((Map) object).entrySet()) {
                 renderedMap.put(
                         ((Map.Entry) entry).getKey(), // key
                         renderFieldValues(((Map.Entry) entry).getValue(), context) // value
@@ -349,8 +358,19 @@ public class MustacheHelper {
         }
     }
 
+    public static String replaceMustacheWithPlaceholder(String query, List<String> mustacheBindings) {
+        return replaceMustacheUsingPatterns(query, APPSMITH_SUBSTITUTION_PLACEHOLDER, mustacheBindings,
+                placeholderTrimmingPattern, APPSMITH_SUBSTITUTION_PLACEHOLDER);
+    }
+
     public static String replaceMustacheWithQuestionMark(String query, List<String> mustacheBindings) {
 
+        return replaceMustacheUsingPatterns(query, "?", mustacheBindings,
+                quoteQuestionPattern, postQuoteTrimmingQuestionMark);
+    }
+
+    private static String replaceMustacheUsingPatterns(String query, String placeholder, List<String> mustacheBindings,
+                                                       Pattern sanitizePattern, String replacement) {
         ActionConfiguration actionConfiguration = new ActionConfiguration();
         actionConfiguration.setBody(query);
 
@@ -359,33 +379,18 @@ public class MustacheHelper {
 
         Map<String, String> replaceParamsMap = mustacheSet
                 .stream()
-                .collect(Collectors.toMap(Function.identity(), v -> "?"));
+                .collect(Collectors.toMap(Function.identity(), v -> placeholder));
 
+        // Replace the mustaches with the values mapped to each mustache in replaceParamsMap
         ActionConfiguration updatedActionConfiguration = renderFieldValues(actionConfiguration, replaceParamsMap);
 
         String body = updatedActionConfiguration.getBody();
 
-        // Trim the quotes around ? if present
+        body = sanitizePattern.matcher(body).replaceAll(replacement);
-        body = quoteQuestionPattern.matcher(body).replaceAll(postQuoteTrimmingQuestionMark);
 
         return body;
     }
 
-    public static String replaceQuestionMarkWithDollarIndex(String query) {
-        final AtomicInteger counter = new AtomicInteger();
-        String updatedQuery = query.chars()
-                .mapToObj(c -> {
-                    if (c == '?') {
-                        return "$" + counter.incrementAndGet();
-                    }
-
-                    return Character.toString(c);
-                })
-                .collect(Collectors.joining());
-
-        return updatedQuery;
-    }
-
     public static Boolean laxIsBindingPresentInString(String input) {
         return laxMustacheBindingPattern.matcher(input).find();
     }

app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/SmartSubstitutionHelper.java

@@ -0,0 +1,25 @@
+package com.appsmith.external.helpers;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+public class SmartSubstitutionHelper {
+
+    public static final String APPSMITH_SUBSTITUTION_PLACEHOLDER = "#_appsmith_placeholder#";
+
+    public static String replaceQuestionMarkWithDollarIndex(String query) {
+        final AtomicInteger counter = new AtomicInteger();
+        String updatedQuery = query.chars()
+                .mapToObj(c -> {
+                    if (c == '?') {
+                        return "$" + counter.incrementAndGet();
+                    }
+
+                    return Character.toString(c);
+                })
+                .collect(Collectors.joining());
+
+        return updatedQuery;
+    }
+
+}

app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java

@@ -422,7 +422,7 @@ public class MongoPlugin extends BasePlugin {
             // First extract all the bindings in order
             List<String> mustacheKeysInOrder = MustacheHelper.extractMustacheKeysInOrder(rawQuery);
             // Replace all the bindings with a ? as expected in a prepared statement.
-            String updatedQuery = MustacheHelper.replaceMustacheWithQuestionMark(rawQuery, mustacheKeysInOrder);
+            String updatedQuery = MustacheHelper.replaceMustacheWithPlaceholder(rawQuery, mustacheKeysInOrder);
 
             updatedQuery = (String) smartSubstitutionOfBindings(updatedQuery,
                     mustacheKeysInOrder,
@@ -880,7 +880,7 @@ public class MongoPlugin extends BasePlugin {
                                              List<Map.Entry<String, String>> insertedParams,
                                              Object... args) {
             String jsonBody = (String) input;
-            return DataTypeStringUtils.jsonSmartReplacementQuestionWithValue(jsonBody, value, insertedParams);
+            return DataTypeStringUtils.jsonSmartReplacementPlaceholderWithValue(jsonBody, value, insertedParams);
         }
 
         @Override

app/server/appsmith-plugins/mongoPlugin/src/test/java/com/external/plugins/MongoPluginTest.java

@@ -1637,4 +1637,58 @@ public class MongoPluginTest {
                 .verifyComplete();
     }
 
+    @Test
+    public void testSmartSubstitutionEvaluatedValueContainingQuestionMark() {
+        ActionConfiguration actionConfiguration = new ActionConfiguration();
+
+        Map<Integer, Object> configMap = new HashMap<>();
+        configMap.put(SMART_BSON_SUBSTITUTION, Boolean.TRUE);
+        configMap.put(COMMAND, "INSERT");
+        configMap.put(COLLECTION, "users");
+        configMap.put(INSERT_DOCUMENT, "{\"name\" : {{Input1.text}}, \"gender\" : {{Input2.text}}, \"age\" : 40, \"tag\" : \"test\"}");
+
+        actionConfiguration.setPluginSpecifiedTemplates(generateMongoFormConfigTemplates(configMap));
+
+        DatasourceConfiguration dsConfig = createDatasourceConfiguration();
+        Mono<MongoClient> dsConnectionMono = pluginExecutor.datasourceCreate(dsConfig);
+
+        ExecuteActionDTO executeActionDTO = new ExecuteActionDTO();
+        List<Param> params = new ArrayList<>();
+        Param param1 = new Param();
+        param1.setKey("Input1.text");
+        param1.setValue("This string contains ? symbol");
+        params.add(param1);
+        Param param3 = new Param();
+        param3.setKey("Input2.text");
+        param3.setValue("F");
+        params.add(param3);
+        executeActionDTO.setParams(params);
+
+        Mono<Object> executeMono = dsConnectionMono.flatMap(conn -> pluginExecutor.executeParameterized(conn, executeActionDTO, dsConfig, actionConfiguration));
+        StepVerifier.create(executeMono)
+                .assertNext(obj -> {
+                    ActionExecutionResult result = (ActionExecutionResult) obj;
+                    assertNotNull(result);
+                    assertTrue(result.getIsExecutionSuccess());
+                    assertNotNull(result.getBody());
+                    assertEquals(
+                            List.of(new ParsedDataType(JSON), new ParsedDataType(RAW)).toString(),
+                            result.getDataTypes().toString()
+                    );
+                })
+                .verifyComplete();
+
+        // Clean up this newly inserted value
+        configMap = new HashMap<>();
+        configMap.put(SMART_BSON_SUBSTITUTION, Boolean.FALSE);
+        configMap.put(COMMAND, "DELETE");
+        configMap.put(COLLECTION, "users");
+        configMap.put(DELETE_QUERY, "{\"tag\" : \"test\"}");
+        configMap.put(DELETE_LIMIT, "ALL");
+
+        actionConfiguration.setPluginSpecifiedTemplates(generateMongoFormConfigTemplates(configMap));
+        // Run the delete command
+        dsConnectionMono.flatMap(conn -> pluginExecutor.executeParameterized(conn, new ExecuteActionDTO(), dsConfig, actionConfiguration)).block();
+    }
+
 }

app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java

@@ -60,7 +60,7 @@ import java.util.Set;
 import java.util.stream.IntStream;
 
 import static com.appsmith.external.constants.ActionConstants.ACTION_CONFIGURATION_BODY;
-import static com.appsmith.external.helpers.MustacheHelper.replaceQuestionMarkWithDollarIndex;
+import static com.appsmith.external.helpers.SmartSubstitutionHelper.replaceQuestionMarkWithDollarIndex;
 import static com.appsmith.external.helpers.PluginUtils.getColumnsListForJdbcPlugin;
 import static com.appsmith.external.helpers.PluginUtils.getIdenticalColumns;
 import static com.appsmith.external.helpers.PluginUtils.getPSParamLabel;

app/server/appsmith-plugins/mysqlPlugin/src/main/java/com/external/plugins/MySqlPlugin.java

@@ -64,7 +64,7 @@ import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 
 import static com.appsmith.external.constants.ActionConstants.ACTION_CONFIGURATION_BODY;
-import static com.appsmith.external.helpers.MustacheHelper.replaceQuestionMarkWithDollarIndex;
+import static com.appsmith.external.helpers.SmartSubstitutionHelper.replaceQuestionMarkWithDollarIndex;
 import static com.appsmith.external.helpers.PluginUtils.getIdenticalColumns;
 import static com.appsmith.external.helpers.PluginUtils.getPSParamLabel;
 import static io.r2dbc.spi.ConnectionFactoryOptions.SSL;

app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java

@@ -68,7 +68,7 @@ import java.util.stream.IntStream;
 import java.util.stream.Stream;
 
 import static com.appsmith.external.constants.ActionConstants.ACTION_CONFIGURATION_BODY;
-import static com.appsmith.external.helpers.MustacheHelper.replaceQuestionMarkWithDollarIndex;
+import static com.appsmith.external.helpers.SmartSubstitutionHelper.replaceQuestionMarkWithDollarIndex;
 import static com.appsmith.external.helpers.PluginUtils.getColumnsListForJdbcPlugin;
 import static com.appsmith.external.helpers.PluginUtils.getIdenticalColumns;
 import static com.appsmith.external.helpers.PluginUtils.getPSParamLabel;

app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java

@@ -154,7 +154,7 @@ public class RestApiPlugin extends BasePlugin {
                     // First extract all the bindings in order
                     List<String> mustacheKeysInOrder = MustacheHelper.extractMustacheKeysInOrder(actionConfiguration.getBody());
                     // Replace all the bindings with a ? as expected in a prepared statement.
-                    String updatedBody = MustacheHelper.replaceMustacheWithQuestionMark(actionConfiguration.getBody(), mustacheKeysInOrder);
+                    String updatedBody = MustacheHelper.replaceMustacheWithPlaceholder(actionConfiguration.getBody(), mustacheKeysInOrder);
 
                     try {
                         updatedBody = (String) smartSubstitutionOfBindings(updatedBody,
@@ -668,7 +668,7 @@ public class RestApiPlugin extends BasePlugin {
                                              List<Map.Entry<String, String>> insertedParams,
                                              Object... args) {
             String jsonBody = (String) input;
-            return DataTypeStringUtils.jsonSmartReplacementQuestionWithValue(jsonBody, value, insertedParams);
+            return DataTypeStringUtils.jsonSmartReplacementPlaceholderWithValue(jsonBody, value, insertedParams);
         }
 
         @Override

app/server/appsmith-plugins/restApiPlugin/src/test/java/com/external/plugins/RestApiPluginTest.java

@@ -505,4 +505,53 @@ public class RestApiPluginTest {
                 .verifyComplete();
     }
 
+    @Test
+    public void testSmartSubstitutionEvaluatedValueContainingQuestionMark() {
+        DatasourceConfiguration dsConfig = new DatasourceConfiguration();
+        dsConfig.setUrl("https://postman-echo.com/post");
+
+        ActionConfiguration actionConfig = new ActionConfiguration();
+        actionConfig.setHeaders(List.of(new Property("content-type", "application/json")));
+        actionConfig.setHttpMethod(HttpMethod.POST);
+        String requestBody = "{\n" +
+                "\t\"name\" : {{Input1.text}},\n" +
+                "\t\"email\" : {{Input2.text}},\n" +
+                "}";
+        actionConfig.setBody(requestBody);
+        List<Property> pluginSpecifiedTemplates = new ArrayList<>();
+        pluginSpecifiedTemplates.add(new Property("jsonSmartSubstitution", "true"));
+        actionConfig.setPluginSpecifiedTemplates(pluginSpecifiedTemplates);
+
+        ExecuteActionDTO executeActionDTO = new ExecuteActionDTO();
+        List<Param> params = new ArrayList<>();
+        Param param1 = new Param();
+        param1.setKey("Input1.text");
+        param1.setValue("this is a string with a ? ");
+        params.add(param1);
+        Param param2 = new Param();
+        param2.setKey("Input2.text");
+        param2.setValue("email@email.com");
+        params.add(param2);
+        executeActionDTO.setParams(params);
+
+        Mono<ActionExecutionResult> resultMono = pluginExecutor.executeParameterized(null, executeActionDTO, dsConfig, actionConfig);
+        StepVerifier.create(resultMono)
+                .assertNext(result -> {
+                    assertTrue(result.getIsExecutionSuccess());
+                    assertNotNull(result.getBody());
+                    String resultBody = "{\"name\":\"this is a string with a ? \",\"email\":\"email@email.com\"}";
+                    JSONParser jsonParser = new JSONParser(JSONParser.MODE_PERMISSIVE);
+                    ObjectMapper objectMapper = new ObjectMapper();
+                    try {
+                        JSONObject resultJson = (JSONObject) jsonParser.parse(String.valueOf(result.getBody()));
+                        Object resultData = resultJson.get("json");
+                        String parsedJsonAsString = objectMapper.writeValueAsString(resultData);
+                        assertEquals(resultBody, parsedJsonAsString);
+                    } catch (ParseException | JsonProcessingException e) {
+                        e.printStackTrace();
+                    }
+                })
+                .verifyComplete();
+    }
+
 }