Skip to content

Core: Make PlanTableScanResponse buider withCredentials replace instead of append #17751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
singhpk234 merged 1 commit into from
Oct 1, 2026
+85 −8
Merged

Core: Make PlanTableScanResponse buider withCredentials replace instead of append #17751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 5 additions & 4 deletions core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.rest.PlanStatus;
import org.apache.iceberg.rest.credentials.Credential;

Expand Down Expand Up @@ -110,7 +109,7 @@ private Builder() {}

private PlanStatus planStatus;
private ErrorResponse errorResponse;
private final List<Credential> credentials = Lists.newArrayList();
private List<Credential> credentials = ImmutableList.of();

public Builder withPlanStatus(PlanStatus status) {
this.planStatus = status;
Expand All @@ -122,8 +121,10 @@ public Builder withErrorResponse(ErrorResponse response) {
return this;
}

public Builder withCredentials(List<Credential> credentialsToAdd) {
credentials.addAll(credentialsToAdd);
public Builder withCredentials(List<Credential> newCredentials) {
Preconditions.checkArgument(null != newCredentials, "Invalid credentials: null");

@singhpk234 singhpk234 Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

per

Suggested change
Preconditions.checkArgument(null != newCredentials, "Invalid credentials: null");
Preconditions.checkArgument(newCredentials, "Invalid credentials list : null");

iceberg/core/src/main/java/org/apache/iceberg/rest/responses/ListTablesResponse.java

Line 83 in 7f879b1

Preconditions.checkNotNull(toAdd, "Invalid table identifier list: null");

@dramaticlly dramaticlly Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @singhpk234 , I think ListTableResponse is relative old (merged Feb 2022) compare to the latest agents.md guideline in

iceberg/AGENTS.md

Line 121 in f80a72f

- Use `ConcurrentMap` for shared mutable state. `Preconditions.checkArgument` over NPE.
which prefer IllegalArgumentException over NPE. Let me know if you want to switch to NPE instead

Preconditions.checkArgument(!newCredentials.contains(null), "Invalid credential: null");

@nastra nastra Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we actually need this check? We should get this for free when doing this.credentials = ImmutableList.copyOf(credentialsToAdd)

@dramaticlly dramaticlly Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think ImmutableList.copyOf(credentialsToAdd) will throw NPE instead of IllegalArgumentException instead, from AGENTS.md seem to favor "Preconditions.checkArgument over NPE"

and I saw we have some precedence in

iceberg/core/src/main/java/org/apache/iceberg/rest/responses/ListTablesResponse.java

Line 84 in 7f879b1

Preconditions.checkArgument(!toAdd.contains(null), "Invalid table identifier: null");
and

iceberg/core/src/main/java/org/apache/iceberg/rest/responses/ListNamespacesResponse.java

Line 83 in 7f879b1

Preconditions.checkArgument(!toAdd.contains(null), "Invalid namespace: null");

this.credentials = ImmutableList.copyOf(newCredentials);
return this;
}

Expand Down
9 changes: 5 additions & 4 deletions core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.rest.PlanStatus;
import org.apache.iceberg.rest.credentials.Credential;

Expand Down Expand Up @@ -144,7 +143,7 @@ public static class Builder extends BaseScanTaskResponse.Builder<Builder, PlanTa
private PlanStatus planStatus;
private String planId;
private ErrorResponse errorResponse;
private final List<Credential> credentials = Lists.newArrayList();
private List<Credential> credentials = ImmutableList.of();

private Builder() {}

Expand All @@ -163,8 +162,10 @@ public Builder withErrorResponse(ErrorResponse response) {
return this;
}

public Builder withCredentials(List<Credential> credentialsToAdd) {
credentials.addAll(credentialsToAdd);
public Builder withCredentials(List<Credential> newCredentials) {
Preconditions.checkArgument(null != newCredentials, "Invalid credentials: null");
Preconditions.checkArgument(!newCredentials.contains(null), "Invalid credential: null");
Comment thread
dramaticlly marked this conversation as resolved.
this.credentials = ImmutableList.copyOf(newCredentials);
return this;
}

Expand Down
38 changes: 38 additions & 0 deletions ...rc/test/java/org/apache/iceberg/rest/responses/TestFetchPlanningResultResponseParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.fasterxml.jackson.databind.InjectableValues;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.ObjectReader;
import java.util.Collections;
import java.util.List;
import org.apache.iceberg.BaseFileScanTask;
import org.apache.iceberg.DeleteFile;
Expand All @@ -58,6 +59,17 @@ public class TestFetchPlanningResultResponseParser {
.build();
private static final ObjectMapper MAPPER = new ObjectMapper(FACTORY);

private static final Credential S3_CREDENTIAL =
ImmutableCredential.builder()
.prefix("s3://custom-uri")
.config(ImmutableMap.of("s3.access-key-id", "keyId"))
.build();
private static final Credential GCS_CREDENTIAL =
ImmutableCredential.builder()
.prefix("gs://custom-uri")
.config(ImmutableMap.of("gcs.oauth2.token", "gcsToken"))
.build();

@BeforeEach
public void before() {
RESTSerializers.registerAll(MAPPER);
Expand Down Expand Up @@ -384,4 +396,30 @@ public void cannotBuildWithErrorResponseWhenStatusIsNotFailed() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid response: error can only be returned in a 'failed' status");
}

@Test
void withCredentialsReplacesPreviouslySetCredentials() {
FetchPlanningResultResponse response =
FetchPlanningResultResponse.builder()
.withPlanStatus(PlanStatus.COMPLETED)
.withCredentials(ImmutableList.of(S3_CREDENTIAL))
.withCredentials(ImmutableList.of(GCS_CREDENTIAL))
.build();

assertThat(response.credentials()).containsExactly(GCS_CREDENTIAL);
}

@Test
void nullCredentials() {
assertThatThrownBy(() -> FetchPlanningResultResponse.builder().withCredentials(null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid credentials: null");

assertThatThrownBy(
() ->
FetchPlanningResultResponse.builder()
.withCredentials(Collections.singletonList(null)))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid credential: null");
}
}
37 changes: 37 additions & 0 deletions core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.util.Collections;
import java.util.List;
import org.apache.iceberg.BaseFileScanTask;
import org.apache.iceberg.DataFile;
Expand All @@ -50,6 +51,17 @@

public class TestPlanTableScanResponseParser {

private static final Credential S3_CREDENTIAL =
ImmutableCredential.builder()
.prefix("s3://custom-uri")
.config(ImmutableMap.of("s3.access-key-id", "keyId"))
.build();
private static final Credential GCS_CREDENTIAL =
ImmutableCredential.builder()
.prefix("gs://custom-uri")
.config(ImmutableMap.of("gcs.oauth2.token", "gcsToken"))
.build();

@Test
public void nullAndEmptyCheck() {
assertThatThrownBy(() -> PlanTableScanResponseParser.toJson(null))
Expand Down Expand Up @@ -855,4 +867,29 @@ public void cannotBuildWithErrorResponseWhenStatusIsNotFailed() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid response: error can only be defined when status is 'failed'");
}

@Test
void withCredentialsReplacesPreviouslySetCredentials() {
PlanTableScanResponse response =
PlanTableScanResponse.builder()
.withPlanStatus(PlanStatus.COMPLETED)
.withSpecsById(PARTITION_SPECS_BY_ID)
.withCredentials(ImmutableList.of(S3_CREDENTIAL))
.withCredentials(ImmutableList.of(GCS_CREDENTIAL))
.build();

assertThat(response.credentials()).containsExactly(GCS_CREDENTIAL);
}

@Test
void nullCredentials() {
assertThatThrownBy(() -> PlanTableScanResponse.builder().withCredentials(null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid credentials: null");

assertThatThrownBy(
() -> PlanTableScanResponse.builder().withCredentials(Collections.singletonList(null)))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid credential: null");
}
}
Loading