singhpk234 commented on code in PR #17751:
URL: https://github.com/apache/iceberg/pull/17751#discussion_r3878144844
##########
core/src/main/java/org/apache/iceberg/rest/responses/PlanTableScanResponse.java:
##########
@@ -144,8 +143,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");
Review Comment:
ditto
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java:
##########
@@ -838,4 +850,58 @@ 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 withCredentialsClearsPreviouslySetCredentials() {
+ PlanTableScanResponse response =
+ PlanTableScanResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withSpecsById(PARTITION_SPECS_BY_ID)
+ .withCredentials(ImmutableList.of(S3_CREDENTIAL))
+ .withCredentials(ImmutableList.of())
+ .build();
+
+ assertThat(response.credentials()).isEmpty();
+ }
Review Comment:
same comment as prev file, is the required ?
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestPlanTableScanResponseParser.java:
##########
@@ -838,4 +850,58 @@ 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 withCredentialsClearsPreviouslySetCredentials() {
+ PlanTableScanResponse response =
+ PlanTableScanResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withSpecsById(PARTITION_SPECS_BY_ID)
+ .withCredentials(ImmutableList.of(S3_CREDENTIAL))
+ .withCredentials(ImmutableList.of())
+ .build();
+
+ assertThat(response.credentials()).isEmpty();
+ }
+
+ @Test
+ void credentialsAreNotAffectedByLaterBuilderChanges() {
+ PlanTableScanResponse.Builder builder =
+ PlanTableScanResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withSpecsById(PARTITION_SPECS_BY_ID)
+ .withCredentials(ImmutableList.of(S3_CREDENTIAL));
+ PlanTableScanResponse response = builder.build();
+
+ builder.withCredentials(ImmutableList.of(GCS_CREDENTIAL));
+
+ assertThat(response.credentials()).containsExactly(S3_CREDENTIAL);
+ }
Review Comment:
is this required ?
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestFetchPlanningResultResponseParser.java:
##########
@@ -396,4 +408,55 @@ 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 withCredentialsClearsPreviouslySetCredentials() {
+ FetchPlanningResultResponse response =
+ FetchPlanningResultResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withCredentials(ImmutableList.of(S3_CREDENTIAL))
+ .withCredentials(ImmutableList.of())
+ .build();
+
+ assertThat(response.credentials()).isEmpty();
+ }
Review Comment:
is this test required ?
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestFetchPlanningResultResponseParser.java:
##########
@@ -396,4 +408,55 @@ 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 withCredentialsClearsPreviouslySetCredentials() {
+ FetchPlanningResultResponse response =
+ FetchPlanningResultResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withCredentials(ImmutableList.of(S3_CREDENTIAL))
+ .withCredentials(ImmutableList.of())
+ .build();
+
+ assertThat(response.credentials()).isEmpty();
+ }
+
+ @Test
+ void credentialsAreNotAffectedByLaterBuilderChanges() {
+ FetchPlanningResultResponse.Builder builder =
+ FetchPlanningResultResponse.builder()
+ .withPlanStatus(PlanStatus.COMPLETED)
+ .withCredentials(ImmutableList.of(S3_CREDENTIAL));
+ FetchPlanningResultResponse response = builder.build();
+
+ builder.withCredentials(ImmutableList.of(GCS_CREDENTIAL));
+
+ assertThat(response.credentials()).containsExactly(S3_CREDENTIAL);
+ }
Review Comment:
are we testing what builder does here ? builder.build() creates a new obj
##########
core/src/main/java/org/apache/iceberg/rest/responses/FetchPlanningResultResponse.java:
##########
@@ -99,8 +98,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");
Review Comment:
per
```suggestion
Preconditions.checkArgument(newCredentials, "Invalid credentials list
: null");
```
https://github.com/apache/iceberg/blob/7f879b11366e17a676a03f15247a821751415529/core/src/main/java/org/apache/iceberg/rest/responses/ListTablesResponse.java#L83
--
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]
