From 1af27835a70d7489a222148e14240d0cb1d48edc Mon Sep 17 00:00:00 2001 From: vikasrathee-cs Date: Mon, 9 Dec 2024 18:29:32 +0530 Subject: [PATCH] Added doNotSkipInvalidMacroForFunctions in StageValidationRequest to throw error in case of invalid oauth macro. --- .../cdap/api/macro/MacroParserOptions.java | 23 ++++++++++++-- .../app/runtime/plugin/MacroParser.java | 4 ++- .../app/runtime/plugin/MacroParserTest.java | 24 ++++++++++++++ .../datapipeline/service/OAuthHandler.java | 4 +-- .../service/RemoteValidationTask.java | 5 ++- .../service/ValidationHandler.java | 6 +++- .../datapipeline/DataPipelineServiceTest.java | 30 ++++++++++++++++++ .../v2/validation/StageValidationRequest.java | 31 ++++++++++++++++++- 8 files changed, 118 insertions(+), 9 deletions(-) diff --git a/cdap-api/src/main/java/io/cdap/cdap/api/macro/MacroParserOptions.java b/cdap-api/src/main/java/io/cdap/cdap/api/macro/MacroParserOptions.java index 9a9e643ba4ed..6287718e067d 100644 --- a/cdap-api/src/main/java/io/cdap/cdap/api/macro/MacroParserOptions.java +++ b/cdap-api/src/main/java/io/cdap/cdap/api/macro/MacroParserOptions.java @@ -33,16 +33,18 @@ public class MacroParserOptions { private final boolean skipInvalid; private final int maxRecurseDepth; private final Set functionWhitelist; + private final Set doNotSkipInvalidMacroForFunctions; private MacroParserOptions(boolean evaluateLookups, boolean evaluateFunctions, boolean escapingEnabled, boolean skipInvalid, - int maxRecurseDepth, Set functionWhitelist) { + int maxRecurseDepth, Set functionWhitelist, Set doNotSkipInvalidMacroForFunctions) { this.evaluateLookups = evaluateLookups; this.evaluateFunctions = evaluateFunctions; this.escapingEnabled = escapingEnabled; this.maxRecurseDepth = maxRecurseDepth; this.skipInvalid = skipInvalid; this.functionWhitelist = functionWhitelist; + this.doNotSkipInvalidMacroForFunctions = doNotSkipInvalidMacroForFunctions; } public boolean shouldEvaluateLookups() { @@ -69,6 +71,10 @@ public Set getFunctionWhitelist() { return functionWhitelist; } + public Set getDoNotSkipInvalidMacroForFunctions() { + return doNotSkipInvalidMacroForFunctions; + } + /** * @return Builder to create options */ @@ -87,6 +93,7 @@ public static class Builder { private boolean skipInvalid; private int maxRecurseDepth = 10; private final Set functionWhitelist = new HashSet<>(); + private final Set doNotSkipInvalidMacroForFunctions = new HashSet<>(); public Builder disableLookups() { evaluateLookups = false; @@ -113,6 +120,16 @@ public Builder setMaxRecurseDepth(int maxRecurseDepth) { return this; } + public Builder setDoNotSkipInvalidMacroForFunctions(Collection whitelist) { + doNotSkipInvalidMacroForFunctions.clear(); + doNotSkipInvalidMacroForFunctions.addAll(whitelist); + return this; + } + + public Builder setDoNotSkipInvalidMacroForFunctions(String... whitelist) { + return setDoNotSkipInvalidMacroForFunctions(Arrays.asList(whitelist)); + } + public Builder setFunctionWhitelist(Collection whitelist) { functionWhitelist.clear(); functionWhitelist.addAll(whitelist); @@ -122,10 +139,10 @@ public Builder setFunctionWhitelist(Collection whitelist) { public Builder setFunctionWhitelist(String... whitelist) { return setFunctionWhitelist(Arrays.asList(whitelist)); } - + public MacroParserOptions build() { return new MacroParserOptions(evaluateLookups, evaluateFunctions, escapingEnabled, - skipInvalid, maxRecurseDepth, functionWhitelist); + skipInvalid, maxRecurseDepth, functionWhitelist, doNotSkipInvalidMacroForFunctions); } } } diff --git a/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/plugin/MacroParser.java b/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/plugin/MacroParser.java index 0a02e8c8ccc6..837e20ce576b 100644 --- a/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/plugin/MacroParser.java +++ b/cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/runtime/plugin/MacroParser.java @@ -43,6 +43,7 @@ public class MacroParser { private final boolean functionsEnabled; private final boolean skipInvalid; private final Set functionWhitelist; + private final Set doNotSkipInvalidMacroForFunctions; public MacroParser(MacroEvaluator macroEvaluator) { this(macroEvaluator, MacroParserOptions.DEFAULT); @@ -55,6 +56,7 @@ public MacroParser(MacroEvaluator macroEvaluator, MacroParserOptions options) { this.functionsEnabled = options.shouldEvaluateFunctions(); this.functionWhitelist = options.getFunctionWhitelist(); this.skipInvalid = options.shouldSkipInvalid(); + this.doNotSkipInvalidMacroForFunctions = options.getDoNotSkipInvalidMacroForFunctions(); } /** @@ -242,7 +244,7 @@ private MacroMetadata getMacroFunctionMetadata(int startIndex, int endIndex, Str } } catch (InvalidMacroException e) { - if (!skipInvalid) { + if (!skipInvalid || doNotSkipInvalidMacroForFunctions.contains(macroFunction)) { throw e; } } diff --git a/cdap-app-fabric/src/test/java/io/cdap/cdap/internal/app/runtime/plugin/MacroParserTest.java b/cdap-app-fabric/src/test/java/io/cdap/cdap/internal/app/runtime/plugin/MacroParserTest.java index ab7cde5fcac6..b8dba1b65c57 100644 --- a/cdap-app-fabric/src/test/java/io/cdap/cdap/internal/app/runtime/plugin/MacroParserTest.java +++ b/cdap-app-fabric/src/test/java/io/cdap/cdap/internal/app/runtime/plugin/MacroParserTest.java @@ -562,4 +562,28 @@ private static void assertSubstitution(String macro, String expected, Map doNotSkipInvalidMacroForFunctions = validationRequest.getDoNotSkipInvalidMacroForFunctions(); MacroParserOptions macroParserOptions = MacroParserOptions.builder() - .skipInvalidMacros() .setEscaping(false) .setFunctionWhitelist(evaluators.keySet()) + .skipInvalidMacros() + .setDoNotSkipInvalidMacroForFunctions(doNotSkipInvalidMacroForFunctions) .build(); Function, Map> macroFn = macroProperties -> systemAppContext diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/ValidationHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/ValidationHandler.java index 96c9cad214c5..4613fe637bc4 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/ValidationHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/ValidationHandler.java @@ -61,6 +61,7 @@ import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.Map; +import java.util.Set; import java.util.function.Function; import javax.ws.rs.GET; import javax.ws.rs.POST; @@ -169,11 +170,14 @@ OAuthAccessTokenMacroEvaluator.FUNCTION_NAME, new OAuthAccessTokenMacroEvaluator ); MacroEvaluator macroEvaluator = new DefaultMacroEvaluator(new BasicArguments(arguments), evaluators, DefaultMacroEvaluator.MAP_FUNCTIONS); + Set doNotSkipInvalidMacroForFunctions = validationRequest.getDoNotSkipInvalidMacroForFunctions(); MacroParserOptions macroParserOptions = MacroParserOptions.builder() - .skipInvalidMacros() .setEscaping(false) .setFunctionWhitelist(evaluators.keySet()) + .skipInvalidMacros() + .setDoNotSkipInvalidMacroForFunctions(doNotSkipInvalidMacroForFunctions) .build(); + Function, Map> macroFn = macroProperties -> getContext().evaluateMacros(namespace, macroProperties, macroEvaluator, macroParserOptions); String validationResponse = GSON.toJson(ValidationUtils.validate( diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/DataPipelineServiceTest.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/DataPipelineServiceTest.java index 5ec6eea3fe55..f69a42adb497 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/DataPipelineServiceTest.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/DataPipelineServiceTest.java @@ -484,4 +484,34 @@ private StageValidationResponse sendRequest(StageValidationRequest requestBody) Assert.assertEquals(200, response.getResponseCode()); return GSON.fromJson(response.getResponseBodyAsString(), StageValidationResponse.class); } + + @Test + public void testValidationForInvalidMacrosForFunctions() throws Exception { + String stageName = "invalidMacro"; + Map properties = + Collections.singletonMap("metadataOperations", "${oauthAccessToken(provider,credential)}"); + ETLStage stage = new ETLStage(stageName, new ETLPlugin(MockSource.NAME, BatchSource.PLUGIN_TYPE, properties)); + + // In case doNotSkipInvalidMacroForFunctions is set in StageValidationRequest, + // it will not skip invalid macro given in the list and will throw the exact exception + HttpResponse withDoNotSkipInvalidMacroForFunctionsResponse = sendRequestForInvalidMacros( + new StageValidationRequest(stage, Collections.emptyList(), false, "oauthAccessToken,oauth")); + Assert.assertEquals(500, withDoNotSkipInvalidMacroForFunctionsResponse.getResponseCode()); + + // In case doNotSkipInvalidMacroForFunctions is not set in StageValidationRequest, + // it will skip invalid macro and will not throw any exception + HttpResponse withoutDoNotSkipInvalidMacroForFunctionsResponse = sendRequestForInvalidMacros( + new StageValidationRequest(stage, Collections.emptyList(), false)); + Assert.assertEquals(200, withoutDoNotSkipInvalidMacroForFunctionsResponse.getResponseCode()); + } + + private HttpResponse sendRequestForInvalidMacros(StageValidationRequest requestBody) throws IOException { + URL validatePipelineURL = serviceURI + .resolve(String.format("v1/contexts/%s/validations/stage", NamespaceId.DEFAULT.getNamespace())) + .toURL(); + HttpRequest request = HttpRequest.builder(HttpMethod.POST, validatePipelineURL) + .withBody(GSON.toJson(requestBody)) + .build(); + return HttpRequests.execute(request, new DefaultHttpRequestConfig(false)); + } } diff --git a/cdap-app-templates/cdap-etl/cdap-etl-proto/src/main/java/io/cdap/cdap/etl/proto/v2/validation/StageValidationRequest.java b/cdap-app-templates/cdap-etl/cdap-etl-proto/src/main/java/io/cdap/cdap/etl/proto/v2/validation/StageValidationRequest.java index 27b8e629bad0..c1c9aff4f94d 100644 --- a/cdap-app-templates/cdap-etl/cdap-etl-proto/src/main/java/io/cdap/cdap/etl/proto/v2/validation/StageValidationRequest.java +++ b/cdap-app-templates/cdap-etl/cdap-etl-proto/src/main/java/io/cdap/cdap/etl/proto/v2/validation/StageValidationRequest.java @@ -17,9 +17,15 @@ package io.cdap.cdap.etl.proto.v2.validation; +import com.google.common.base.Strings; import io.cdap.cdap.etl.proto.v2.ETLStage; + +import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; /** * Request to validate a pipeline stage. @@ -29,13 +35,21 @@ public class StageValidationRequest { private final ETLStage stage; private final List inputSchemas; private final Boolean resolveMacrosFromPreferences; + private final String doNotSkipInvalidMacroForFunctions; + public StageValidationRequest(ETLStage stage, + List inputSchemas, + boolean resolveMacrosFromPreferences) { + this(stage, inputSchemas, resolveMacrosFromPreferences, null); + } public StageValidationRequest(ETLStage stage, List inputSchemas, - boolean resolveMacrosFromPreferences) { + boolean resolveMacrosFromPreferences, + String doNotSkipInvalidMacroForFunctions) { this.stage = stage; this.inputSchemas = inputSchemas; this.resolveMacrosFromPreferences = resolveMacrosFromPreferences; + this.doNotSkipInvalidMacroForFunctions = doNotSkipInvalidMacroForFunctions; } public ETLStage getStage() { @@ -50,6 +64,21 @@ public boolean getResolveMacrosFromPreferences() { return resolveMacrosFromPreferences != null ? resolveMacrosFromPreferences : false; } + /** + * This method will return macro function names for which invalid macros should not be skipped. + * @return Set of macro function names + */ + public Set getDoNotSkipInvalidMacroForFunctions() { + Set doNotSkipInvalidMacroForFunctionsSet = new HashSet<>(); + if (!Strings.isNullOrEmpty(doNotSkipInvalidMacroForFunctions)) { + doNotSkipInvalidMacroForFunctionsSet.addAll(Arrays.stream(doNotSkipInvalidMacroForFunctions.split(",")) + .map(String::trim) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toSet())); + } + return doNotSkipInvalidMacroForFunctionsSet; + } + /** * Validate that the request contains all required information. This should be called whenever * this instance is created by deserializing user provided input.