Skip to content

Commit 8b94f1d

Browse files
adityamparikhclaude
adityamparikh
and
claude
committed
fix(collection): address code review findings across CollectionService
- Fix ClassCastException risk in CollectionUtils.getFloat by adding instanceof check and String fallback parsing, matching getLong/getInteger - Add extractCollectionName() call to checkHealth for shard name support - Populate collection field in SolrHealthStatus response - Eliminate redundant validateCollectionExists calls when getCacheMetrics and getHandlerMetrics are invoked from getCollectionStats (saves 3 extra listCollections() round-trips per call) - Replace System.out.println debug statements with SLF4J logger - Migrate integration test setup from fragile @BeforeEach/static-boolean to @BeforeAll with @testinstance(PER_CLASS) - Add String-based getFloat tests and collection field assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: adityamparikh <aditya.m.parikh@gmail.com>
1 parent 5fb7416 commit 8b94f1d

5 files changed

Lines changed: 101 additions & 43 deletions

File tree

‎src/main/java/org/apache/solr/mcp/server/collection/CollectionService.java‎

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -428,7 +428,7 @@ public SolrMetrics getCollectionStats(
428428
QueryResponse statsResponse = solrClient.query(actualCollection, new SolrQuery(ALL_DOCUMENTS_QUERY).setRows(0));
429429

430430
return new SolrMetrics(buildIndexStats(lukeResponse), buildQueryStats(statsResponse),
431-
getCacheMetrics(actualCollection), getHandlerMetrics(actualCollection), new Date());
431+
fetchCacheMetrics(actualCollection), fetchHandlerMetrics(actualCollection), new Date());
432432
}
433433

