You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/08/12 21:08:23 UTC
[PR] Validate segment upload destinations [pinot]
xiangfu0 opened a new pull request, #19231:
URL: https://github.com/apache/pinot/pull/19231
## Summary
- Establish and canonicalize the final segment destination before controller storage or metadata handling.
- Authorize the exact destination table for every v1 upload variant and reject inconsistent request, header, or segment metadata table names.
- Preserve v2 request-table promotion behavior while binding database headers and batch uploads to their authorized destinations.
- Return clear client errors for missing, blank, invalid, or mismatched table inputs.
## Root cause
The upload handlers did not share a canonical destination-binding step across request inputs and segment metadata. In v1, authorization could therefore complete before the final table destination was established.
## Impact
Upload and refresh authorization now targets the same table used by all subsequent controller operations, independently of the configured access-control implementation. Existing valid v2 promotion behavior remains supported.
## How to reproduce
Run the added `PinotSegmentUploadAuthorizationTest`. It configures permissive cluster-level create access with table-scoped authorization and exercises v1, v2, batch, upload, and refresh destination handling.
## Validation
- `FineGrainedAuthUtilsTest`
- `PinotSegmentUploadDownloadRestletResourceTest`
- `PinotSegmentUploadAuthorizationTest`
- Spotless, Checkstyle, and license checks for `pinot-core` and `pinot-controller`
- JDK 25 warning-enabled test compilation for both affected modules
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate segment upload destinations [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19231:
URL: https://github.com/apache/pinot/pull/19231#discussion_r3798298026
##########
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:
Updated table-name normalization so only null is treated as absent; any supplied blank request, header, or strict metadata value now returns HTTP 400. Added resolver and batch endpoint regression coverage.
##########
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:
Batch uploads now normalize and compare each extracted segment metadata table with the canonical request destination before segment validation, deep-store, or ZooKeeper work. Added a request-A/metadata-B regression without a table header and verified that no destination segment state is created.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate segment upload destinations [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19231:
URL: https://github.com/apache/pinot/pull/19231#issuecomment-5418449580
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/1006 (merged).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate segment upload destinations [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #19231:
URL: https://github.com/apache/pinot/pull/19231
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate segment upload destinations [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19231:
URL: https://github.com/apache/pinot/pull/19231#discussion_r3857802589
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -367,19 +358,15 @@ private SuccessResponse uploadSegment(@Nullable String tableName, TableType tabl
// Fetch segment name
String segmentName = segmentMetadata.getName();
- // Fetch table name. Try to derive the table name from the parameter and then from segment metadata
- String rawTableName;
- if (StringUtils.isNotEmpty(tableName)) {
- rawTableName = TableNameBuilder.extractRawTableName(tableName);
- } else {
- // TODO: remove this when we completely deprecate the table name from segment metadata
- rawTableName = segmentMetadata.getTableName();
- LOGGER.warn("Table name is not provided as request query parameter when uploading segment: {} for table: {}",
- segmentName, rawTableName);
- }
- String tableNameWithType = tableType == TableType.OFFLINE
- ? TableNameBuilder.OFFLINE.tableNameWithType(rawTableName)
- : TableNameBuilder.REALTIME.tableNameWithType(rawTableName);
+ String rawTableName = resolveDestinationTableName(tableName, tableNameInHeader, segmentMetadata.getTableName(),
+ tableType, headers, requireMatchingMetadataTable);
Review Comment:
Moved request and header normalization to the start of `uploadSegment`, before temp-file creation, URI download, decryption, or metadata extraction. I also added a URI-upload regression that asserts the segment source receives zero requests for an invalid request/header binding.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -830,8 +895,8 @@ private void decryptFile(String crypterClassName, File tempEncryptedFile, File t
// request if a multipart object is not sent. This endpoint does not move the segment to its final location;
// it keeps it at the downloadURI header that is set. We will not support this endpoint going forward.
public void uploadSegmentAsJson(String segmentJsonStr,
- @ApiParam(value = "Name of the table to upload into. Overrides segment.table.name in segment metadata when set "
- + "(allows promoting a segment built for another table). Falls back to metadata when omitted.")
+ @ApiParam(value = "Name of the table to upload into. Must match segment.table.name in segment metadata when both "
+ + "are set. Falls back to metadata when omitted.")
Review Comment:
Kept strict v1 matching because the destination-binding requirement intentionally rejects inconsistent request and metadata tables. The new Compatibility section calls out that v1 mismatches return 400 and directs intentional metadata overrides or promotion to `/v2/segments`.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -735,6 +736,70 @@ 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");
Review Comment:
Documented this in the new Compatibility section: when `Pinot-TableName` is supplied, it must match the canonical destination and a mismatch returns 400.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskGenerator.java:
##########
@@ -145,7 +145,7 @@ public List<PinotTaskConfig> generateTasks(List<TableConfig> tableConfigs) {
}
configs.put(MinionConstants.DOWNLOAD_URL_KEY, segmentZKMetadata.getDownloadUrl());
configs.put(MinionConstants.UPLOAD_URL_KEY,
- _clusterInfoAccessor.getVipUrlForLeadController(tableName) + "/segments");
+ _clusterInfoAccessor.getVipUrlForLeadController(tableName) + "/v2/segments");
Review Comment:
Added load-bearing comments at both the Purge and Refresh v2 URL sites. Those tasks can reuse the original artifact and source-table metadata; other in-tree conversion generators remain on v1 only because they regenerate destination-bound metadata, and any future artifact-reuse path must use v2.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate segment upload destinations [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #19231:
URL: https://github.com/apache/pinot/pull/19231#discussion_r3857133859
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -830,8 +895,8 @@ private void decryptFile(String crypterClassName, File tempEncryptedFile, File t
// request if a multipart object is not sent. This endpoint does not move the segment to its final location;
// it keeps it at the downloadURI header that is set. We will not support this endpoint going forward.
public void uploadSegmentAsJson(String segmentJsonStr,
- @ApiParam(value = "Name of the table to upload into. Overrides segment.table.name in segment metadata when set "
- + "(allows promoting a segment built for another table). Falls back to metadata when omitted.")
+ @ApiParam(value = "Name of the table to upload into. Must match segment.table.name in segment metadata when both "
+ + "are set. Falls back to metadata when omitted.")
Review Comment:
This drops promotion from v1: a segment built for table A can no longer be uploaded to table B through `/segments`. The old doc advertised that capability explicitly.
Re-authorizing against the resolved destination already closes the gap on its own, since a v1 promotion would be authorized against B. So the metadata-match requirement is extra hardening that costs compatibility. Is it needed, or can v1 keep promotion now that the destination is authorized? Either way this belongs in the description as a breaking change: external v1 callers that promote will start getting 400.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -735,6 +736,70 @@ 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");
Review Comment:
This makes the `Pinot-TableName` header authoritative. It was read and discarded before, and nothing in this repo sets it — only the new test — so the new requirement has no coverage against a real client.
An external uploader that sets it to, say, the source table of a conversion now gets a 400 where it previously succeeded. Worth a line in the description.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/purge/PurgeTaskGenerator.java:
##########
@@ -145,7 +145,7 @@ public List<PinotTaskConfig> generateTasks(List<TableConfig> tableConfigs) {
}
configs.put(MinionConstants.DOWNLOAD_URL_KEY, segmentZKMetadata.getDownloadUrl());
configs.put(MinionConstants.UPLOAD_URL_KEY,
- _clusterInfoAccessor.getVipUrlForLeadController(tableName) + "/segments");
+ _clusterInfoAccessor.getVipUrlForLeadController(tableName) + "/v2/segments");
Review Comment:
MergeRollup, UpsertCompaction, UpsertCompactMerge and RealtimeToOffline still post to `/segments`. They are safe only because their executors always regenerate the segment, so `segment.table.name` matches the destination. Purge and Refresh are the two that can return the original `indexDir`.
That invariant is implicit and now load-bearing. If someone later adds a reuse path to one of those executors it breaks at runtime, not at compile time. Either move all the generators to `/v2/segments`, or record the invariant in a comment here.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -367,19 +358,15 @@ private SuccessResponse uploadSegment(@Nullable String tableName, TableType tabl
// Fetch segment name
String segmentName = segmentMetadata.getName();
- // Fetch table name. Try to derive the table name from the parameter and then from segment metadata
- String rawTableName;
- if (StringUtils.isNotEmpty(tableName)) {
- rawTableName = TableNameBuilder.extractRawTableName(tableName);
- } else {
- // TODO: remove this when we completely deprecate the table name from segment metadata
- rawTableName = segmentMetadata.getTableName();
- LOGGER.warn("Table name is not provided as request query parameter when uploading segment: {} for table: {}",
- segmentName, rawTableName);
- }
- String tableNameWithType = tableType == TableType.OFFLINE
- ? TableNameBuilder.OFFLINE.tableNameWithType(rawTableName)
- : TableNameBuilder.REALTIME.tableNameWithType(rawTableName);
+ String rawTableName = resolveDestinationTableName(tableName, tableNameInHeader, segmentMetadata.getTableName(),
+ tableType, headers, requireMatchingMetadataTable);
Review Comment:
The request and header table-name checks now run here, after the segment has been downloaded, decrypted and untarred. The type-mismatch check used to sit at the top of `uploadSegment`, before any of that.
So `?tableName=myTable_REALTIME&tableType=OFFLINE` now pulls the whole segment before returning 400, and for URI push that is a full remote fetch. Neither the request name nor the header name depends on the segment, so both could be normalized up front, leaving only the metadata comparison here.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate segment upload destinations [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19231:
URL: https://github.com/apache/pinot/pull/19231#issuecomment-5273209851
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19231?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `90.56604%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 39.17%. Comparing base ([`d142389`](https://app.codecov.io/gh/apache/pinot/commit/d1423896041b0bd2787bee3ee0268cf8768e3936?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0602c61`](https://app.codecov.io/gh/apache/pinot/commit/0602c61abc6baa2f074d471f9fef0173bfcf2b4e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19231?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ces/PinotSegmentUploadDownloadRestletResource.java](https://app.codecov.io/gh/apache/pinot/pull/19231?src=pr&el=tree&filepath=pinot-controller%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcontroller%2Fapi%2Fresources%2FPinotSegmentUploadDownloadRestletResource.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90U2VnbWVudFVwbG9hZERvd25sb2FkUmVzdGxldFJlc291cmNlLmphdmE=) | 92.68% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...g/apache/pinot/core/auth/FineGrainedAuthUtils.java](https://app.codecov.io/gh/apache/pinot/pull/19231?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fauth%2FFineGrainedAuthUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hdXRoL0ZpbmVHcmFpbmVkQXV0aFV0aWxzLmphdmE=) | 66.66% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (d142389) and HEAD (0602c61). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (d142389) | HEAD (0602c61) |
>|------|------|------|
>|unittests|2|1|
>|java-25|6|1|
>|temurin|6|1|
>|lane-a|2|0|
>|integration|4|0|
>|integration2|2|0|
>|lane-b|2|0|
>|integration1|2|0|
>|unittests1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19231 +/- ##
=============================================
- Coverage 66.94% 39.17% -27.78%
+ Complexity 1423 1416 -7
=============================================
Files 3452 3452
Lines 218541 218573 +32
Branches 34740 34743 +3
=============================================
- Hits 146310 85629 -60681
- Misses 60546 125116 +64570
+ Partials 11685 7828 -3857
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.17% <90.56%> (-27.78%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.17% <90.56%> (-27.78%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.17% <90.56%> (-27.77%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19231/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.17% <90.56%> (+0.15%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19231?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate segment upload destinations [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org