Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,6 @@ public static Set<Path> pluginLocations(String pluginPath, boolean failFast) {
for (String path : pluginPathElements) {
try {
Path pluginPathElement = Paths.get(path).toAbsolutePath();
if (pluginPath.isEmpty()) {
log.warn("Plugin path element is empty, evaluating to {}.", pluginPathElement);
}
if (!Files.exists(pluginPathElement)) {
throw new FileNotFoundException(pluginPathElement.toString());
}
Expand Down
22 changes: 22 additions & 0 deletions tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import java.util.Objects;
import java.util.Properties;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand All @@ -68,6 +69,8 @@ public class ConnectPluginPath {
};
public static final String NO_ALIAS = "N/A";

private static final Pattern COMMA_WITH_WHITESPACE = Pattern.compile("\\s*,\\s*");

public static void main(String[] args) {
Exit.exit(mainNoExit(args, System.out, System.err));
}
Expand Down Expand Up @@ -162,6 +165,9 @@ private static Set<Path> parseLocations(ArgumentParser parser, Namespace namespa
if (rawLocations.isEmpty() && rawPluginPaths.isEmpty() && rawWorkerConfigs.isEmpty()) {
throw new ArgumentParserException("Must specify at least one --plugin-location, --plugin-path, or --worker-config", parser);
}
for (String pluginPath : rawPluginPaths) {
validatePluginPath(pluginPath, "--plugin-path");
}
Set<Path> pluginLocations = new LinkedHashSet<>();
for (String rawWorkerConfig : rawWorkerConfigs) {
Properties properties;
Expand All @@ -172,6 +178,7 @@ private static Set<Path> parseLocations(ArgumentParser parser, Namespace namespa
}
String pluginPath = properties.getProperty(WorkerConfig.PLUGIN_PATH_CONFIG);
if (pluginPath != null) {
validatePluginPath(pluginPath, WorkerConfig.PLUGIN_PATH_CONFIG);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should throw a ConfigException when validating WorkerConfig.PLUGIN_PATH_CONFIG at this stage. This would preserve the same behavior as when WorkerConfig.PLUGIN_PATH_CONFIG is set to an empty value in the config.

rawPluginPaths.add(pluginPath);
}
}
Expand All @@ -192,6 +199,21 @@ private static Set<Path> parseLocations(ArgumentParser parser, Namespace namespa
return pluginLocations;
}

private static void validatePluginPath(String pluginPath, String configName) throws TerseException {
String trimmed = pluginPath.trim();
if (trimmed.isEmpty()) {
throw new TerseException("'" + configName + "' must not be empty.");
}

String[] pluginPathElements = COMMA_WITH_WHITESPACE.split(trimmed, -1);

for (String path : pluginPathElements) {
if (path.isEmpty()) {
throw new TerseException("'" + configName + "' values must not be empty.");
}
}
}

enum Command {
LIST, SYNC_MANIFESTS
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,63 @@ public void testSyncManifestsKeepNotFound(PluginLocationType type) {
assertBadPackagingPluginsStatus(table, false);
}

@Test
public void testListEmptyPluginPathArg() {
CommandResult res = runCommand(
"list",
"--plugin-path",
""
);
assertNotEquals(0, res.returnCode);
assertTrue("'--plugin-path' must not be empty.\n".equals(res.err));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use assertEquals to validate the error message.

}

@Test
public void testListEmptyPluginPathElementArg() {
CommandResult res = runCommand(
"list",
"--plugin-path",
"location-a,,location-b"
);
assertNotEquals(0, res.returnCode);
assertTrue("'--plugin-path' values must not be empty.\n".equals(res.err));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}

@Test
public void testListEmptyPluginPathInWorkerConfig() {
Path configPath = workspace.resolve("worker-empty.properties");
try {
Files.writeString(configPath, "plugin.path=", StandardCharsets.UTF_8);
} catch (IOException e) {
fail("Failed to create test worker config: " + e.getMessage());
}

CommandResult res = runCommand(
"list",
"--worker-config",
configPath.toString()
);
assertNotEquals(0, res.returnCode);
assertTrue("'plugin.path' must not be empty.\n".equals(res.err));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}

@Test
public void testListEmptyPluginPathElementInWorkerConfig() {
Path configPath = workspace.resolve("worker-empty-element.properties");
try {
Files.writeString(configPath, "plugin.path=location-a,,location-b", StandardCharsets.UTF_8);
} catch (IOException e) {
fail("Failed to create test worker config: " + e.getMessage());
}

CommandResult res = runCommand(
"list",
"--worker-config",
configPath.toString()
);
assertNotEquals(0, res.returnCode);
assertTrue("'plugin.path' values must not be empty.\n".equals(res.err));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}

private static Map<String, List<String[]>> assertListSuccess(CommandResult result) {
assertEquals(0, result.returnCode);
Expand Down
Loading