Dev/1272 item categories testing#1279
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
📝 WalkthroughWalkthroughAhoy there! This PR adds two hearty test methods to ChangesItemCategories Integration Test
Possibly Related Issues
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
i will rebase before merge, just want to make clear. #1279 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
software/core/oqm-core-api/src/test/java/tech/ebp/oqm/core/api/interfaces/endpoints/inventory/ItemCategoriesCrudTest.java (1)
34-40: ⚡ Quick winBatton down this test with response-body checks.
Line 39 only verifies
200, matey; that can pass even if the endpoint returns malformed or wrong cargo. Add assertions on returned JSON fields (name,description, maybe generated id) so this integration test guards the create contract, not just status.⚓ Proposed tightening
- setupJwtCall(given(), testUser.getAttributes().get(TestUserService.TEST_JWT_ATT_KEY)) + String requestName = "Test Category"; + String requestDescription = "A category created during testing."; + setupJwtCall(given(), testUser.getAttributes().get(TestUserService.TEST_JWT_ATT_KEY)) - .body(objectMapper.writeValueAsString(new ItemCategory().setName("Test Category").setDescription("A category created during testing."))) + .body(objectMapper.writeValueAsString(new ItemCategory().setName(requestName).setDescription(requestDescription))) .contentType(ContentType.JSON) .pathParam("oqmDbIdOrName", DEFAULT_TEST_DB_NAME) .post() - .then().statusCode(200) - .extract().response().asString(); + .then() + .statusCode(200) + .body("name", org.hamcrest.Matchers.equalTo(requestName)) + .body("description", org.hamcrest.Matchers.equalTo(requestDescription)) + .body("id", org.hamcrest.Matchers.notNullValue());
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e68600d6-a20d-4617-9f5a-d548cf9c6269
📒 Files selected for processing (2)
software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoService.javasoftware/core/oqm-core-api/src/test/java/tech/ebp/oqm/core/api/interfaces/endpoints/inventory/ItemCategoriesCrudTest.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI-Pipeline / Build
🧰 Additional context used
🪛 PMD (7.24.0)
software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoService.java
[Low] 145-145: InvalidLogMessageFormat (Error Prone): Too many arguments, expected 1 argument but found 2
(InvalidLogMessageFormat (Error Prone))
|
|
||
| public abstract int getCurrentSchemaVersion(); | ||
|
|
||
| public abstract List<Bson> getDbIndexes(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoService.java"
# Sanity check file exists
test -f "$FILE"
# Show around the mentioned areas (keep output small)
nl -ba "$FILE" | sed -n '90,190p'
# Find any usage of getDbIndexes and IndexOptions in the file
rg -n "getDbIndexes\s*\(|IndexOptions|recreate|drop|createIndex|IndexModel|listIndexes|indexes" "$FILE" || true
# Find interface/implementations of getDbIndexes within the repo
rg -n "getDbIndexes\s*\(" software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service -S || true
# If MongoService is abstract class, locate subclasses/overrides
rg -n "extends\s+MongoService|class\s+.*MongoService" software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service -S || trueRepository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 132
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoService.java"
test -f "$FILE"
# Print small windows with line numbers (without nl)
awk -v start=80 -v end=190 'NR>=start && NR<=end {printf "%d:%s\n", NR, $0}' "$FILE"
echo "---- rg: getDbIndexes usage / recreate path ----"
rg -n "getDbIndexes\s*\(|IndexOptions|recreate|drop|createIndex|IndexModel|listIndexes|indexes" "$FILE" || true
echo "---- rg: getDbIndexes implementations in service package ----"
rg -n "getDbIndexes\s*\(" software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service -S || true
echo "---- rg: IndexModel / creation patterns in mongo service package ----"
rg -n "IndexModel|createIndex|createIndexes|listIndexes|dropIndexes|IndexOptions" software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service -S || trueRepository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 8713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show setupIndexes + getExistingIndexes bodies (more context)
FILE="software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoService.java"
awk 'NR>=120 && NR<=210 {printf "%d:%s\n", NR, $0}' "$FILE"
echo "---- getDbIndexes implementations (small windows) ----"
for f in \
software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoDbAwareService.java \
software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/InventoryItemService.java \
software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/StorageBlockService.java \
software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/TopLevelMongoService.java \
software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/ItemListService.java
do
test -f "$f" || continue
echo "## $f"
# Print around first occurrence of getDbIndexes()
start=$(rg -n "List<Bson>\s+getDbIndexes\s*\(" "$f" | head -n1 | cut -d: -f1)
if [ -n "${start:-}" ]; then
s=$((start-15)); e=$((start+60));
awk -v s="$s" -v e="$e" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$f"
fi
doneRepository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 7141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "List<\s*Bson\s*>\s+getDbIndexes\s*\(" . -S || trueRepository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 1106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "List<\s*Bson\s*>\s+getDbIndexes\s*\(" . -S || trueRepository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 1106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/serviceState/db/OqmDatabaseService.java"
awk 'NR>=160 && NR<=240 {printf "%d:%s\n", NR, $0}' "$FILE"Repository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 1076
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/serviceState/db/OqmDatabaseService.java"
awk 'NR>=160 && NR<=240 {printf "%d:%s\n", NR, $0}' "$FILE"Repository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 1076
Fix Mongo index setup: key-only getDbIndexes() can’t preserve index options
MongoService.setupIndexes() matches existing indexes only by the index "key" (via listIndexes() → doc.get("key")) and recreates using a hard-coded IndexOptions options = new IndexOptions().background(true). Since getDbIndexes() only provides List<Bson> keys, unique, TTL (expireAfter), partial/sparse, collation, etc. can be lost when an existing index has different options (drop + recreate with only background(true)).
This is already wired as “options are always up to date”, but the implementation can’t actually do that (see getDbIndexes() line ~119 and setupIndexes() lines ~133-147).
| if (existingIndexes.containsKey(expectedKey)) { | ||
| try { | ||
| collection.createIndex(index, options); | ||
| } catch (MongoCommandException e) { | ||
| log.warn("failed to create index with key {}, dropping and recreating index", expectedKey, e); | ||
| collection.dropIndex(existingIndexes.get(expectedKey)); | ||
| collection.createIndex(index, options); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant code around the reported lines
sed -n '110,180p' software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoService.java
# Identify how existingIndexes/expectedKey/index/options are computed in this method (wider context)
sed -n '1,260p' software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoService.java | sed -n '120,220p'
# Find all MongoCommandException handling in this file/project for similar patterns
rg -n "MongoCommandException" software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoService.java
rg -n "dropIndex\\(" software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo -SRepository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 6213
🌐 Web query:
MongoDB error code IndexOptionsConflict createIndex different options MongoCommandException code name
💡 Result:
The IndexOptionsConflict error (error code 85) occurs in MongoDB when you attempt to create or define an index that already exists, but the requested index options (such as unique, sparse, or other configuration settings) differ from the existing index [1][2]. Key points regarding this error: Cause: You are trying to create an index with the same name or the same key pattern as an existing index, but with a different configuration [1][3]. MongoDB requires index options to match exactly for an existing index key pattern [1][4]. Even minor differences, such as changing a unique index to a non-unique one, will trigger this error [5]. Resolution: 1. Drop the existing index: In most cases, the standard solution is to drop the existing index using db.collection.dropIndex("index_name") and then recreate it with the desired options [1][3][4]. 2. Use collMod for specific cases: For some options, such as expireAfterSeconds (TTL indexes), you may be able to update the value using the collMod database command instead of dropping the index [3][6]. 3. Check for naming conflicts: The error can also occur if you attempt to create an index with a name that is already taken by a different index, or if you accidentally attempt to create multiple text indexes, as only one text index is allowed per collection [5][1]. If you are encountering this in an application (such as Mongoose or a driver-based implementation), it is often because the application code defines an index (e.g., via a schema model) that conflicts with an index already present in the database [5][7]. In development environments, this is sometimes resolved by dropping the entire collection if the data is not critical, though dropping the specific index is the safer, surgical approach [5].
Citations:
- 1: https://www.mongodb.com/docs/manual/reference/command/createindexes/
- 2: https://www.mongodb.com/docs/manual/reference/error-codes/
- 3: https://stackoverflow.com/questions/56281241/can-indexoptionsconflict-be-avoided-to-change-expiredafterseconds-in-mongodb-ind
- 4: https://www.mongodb.com/docs/manual/reference/method/db.collection.createindex/
- 5: https://stackoverflow.com/questions/30232081/mongoexception-index-with-name-code-already-exists-with-different-options
- 6: https://www.mongodb.com/docs/manual/tutorial/manage-indexes/
- 7: MongoError: Index with name: username_1 already exists with different options meteor/meteor#11666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoService.java"
# See if any error-code checks already exist in this file
rg -n "getErrorCode\\(|getErrorName\\(|getErrorMessage\\(|ErrorCode|IndexOptionsConflict" "$FILE" || true
# Search broader for similar patterns in the repository (limited to Java)
rg -n "MongoCommandException" software/core/oqm-core-api/src/main/java -S
rg -n "getErrorCode\\(|IndexOptionsConflict|errorCode\\s*==|ErrorCode" software/core/oqm-core-api/src/main/java -SRepository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Broader scan across repository (Java/Kotlin) for error-code based handling
rg -n "catch \\(MongoCommandException" -S . || true
rg -n "MongoCommandException.*getErrorCode\\(|getErrorCode\\(\\).*MongoCommandException|IndexOptionsConflict|errorCode\\s*\\(" -S . || true
rg -n "getErrorCode\\(\\)" software -S || trueRepository: Epic-Breakfast-Productions/OpenQuarterMaster
Length of output: 241
🌐 Web query:
MongoDB MongoCommandException getErrorCode getErrorName IndexOptionsConflict error code 85 Java driver
💡 Result:
MongoDB error code 85 is IndexOptionsConflict, surfaced to Java as MongoCommandException when an index with the same name already exists but with different options. 1) What it means - MongoDB documents error code 85 as IndexOptionsConflict [1]. - The server message is typically of the form: “Index with name: already exists with different options” and includes both the error code (85) and codeName (IndexOptionsConflict) [2]. - Example: creating an index again with the same key spec but different index options (commonly unique, collation, partial filter, TTL expiredAfterSeconds, etc.) triggers the conflict [3][4]. 2) How to read it in the Java driver - MongoCommandException provides getErrorCode, getErrorCodeName and getErrorMessage [5]. Example (sync driver): try { collection.createIndex(keys, new IndexOptions.unique(true)); } catch (MongoCommandException e) { if (e.getErrorCode == 85 || "IndexOptionsConflict".equals(e.getErrorCodeName)) { // handle index options conflict } } 3) Typical causes - Your application calls createIndex (or an annotation-based index creation) multiple times with different IndexOptions, so the second attempt tries to “recreate” an existing named index with different options [3][6][7]. - You cannot change certain options by re-running createIndex: if you recreate the same index with different options (except collation), MongoDB will not change the index options; you must drop and recreate [8][7]. 4) How to fix - Determine the existing index definition/options, then make your code match them (same name + same options). - Or drop the conflicting index and recreate it with the desired options. - For “can’t change TTL/expiredAfterSeconds” scenarios: you generally must drop and recreate the index with the new expireAfterSeconds value (no createIndex override) [4]. If you paste the exact exception text (including the index name and the options it says are requested vs existing) and the Java code that creates the index (keys + IndexOptions fields you set), I can tell you exactly which option(s) differ and the safest remediation plan.
Citations:
- 1: https://www.mongodb.com/docs/manual/reference/error-codes/
- 2: https://wenku.csdn.net/answer/76544be243c84fd3b6f5a438c853edf8
- 3: https://stackoverflow.com/questions/27288373/index-already-exists-with-different-options-error-while-using-createindex-in-l
- 4: https://stackoverflow.com/questions/56281241/can-indexoptionsconflict-be-avoided-to-change-expiredafterseconds-in-mongodb-ind
- 5: https://mongodb.github.io/mongo-java-driver/4.1/apidocs/mongodb-driver-core/com/mongodb/MongoCommandException.html
- 6: https://www.mongodb.com/docs/manual/core/indexes/create-index/
- 7: https://www.mongodb.com/docs/manual/reference/method/db.collection.createIndex/index.html
- 8: https://www.mongodb.com/docs/v8.0/reference/method/db.collection.createindex/
Only drop/recreate the index on MongoDB “IndexOptionsConflict” (code 85)
Ye be “droppin’ anchor” on any MongoCommandException in MongoService.setupIndexes (lines 141–147) — that could trash a perfectly good index if the create failed for some other reason (not an options conflict). Split the catch: only dropIndex(...) + retry when e.getErrorCode() == 85 / e.getErrorCodeName() is IndexOptionsConflict; otherwise, rethrow (or handle) without deleting the existing index.
🧰 Tools
🪛 PMD (7.24.0)
[Low] 145-145: InvalidLogMessageFormat (Error Prone): Too many arguments, expected 1 argument but found 2
(InvalidLogMessageFormat (Error Prone))
| for (Map.Entry<BsonDocument, String> existing : existingIndexes.entrySet()) { | ||
| if (!expectedKeys.contains(existing.getKey())) { | ||
| collection.dropIndex(existing.getValue()); | ||
| } |
There was a problem hiding this comment.
Don't scuttle undeclared indexes by default.
As wired through MongoDbAwareService.initDbByIdOrName(...), this method is called with getDbIndexes(), and that contract defaults to List.of() in software/core/oqm-core-api/src/main/java/tech/ebp/oqm/core/api/service/mongo/MongoDbAwareService.java:180-187. With an empty list, this loop drops every non-_id_ index on startup. Gate the prune step behind explicit opt-in, or treat an empty expected set as “leave the existing rigging alone.”
|
i can't update this message #1272 (comment) ,so, i will use this message to track the task by filling checkboxes Endpoints:
... more / the rest of the Item Categories endpoints |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
software/core/oqm-core-api/src/test/java/tech/ebp/oqm/core/api/interfaces/endpoints/inventory/ItemCategoriesCrudTest.java (2)
43-54: ⚡ Quick winAhoy! These two tests be sailin' the same course, matey!
I spy a fair bit o' duplicated rigging betwixt this here
createInvalidTestand its matecreateTestabove. Both be settin' up the JWT call, configuring the content type, and extractin' the response in much the same fashion, arr!Consider haulin' that common setup into a private helper method, somethin' like
executeCreateRequest(ItemCategory category). That way, when the winds change and we need to adjust our authentication or path parameters, ye'll only need to batten down one hatch instead o' two!⚓ Proposed refactor to reduce duplication
+ private String executeCreateRequest(ItemCategory category) throws JsonProcessingException { + User testUser = this.getTestUserService().getTestUser(); + + return setupJwtCall(given(), testUser.getAttributes().get(TestUserService.TEST_JWT_ATT_KEY)) + .body(objectMapper.writeValueAsString(category)) + .contentType(ContentType.JSON) + .pathParam("oqmDbIdOrName", DEFAULT_TEST_DB_NAME) + .post() + .extract().response(); + } + `@Test` void createTest() throws JsonProcessingException { - User testUser = this.getTestUserService().getTestUser(); - - setupJwtCall(given(), testUser.getAttributes().get(TestUserService.TEST_JWT_ATT_KEY)) - .body(objectMapper.writeValueAsString(new ItemCategory().setName("Test Category").setDescription("A category created during testing."))) - .contentType(ContentType.JSON) - .pathParam("oqmDbIdOrName", DEFAULT_TEST_DB_NAME) - .post() + executeCreateRequest(new ItemCategory().setName("Test Category").setDescription("A category created during testing.")) .then().statusCode(200); - .extract().response().asString(); } `@Test` void createInvalidTest() throws JsonProcessingException { - User testUser = this.getTestUserService().getTestUser(); - - setupJwtCall(given(), testUser.getAttributes().get(TestUserService.TEST_JWT_ATT_KEY)) - .body(objectMapper.writeValueAsString(new ItemCategory().setName("").setDescription(""))) - .contentType(ContentType.JSON) - .pathParam("oqmDbIdOrName", DEFAULT_TEST_DB_NAME) - .post() + executeCreateRequest(new ItemCategory().setName("").setDescription("")) .then().statusCode(400); - .extract().response().asString(); }
47-53: ⚡ Quick winVerify the error message in the hold, sailor!
Yer test be checkin' fer a 400 status code when the payload be foul, which be shipshape! But ye be extractin' the response body and then lettin' it drift away into the briny deep without inspectin' its cargo!
Consider addin' an assertion on the response body to verify it contains a proper validation error message. This'll give ye more confidence that the endpoint be reportin' the problem clearly to those callin' upon it, instead of just returnin' an empty hold with a 400 flag flyin'.
🔍 Proposed enhancement to verify error message
setupJwtCall(given(), testUser.getAttributes().get(TestUserService.TEST_JWT_ATT_KEY)) .body(objectMapper.writeValueAsString(new ItemCategory().setName("").setDescription(""))) .contentType(ContentType.JSON) .pathParam("oqmDbIdOrName", DEFAULT_TEST_DB_NAME) .post() .then().statusCode(400) + .body(org.hamcrest.Matchers.not(org.hamcrest.Matchers.emptyString())) .extract().response().asString();Or even better, verify the error mentions validation:
setupJwtCall(given(), testUser.getAttributes().get(TestUserService.TEST_JWT_ATT_KEY)) .body(objectMapper.writeValueAsString(new ItemCategory().setName("").setDescription(""))) .contentType(ContentType.JSON) .pathParam("oqmDbIdOrName", DEFAULT_TEST_DB_NAME) .post() .then().statusCode(400) + .body(org.hamcrest.Matchers.containsString("validation")) .extract().response().asString();
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad3bea27-1db3-4dcb-995a-2a95929edb9d
📒 Files selected for processing (1)
software/core/oqm-core-api/src/test/java/tech/ebp/oqm/core/api/interfaces/endpoints/inventory/ItemCategoriesCrudTest.java
GregJohnStewart
left a comment
GregJohnStewart
left a comment
There was a problem hiding this comment.
Let's sync on some ideas and themes for this. Good start, just going to point to some things
There was a problem hiding this comment.
Looks like this was added by mistake? Shouldn't this be in the other PR?
| void createTest() throws JsonProcessingException { | ||
| User testUser = this.getTestUserService().getTestUser(); | ||
|
|
||
| setupJwtCall(given(), testUser.getAttributes().get(TestUserService.TEST_JWT_ATT_KEY)) |
There was a problem hiding this comment.
Should also verify the result data
| void createInvalidTest() throws JsonProcessingException { | ||
| User testUser = this.getTestUserService().getTestUser(); | ||
|
|
||
| setupJwtCall(given(), testUser.getAttributes().get(TestUserService.TEST_JWT_ATT_KEY)) |
There was a problem hiding this comment.
Should also should verify result data
axgiri
force-pushed
the
dev/1272-item-categories-testing
branch
from
013d82d to
983f975
Compare
2 participants
Checklist:
Summary by CodeRabbit