kennknowles commented on code in PR #31406:
URL: https://github.com/apache/beam/pull/31406#discussion_r1622743802
##########
sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java:
##########
@@ -488,8 +489,49 @@ public static AllowList everything() {
AllowedClass.create("*", AllowedClass.WILDCARD,
AllowedClass.WILDCARD)));
}
+ static AllowList parseFromYamlStream(InputStream inputStream) {
+ Yaml yaml = new Yaml();
+ Map<Object, Object> config = yaml.load(inputStream);
+
+ if (config != null) {
Review Comment:
Readability: Reverse the `if` statement and make it `if (config == null)
throw IllegalArgumentException`
##########
sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java:
##########
@@ -104,16 +105,16 @@ class ExpansionServiceConfigFactory implements
DefaultValueFactory<ExpansionServ
public ExpansionServiceConfig create(PipelineOptions options) {
String configFile =
options.as(ExpansionServiceOptions.class).getExpansionServiceConfigFile();
if (configFile != null) {
- ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
File configFileObj = new File(configFile);
if (!configFileObj.exists()) {
throw new IllegalArgumentException("Config file " + configFile + "
does not exist");
}
- try {
- return mapper.readValue(configFileObj, ExpansionServiceConfig.class);
+ try (InputStream stream = new FileInputStream(configFileObj)) {
Review Comment:
Here too, would be nice to add a message to these RuntimeException wrappers,
like "when opening file to parse as ExpansionServiceConfig"
##########
sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java:
##########
@@ -78,17 +79,17 @@ public AllowList create(PipelineOptions options) {
if (allowListFile.equals("*")) {
return AllowList.everything();
}
- ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
File allowListFileObj = new File(allowListFile);
if (!allowListFileObj.exists()) {
throw new IllegalArgumentException(
"Allow list file " + allowListFile + " does not exist");
}
- try {
- return mapper.readValue(allowListFileObj, AllowList.class);
+ try (InputStream stream = new FileInputStream(allowListFileObj)) {
+ return AllowList.parseFromYamlStream(stream);
+ } catch (FileNotFoundException e) {
+ throw new RuntimeException(e);
Review Comment:
Would be nice to add a message to these RuntimeException wrappers, like
"when opening file to parse as ExpansionServiceConfig"
##########
sdks/java/expansion-service/build.gradle:
##########
@@ -41,10 +41,8 @@ dependencies {
implementation project(path: ":sdks:java:core", configuration: "shadow")
implementation project(path: ":runners:java-fn-execution")
implementation project(path: ":sdks:java:harness")
+ implementation "org.yaml:snakeyaml:2.0"
Review Comment:
put this in the BeamModulePlugin?
##########
sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java:
##########
@@ -488,8 +489,49 @@ public static AllowList everything() {
AllowedClass.create("*", AllowedClass.WILDCARD,
AllowedClass.WILDCARD)));
}
+ static AllowList parseFromYamlStream(InputStream inputStream) {
+ Yaml yaml = new Yaml();
+ Map<Object, Object> config = yaml.load(inputStream);
+
+ if (config != null) {
+ String version = config.get("version") != null ? (String)
config.get("version") : "";
+ List<AllowedClass> allowedClasses = new ArrayList<>();
+ if (config.get("allowedClasses") != null) {
+ allowedClasses =
+ ((List<Map<Object, Object>>) config.get("allowedClasses"))
+ .stream()
+ .map(
+ data -> {
+ String className = (String) data.get("className");
+ if (className == null) {
+ throw new IllegalArgumentException(
+ "Expected each entry in the allowlist to
include the 'className'");
+ }
+ List<String> allowedBuilderMethods =
+ (List<String>)
data.get("allowedBuilderMethods");
+ List<String> allowedConstructorMethods =
+ (List<String>)
data.get("allowedConstructorMethods");
+ if (allowedBuilderMethods == null) {
+ allowedBuilderMethods = new ArrayList<>();
+ }
+ if (allowedConstructorMethods == null) {
+ allowedConstructorMethods = new ArrayList<>();
+ }
+ return AllowedClass.create(
+ className, allowedBuilderMethods,
allowedConstructorMethods);
+ })
+ .collect(Collectors.toList());
+ }
+ return AllowList.create(version, allowedClasses);
+ } else {
+ throw new IllegalArgumentException(
+ "Could not parse the provided YAML stream into a non-trivial
AllowList");
+ }
+ }
+
public abstract String getVersion();
+ @SuppressWarnings("mutable")
Review Comment:
Would making it ImmutableList be just as good as suppression?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
