Skip to content

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

Open
dramaticlly wants to merge 1 commit into
apache:mainfrom
dramaticlly:builder-credentials-replace-semantics
Open

Core: Make PlanTableScanResponse buider withCredentials replace instead of append#17751
dramaticlly wants to merge 1 commit into
apache:mainfrom
dramaticlly:builder-credentials-replace-semantics

Conversation

@dramaticlly

@dramaticllydramaticlly commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Follow up of #17638 (comment) with changes in PlanTableScanResponse.Builder and FetchPlanningResultResponse.Builder

  1. withCredentials(List<Credential>) now replaces the builder's credentials instead of appending to them.
  2. It now rejects a null list and null elements as the follow up of Core: Add storage credentials to FetchPlanningResultResponse #14994 (comment)
  3. build() now passes ImmutableList.copyOf(credentials) instead of the builder's live ArrayList.

AI Disclosure

@dramaticllydramaticlly changed the title Core: Make builder of withCredentials replace instead of appendCore: Make PlanTableScanResponse buider withCredentials replace instead of appendAug 21, 2026
@dramaticlly

Copy link
Copy Markdown
ContributorAuthor

@singhpk234@nastra@huaxingao if you want to take a look

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

@nastranastraAug 21, 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.

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

@dramaticllydramaticllyAug 21, 2026

Copy link
Copy Markdown
ContributorAuthor

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

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

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

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");

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

Copy link
Copy Markdown
ContributorAuthor

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

- 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

@dramaticlly
dramaticllyforce-pushed the builder-credentials-replace-semantics branch from 1ed9d07 to 9f2604dCompareAugust 28, 2026 17:20
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dramaticlly@nastra@singhpk234