434434
/**
@@ -557,14 +557,22 @@ public QueryStats buildQueryStats(QueryResponse response) {
557557
* @see #isCacheStatsEmpty(CacheStats)
558558
*/
559559
public CacheStats getCacheMetrics(String collection) {
560-
try {
561-
String actualCollection = extractCollectionName(collection);
560+
String actualCollection = extractCollectionName(collection);
562561

563-
if (!validateCollectionExists(actualCollection)) {
564-
return null;
565-
}
562+
if (!validateCollectionExists(actualCollection)) {
563+
return null;
564+
}
566565

567-
NamedList<Object> coreMetrics = fetchMetrics(actualCollection, CACHE_METRIC_PREFIX);
566+
return fetchCacheMetrics(actualCollection);
567+
}
568+
569+
/**
570+
* Internal cache metrics fetch that assumes the collection has already been
571+
* validated and the name has been extracted from any shard identifier.
572+
*/
573+
private CacheStats fetchCacheMetrics(String collection) {
574+
try {
575+
NamedList<Object> coreMetrics = fetchMetrics(collection, CACHE_METRIC_PREFIX);
568576
if (coreMetrics == null) {
569577
return null;
570578
}
@@ -661,18 +669,26 @@ private CacheInfo extractSingleCacheInfo(NamedList<Object> coreMetrics, String k
661669
* @see #isHandlerStatsEmpty(HandlerStats)
662670
*/
663671
public HandlerStats getHandlerMetrics(String collection) {
664-
try {
665-
String actualCollection = extractCollectionName(collection);
672+
String actualCollection = extractCollectionName(collection);
666673

667-
if (!validateCollectionExists(actualCollection)) {
668-
return null;
669-
}
674+
if (!validateCollectionExists(actualCollection)) {
675+
return null;
676+
}
670677

678+
return fetchHandlerMetrics(actualCollection);
679+
}
680+
681+
/**
682+
* Internal handler metrics fetch that assumes the collection has already been
683+
* validated and the name has been extracted from any shard identifier.
684+
*/
685+
private HandlerStats fetchHandlerMetrics(String collection) {
686+
try {
671687
// Handler metrics are flat keys (e.g. QUERY./select.requests) so we
672688
// fetch each handler prefix separately and reconstruct HandlerInfo
673-
HandlerInfo selectHandler = fetchFlatHandlerInfo(actualCollection, SELECT_HANDLER_METRIC_PREFIX,
689+
HandlerInfo selectHandler = fetchFlatHandlerInfo(collection, SELECT_HANDLER_METRIC_PREFIX,
674690
SELECT_HANDLER_KEY);
675-
HandlerInfo updateHandler = fetchFlatHandlerInfo(actualCollection, UPDATE_HANDLER_METRIC_PREFIX,
691+
HandlerInfo updateHandler = fetchFlatHandlerInfo(collection, UPDATE_HANDLER_METRIC_PREFIX,
676692
UPDATE_HANDLER_KEY);
677693

678694
HandlerStats stats = new HandlerStats(selectHandler, updateHandler);
@@ -941,18 +957,20 @@ private boolean validateCollectionExists(String collection) {
941957
*/
942958
@McpTool(name = "check-health", description = "Check health of a Solr collection")
943959
public SolrHealthStatus checkHealth(@McpToolParam(description = "Solr collection") String collection) {
960+
String actualCollection = extractCollectionName(collection);
944961
try {
945962
// Ping Solr
946-
SolrPingResponse pingResponse = solrClient.ping(collection);
963+
SolrPingResponse pingResponse = solrClient.ping(actualCollection);
947964

948965
// Get basic stats
949-
QueryResponse statsResponse = solrClient.query(collection, new SolrQuery(ALL_DOCUMENTS_QUERY).setRows(0));
966+
QueryResponse statsResponse = solrClient.query(actualCollection,
967+
new SolrQuery(ALL_DOCUMENTS_QUERY).setRows(0));
950968

951969
return new SolrHealthStatus(true, null, pingResponse.getElapsedTime(),
952-
statsResponse.getResults().getNumFound(), new Date(), null, null, null);
970+
statsResponse.getResults().getNumFound(), new Date(), actualCollection, null, null);
953971

954972
} catch (Exception e) {
955-
return new SolrHealthStatus(false, e.getMessage(), null, null, new Date(), null, null, null);
973+
return new SolrHealthStatus(false, e.getMessage(), null, null, new Date(), actualCollection, null, null);
956974
}
957975
}
958976

‎src/main/java/org/apache/solr/mcp/server/collection/CollectionUtils.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,18 @@ public static Long getLong(NamedList<Object> response, String key) {
182182
*/
183183
public static Float getFloat(NamedList<Object> stats, String key) {
184184
Object value = stats.get(key);
185-
return value != null ? ((Number) value).floatValue() : 0.0f;
185+
if (value == null)
186+
return null;
187+
188+
if (value instanceof Number number) {
189+
return number.floatValue();
190+
}
191+
192+
try {
193+
return Float.parseFloat(value.toString());
194+
} catch (NumberFormatException _) {
195+
return null;
196+
}
186197
}
187198

188199
/**

‎src/test/java/org/apache/solr/mcp/server/collection/CollectionServiceIntegrationTest.java‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,11 @@
2222
import org.apache.solr.client.solrj.SolrClient;
2323
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
2424
import org.apache.solr.mcp.server.TestcontainersConfiguration;
25-
import org.junit.jupiter.api.BeforeEach;
25+
import org.junit.jupiter.api.BeforeAll;
2626
import org.junit.jupiter.api.Test;
27+
import org.junit.jupiter.api.TestInstance;
28+
import org.slf4j.Logger;
29+
import org.slf4j.LoggerFactory;
2730
import org.springframework.beans.factory.annotation.Autowired;
2831
import org.springframework.boot.test.context.SpringBootTest;
2932
import org.springframework.context.annotation.Import;
@@ -32,41 +35,36 @@
3235
@SpringBootTest
3336
@Import(TestcontainersConfiguration.class)
3437
@Testcontainers(disabledWithoutDocker = true)
38+
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
3539
class CollectionServiceIntegrationTest {
3640

41+
private static final Logger log = LoggerFactory.getLogger(CollectionServiceIntegrationTest.class);
42+
3743
private static final String TEST_COLLECTION = "test_collection";
3844
@Autowired
3945
private CollectionService collectionService;
4046
@Autowired
4147
private SolrClient solrClient;
42-
private static boolean initialized = false;
4348

44-
@BeforeEach
49+
@BeforeAll
4550
void setupCollection() throws Exception {
51+
CollectionAdminRequest.Create createRequest = CollectionAdminRequest.createCollection(TEST_COLLECTION,
52+
"_default", 1, 1);
53+
createRequest.process(solrClient);
4654

47-
if (!initialized) {
48-
// Create a test collection using the container's connection details
49-
// Create a collection for testing
50-
CollectionAdminRequest.Create createRequest = CollectionAdminRequest.createCollection(TEST_COLLECTION,
51-
"_default", 1, 1);
52-
createRequest.process(solrClient);
53-
54-
// Verify collection was created successfully
55-
CollectionAdminRequest.List listRequest = new CollectionAdminRequest.List();
56-
listRequest.process(solrClient);
55+
// Verify collection was created successfully
56+
CollectionAdminRequest.List listRequest = new CollectionAdminRequest.List();
57+
listRequest.process(solrClient);
5758

58-
System.out.println("[DEBUG_LOG] Test collection created: " + TEST_COLLECTION);
59-
initialized = true;
60-
}
59+
log.debug("Test collection created: {}", TEST_COLLECTION);
6160
}
6261

6362
@Test
6463
void testListCollections() {
6564
// Test listing collections
6665
List<String> collections = collectionService.listCollections();
6766

68-
// Print the collections for debugging
69-
System.out.println("[DEBUG_LOG] Collections: " + collections);
67+
log.debug("Collections: {}", collections);
7068

7169
// Enhanced assertions for collections list
7270
assertNotNull(collections, "Collections list should not be null");
@@ -149,8 +147,7 @@ void testCheckHealthHealthy() {
149147
// Test checking health of a valid collection
150148
SolrHealthStatus status = collectionService.checkHealth(TEST_COLLECTION);
151149

152-
// Print the status for debugging
153-
System.out.println("[DEBUG_LOG] Health status for valid collection: " + status);
150+
log.debug("Health status for valid collection: {}", status);
154151

155152
// Enhanced assertions for healthy collection
156153
assertNotNull(status, "Health status should not be null");
@@ -175,6 +172,9 @@ void testCheckHealthHealthy() {
175172
// Verify no error message for healthy collection
176173
assertNull(status.errorMessage(), "Error message should be null for healthy collection");
177174

175+
// Verify collection name is populated
176+
assertEquals(TEST_COLLECTION, status.collection(), "Collection name should be populated in health status");
177+
178178
// Verify string representation contains meaningful information
179179
String statusString = status.toString();
180180
assertTrue(statusString.contains("healthy") || statusString.contains("true"),
@@ -187,8 +187,7 @@ void testCheckHealthUnhealthy() {
187187
String nonExistentCollection = "non_existent_collection";
188188
SolrHealthStatus status = collectionService.checkHealth(nonExistentCollection);
189189

190-
// Print the status for debugging
191-
System.out.println("[DEBUG_LOG] Health status for invalid collection: " + status);
190+
log.debug("Health status for invalid collection: {}", status);
192191

193192
// Enhanced assertions for unhealthy collection
194193
assertNotNull(status, "Health status should not be null");
@@ -210,6 +209,10 @@ void testCheckHealthUnhealthy() {
210209
assertNull(status.responseTime(), "Response time should be null for unhealthy collection");
211210
assertNull(status.totalDocuments(), "Total documents should be null for unhealthy collection");
212211

212+
// Verify collection name is populated even for unhealthy collection
213+
assertEquals(nonExistentCollection, status.collection(),
214+
"Collection name should be populated for unhealthy collection");
215+
213216
// Verify error message contains meaningful information
214217
String errorMessage = status.errorMessage().toLowerCase();
215218
assertTrue(

‎src/test/java/org/apache/solr/mcp/server/collection/CollectionServiceTest.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,7 @@ void checkHealth_WithHealthyCollection_ShouldReturnHealthyStatus() throws Except
200200
assertNull(result.errorMessage());
201201
assertEquals(10L, result.responseTime());
202202
assertEquals(100L, result.totalDocuments());
203+
assertEquals("test_collection", result.collection());
203204
}
204205

205206
@Test
@@ -217,6 +218,7 @@ void checkHealth_WithUnhealthyCollection_ShouldReturnUnhealthyStatus() throws Ex
217218
assertTrue(result.errorMessage().contains("Connection failed"));
218219
assertNull(result.responseTime());
219220
assertNull(result.totalDocuments());
221+
assertEquals("unhealthy_collection", result.collection());
220222
}
221223

222224
@Test

‎src/test/java/org/apache/solr/mcp/server/collection/CollectionUtilsTest.java‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,14 +123,14 @@ void testGetFloat_withNullValue() {
123123
NamedList<Object> namedList = new NamedList<>();
124124
namedList.add("nullKey", null);
125125

126-
assertEquals(0.0f, CollectionUtils.getFloat(namedList, "nullKey"));
126+
assertNull(CollectionUtils.getFloat(namedList, "nullKey"));
127127
}
128128

129129
@Test
130130
void testGetFloat_withMissingKey() {
131131
NamedList<Object> namedList = new NamedList<>();
132132

133-
assertEquals(0.0f, CollectionUtils.getFloat(namedList, "missingKey"));
133+
assertNull(CollectionUtils.getFloat(namedList, "missingKey"));
134134
}
135135

136136
@Test
@@ -173,6 +173,30 @@ void testGetFloat_withBigDecimalValue() {
173173
assertEquals(123.45f, CollectionUtils.getFloat(namedList, "bigDecKey"), 0.001f);
174174
}
175175

176+
@Test
177+
void testGetFloat_withValidStringValue() {
178+
NamedList<Object> namedList = new NamedList<>();
179+
namedList.add("stringKey", "123.45");
180+
181+
assertEquals(123.45f, CollectionUtils.getFloat(namedList, "stringKey"), 0.001f);
182+
}
183+
184+
@Test
185+
void testGetFloat_withInvalidStringValue() {
186+
NamedList<Object> namedList = new NamedList<>();
187+
namedList.add("invalidStringKey", "not_a_number");
188+
189+
assertNull(CollectionUtils.getFloat(namedList, "invalidStringKey"));
190+
}
191+
192+
@Test
193+
void testGetFloat_withEmptyStringValue() {
194+
NamedList<Object> namedList = new NamedList<>();
195+
namedList.add("emptyStringKey", "");
196+
197+
assertNull(CollectionUtils.getFloat(namedList, "emptyStringKey"));
198+
}
199+
176200
@Test
177201
void testGetInteger_withNullValue() {
178202
NamedList<Object> namedList = new NamedList<>();

0 commit comments

Comments
 (0)