Copilot commented on code in PR #18660:
URL: https://github.com/apache/pinot/pull/18660#discussion_r3352309905
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/FileIngestionHelper.java:
##########
@@ -176,14 +179,45 @@ public SuccessResponse buildSegmentAndPush(DataPayload
payload)
*/
public static void copyURIToLocal(Map<String, String> batchConfigMap, URI
sourceFileURI, File destFile)
throws Exception {
+ copyURIToLocal(batchConfigMap, sourceFileURI, destFile, true);
+ }
+
+ public static void copyURIToLocal(Map<String, String> batchConfigMap, URI
sourceFileURI, File destFile,
+ boolean allowLocalFileSystem)
+ throws Exception {
String sourceFileURIScheme = sourceFileURI.getScheme();
+ Preconditions.checkArgument(allowLocalFileSystem ||
StringUtils.isNotBlank(sourceFileURIScheme),
+ "Source URI must include a scheme: %s", sourceFileURI);
+ Preconditions.checkArgument(allowLocalFileSystem
+ ||
!PinotFSFactory.LOCAL_PINOT_FS_SCHEME.equalsIgnoreCase(sourceFileURIScheme),
+ "Local filesystem URIs are not allowed for /ingestFromURI: %s",
sourceFileURI);
if (!PinotFSFactory.isSchemeSupported(sourceFileURIScheme)) {
- PinotFSFactory.register(sourceFileURIScheme,
batchConfigMap.get(BatchConfigProperties.INPUT_FS_CLASS),
+ String inputFsClassName =
batchConfigMap.get(BatchConfigProperties.INPUT_FS_CLASS);
+ Preconditions.checkArgument(StringUtils.isNotBlank(inputFsClassName),
+ "Must provide %s for unsupported scheme: %s",
BatchConfigProperties.INPUT_FS_CLASS, sourceFileURIScheme);
+ Preconditions.checkArgument(allowLocalFileSystem ||
!isLocalPinotFSClass(inputFsClassName),
+ "Local filesystem implementation is not allowed for /ingestFromURI:
%s", inputFsClassName);
+ PinotFSFactory.register(sourceFileURIScheme, inputFsClassName,
IngestionConfigUtils.getInputFsProps(batchConfigMap));
}
+ Preconditions.checkArgument(allowLocalFileSystem
+ || !PinotFSFactory.isSchemeRegisteredWith(sourceFileURIScheme,
LocalPinotFS.class),
+ "Local filesystem implementation is not allowed for /ingestFromURI:
%s", sourceFileURIScheme);
PinotFSFactory.create(sourceFileURIScheme).copyToLocalFile(sourceFileURI,
destFile);
}
+ private static boolean isLocalPinotFSClass(String className)
+ throws ClassNotFoundException {
+ String pluginName = PluginManager.DEFAULT_PLUGIN_NAME;
+ String rawClassName = className;
+ int pluginSeparatorIndex = className.indexOf(':');
+ if (pluginSeparatorIndex >= 0) {
+ pluginName = className.substring(0, pluginSeparatorIndex);
+ rawClassName = className.substring(pluginSeparatorIndex + 1);
+ }
+ return
LocalPinotFS.class.isAssignableFrom(PluginManager.get().loadClass(pluginName,
rawClassName));
+ }
Review Comment:
`isLocalPinotFSClass` currently throws `ClassNotFoundException` for an
invalid `input.fs.className`, which will bubble up as a 500 from
`/ingestFromURI` (caught by the generic `Exception` handler) instead of a 400.
Since this class name is user-provided, it should be treated as an invalid
argument and mapped to `IllegalArgumentException` with a clear message.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
