Copilot commented on code in PR #19231:
URL: https://github.com/apache/pinot/pull/19231#discussion_r3791395105
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -735,6 +733,66 @@ private void cleanupTempFiles(List<File> tempFiles) {
}
}
+ @VisibleForTesting
+ static String resolveDestinationTableName(@Nullable String requestTableName,
@Nullable String headerTableName,
+ @Nullable String metadataTableName, TableType tableType, HttpHeaders
headers,
+ boolean requireMatchingMetadataTable) {
+ String normalizedRequestTable = normalizeTableName(requestTableName,
tableType, headers, "request tableName");
+ String normalizedHeaderTable = normalizeTableName(headerTableName,
tableType, headers,
+ CommonConstants.Controller.TABLE_NAME_HTTP_HEADER + " header");
+ String destinationTable = normalizedRequestTable != null ?
normalizedRequestTable
+ : normalizedHeaderTable;
+ String normalizedMetadataTable = null;
+ if (requireMatchingMetadataTable || destinationTable == null) {
+ normalizedMetadataTable =
+ normalizeTableName(metadataTableName, tableType, headers, "segment
metadata table name");
+ if (destinationTable == null) {
+ destinationTable = normalizedMetadataTable;
+ }
+ }
+ if (destinationTable == null) {
+ throw new ControllerApplicationException(LOGGER,
+ "Table name is required in the request, " +
CommonConstants.Controller.TABLE_NAME_HTTP_HEADER
+ + " header, or segment metadata",
+ Response.Status.BAD_REQUEST);
+ }
+
+ validateMatchingTableName(destinationTable, normalizedHeaderTable,
+ CommonConstants.Controller.TABLE_NAME_HTTP_HEADER + " header");
+ if (requireMatchingMetadataTable) {
+ validateMatchingTableName(destinationTable, normalizedMetadataTable,
"segment metadata table name");
+ }
+ return destinationTable;
+ }
+
+ @Nullable
+ private static String normalizeTableName(@Nullable String tableName,
TableType tableType, HttpHeaders headers,
+ String source) {
+ if (StringUtils.isBlank(tableName)) {
+ return null;
+ }
Review Comment:
A supplied whitespace-only table value is converted to “not supplied.” In
particular, a blank table header is silently ignored when another source
provides the destination, and batch upload accepts it, contrary to the stated
rejection of blank table inputs. Preserve `null` as optional, but return a
clear 400 for non-null blank values.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -574,9 +561,20 @@ private SuccessResponse uploadReingestedSegment(String
tableName, FormDataMultiP
private SuccessResponse uploadSegments(String tableName, TableType
tableType, FormDataMultiPart multiPart,
boolean enableParallelPushProtection, boolean allowRefresh, HttpHeaders
headers, Request request) {
long segmentsUploadStartTimeMs = System.currentTimeMillis();
- String rawTableName = TableNameBuilder.extractRawTableName(tableName);
- String tableNameWithType = tableType == TableType.OFFLINE ?
TableNameBuilder.OFFLINE.tableNameWithType(rawTableName)
- : TableNameBuilder.REALTIME.tableNameWithType(rawTableName);
+ String rawTableName = normalizeTableName(tableName, tableType, headers,
"request tableName");
+ if (rawTableName == null) {
+ throw new ControllerApplicationException(LOGGER, "tableName is required
for batch segment upload",
+ Response.Status.BAD_REQUEST);
+ }
+ String tableNameInHeader = normalizeTableName(
+ extractHttpHeader(headers,
CommonConstants.Controller.TABLE_NAME_HTTP_HEADER), tableType, headers,
+ CommonConstants.Controller.TABLE_NAME_HTTP_HEADER + " header");
+ validateMatchingTableName(rawTableName, tableNameInHeader,
+ CommonConstants.Controller.TABLE_NAME_HTTP_HEADER + " header");
Review Comment:
The batch path validates only the request and table header. After each
segment's metadata is extracted, its `segment.table.name` is never compared
with `rawTableName`; `ZKOperator.completeSegmentsOperations` then uses the
request-derived `TableConfig`, so a v1 batch can still place a segment built
for another table into the authorized destination. Validate every extracted
metadata table against the canonical destination before adding it to
`segmentUploadMetadataList`, and cover the request-A/metadata-B case without a
mismatched header.
--
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]
