Skip to content

Conversation

@AncD8972
Copy link

@AncD8972 AncD8972 commented Jan 3, 2026

Mirrored from ai-code-review-evaluation#2.

Test 2

}

String orgaId = testRealm().organizations().getAll().get(0).getId();
for (int i = 10; i < 20; i++) {
Copy link
Author

Choose a reason for hiding this comment

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

🔴 HIGH | logic_defect

Test cleanup issue: cleanup code references 'alias' but IDPs are created with specific aliases like 'idp-alias-' + i. This will cause cleanup to fail.

💡 Suggestion: Fix the cleanup code to use the correct alias or remove cleanup calls as they seem to be using wrong identifiers.

Suggested change
for (int i = 10; i < 20; i++) {
getCleanup().addCleanup(() -> testRealm().identityProviders().get("idp-alias-" + 20).remove());

// perform some login IDP searches and ensure they are cached.
session.identityProviders().getForLogin(FetchMode.REALM_ONLY, null);
IdentityProviderListQuery identityProviderListQuery = realmCache.getCache().get(cacheKeyForLogin(realm, FetchMode.REALM_ONLY), IdentityProviderListQuery.class);
assertNotNull(identityProviderListQuery);
Copy link
Author

Choose a reason for hiding this comment

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

🔴 HIGH | logic_defect

Same cleanup issue - using 'alias' instead of the actual IDP alias 'idp-alias-20'.

💡 Suggestion: Fix the cleanup alias reference.

Suggested change
assertNotNull(identityProviderListQuery);
getCleanup().addCleanup(() -> testRealm().identityProviders().get("idp-alias-20").remove());


@Override
public Stream<IdentityProviderModel> getForLogin(FetchMode mode, String organizationId) {
String cacheKey = cacheKeyForLogin(getRealm(), mode);
Copy link
Author

Choose a reason for hiding this comment

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

🟡 MEDIUM | static_defect

Potential NPE: idpDelegate.getForLogin() could return null, which would cause NPE when calling .map() on the result.

💡 Suggestion: Add null check or handle null stream appropriately.

Suggested change
String cacheKey = cacheKeyForLogin(getRealm(), mode);
if (isInvalid(cacheKey)) {
Stream<IdentityProviderModel> result = idpDelegate.getForLogin(mode, organizationId);
return result != null ? result.map(this::createOrganizationAwareIdentityProviderModel) : Stream.empty();
}

IdentityProviderListQuery query = cache.get(cacheKey, IdentityProviderListQuery.class);
String searchKey = organizationId != null ? organizationId : "";
Set<String> cached;

Copy link
Author

Choose a reason for hiding this comment

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

🟡 MEDIUM | static_defect

Potential NPE: idpDelegate.getForLogin() could return null, causing NPE in collect operation.

💡 Suggestion: Add null check before collecting stream.

Suggested change
Stream<IdentityProviderModel> idpStream = idpDelegate.getForLogin(mode, organizationId);
if (idpStream == null) {
cached = Set.of();
} else {
cached = idpStream.map(IdentityProviderModel::getInternalId).collect(Collectors.toSet());
}

cached = idpDelegate.getForLogin(mode, organizationId).map(IdentityProviderModel::getInternalId).collect(Collectors.toSet());
query = new IdentityProviderListQuery(loaded, cacheKey, getRealm(), searchKey, cached);
cache.addRevisioned(query, startupRevision);
} else {
Copy link
Author

Choose a reason for hiding this comment

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

🟡 MEDIUM | static_defect

Potential NPE: idpDelegate.getForLogin() could return null, causing NPE in collect operation.

💡 Suggestion: Add null check before collecting stream.

Suggested change
} else {
Stream<IdentityProviderModel> idpStream = idpDelegate.getForLogin(mode, organizationId);
if (idpStream == null) {
cached = Set.of();
} else {
cached = idpStream.map(IdentityProviderModel::getInternalId).collect(Collectors.toSet());
}

Comment on lines +242 to +244
}

Set<IdentityProviderModel> identityProviders = new HashSet<>();
Copy link
Author

Choose a reason for hiding this comment

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

🟡 MEDIUM | static_defect

Potential NPE: idpDelegate.getForLogin() could return null, causing NPE when calling .map().

💡 Suggestion: Add null check before mapping stream.

Suggested change
}
Set<IdentityProviderModel> identityProviders = new HashSet<>();
if (idp == null) {
realmCache.registerInvalidation(cacheKey);
Stream<IdentityProviderModel> result = idpDelegate.getForLogin(mode, organizationId);
return result != null ? result.map(this::createOrganizationAwareIdentityProviderModel) : Stream.empty();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants