Skip to content

Conversation

@jglick
Copy link
Member

@jglick jglick commented Jan 6, 2026

The basis of #980 (a full fix will require some downstream changes).

Prior cleanup: #1001.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces optimized credential lookup methods to address performance issues when searching for credentials by ID, particularly when using remote vault providers like HashiCorp Vault. The optimization allows providers to implement direct ID-based lookups instead of retrieving all credentials and filtering, enabling early termination when a matching credential is found.

Key changes:

  • Added new static lookup methods findCredentialByIdInItemGroup and findCredentialByIdInItem that search providers sequentially and return on first match
  • Added overridable provider-level methods getCredentialByIdInItemGroup and getCredentialByIdInItem with default implementations that fall back to existing list-based methods
  • Refactored findCredentialById(Run) to use the new optimized lookup methods

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java Added optimized credential lookup infrastructure with four new methods (two static, two instance) and refactored existing findCredentialById to use new optimized paths; added debug logging for lookup operations
src/main/java/com/cloudbees/plugins/credentials/CredentialsMatchers.java Added cross-references to new lookup methods in the javadoc for withId matcher
src/test/java/com/cloudbees/plugins/credentials/ByIdTest.java Added comprehensive test suite with test providers to verify lazy evaluation behavior and optimization effectiveness

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

static void addCredential(IdCredentials credential) {
credentials.add(credential);
}

Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The static credential lists in LazyProvider2 and LazyProvider3 are never cleared between tests, which could lead to test interference. Consider adding methods to clear these credentials and calling them in a setup or teardown method (e.g., @beforeeach or @AfterEach) to ensure test isolation.

Suggested change
static void clearCredentials() {
credentials.clear();
}

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

(should avoid static fields, use lookupSingleton and instance fields)

Comment on lines +277 to +282
private static final List<IdCredentials> credentials = new java.util.concurrent.CopyOnWriteArrayList<>();
static final AtomicInteger listCalls = new AtomicInteger(0);

static void addCredential(IdCredentials credential) {
credentials.add(credential);
}
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The static credential list in LazyProvider3 is never cleared between tests, which could lead to test interference. Consider adding a method to clear these credentials and calling it in a setup or teardown method (e.g., @beforeeach or @AfterEach) to ensure test isolation.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant