RANCHER-3121 Reindex return ID safe guard check

[eldiiar-duishenaliev]

No description provided.

[github-actions]

Code Review Summary

Overall Score: 72/100 — Good defensive programming with solid improvements, but a few maintainability concerns need attention before merging.

---

✅ Strengths

• Good defensive programming: Adds null-safety checks for API responses that may not return expected fields
• Improved clarity: Refactored control flow is more explicit and easier to follow
• Better observability: Added informative warning and info messages for debugging
• Helpful documentation: TODO comment clarifies the design inconsistency between instance and generic index flows

---

🔴 High Priority

Library version pinned to feature branch

File: pipelines/folioRancher/folioNamespaceTools/manageNamespace/createNamespaceFromBranch/Jenkinsfile:15

The shared library is pinned to @RANCHER-3121 (a feature branch), which is problematic:

• Feature branches are temporary and may be deleted, breaking the build
• Creates hidden dependencies not obvious to future maintainers
• Prevents the pipeline from using shared library updates

Action: Either revert to the default branch or pin to a stable release tag with clear documentation. If temporary, add a comment explaining the expected duration.

---

🟡 Medium Priority

Inconsistent return types create API contract confusion

File: src/org/folio/rest_v2/eureka/kong/Indexes.groovy:24-31

The runIndexFlow() method returns null for instance indexes but a String jobId for generic indexes. This implicit contract is error-prone and not reflected in the method signature.

Suggested fixes:

1. Create a wrapper return type (e.g., class IndexResult { String jobId; boolean hasJobId })
2. Split into two methods with explicit return types
3. Use Optional<String> and add @Nullable/@Nonnull annotations with updated JavaDoc

Logging warning but continuing silently may mask issues

File: src/org/folio/rest_v2/eureka/kong/Indexes.groovy:73-82

When the reindex endpoint returns no job ID, the code logs a warning and returns null. However, callers may not be checking for null returns, leading to silent failures.

Suggested fixes:

• Audit all callers of runIndexFlow() to verify they handle null returns correctly
• Consider throwing an exception instead of warning in cases where a jobId is expected
• Add documentation clarifying which callers can safely ignore null returns

---

📋 Additional Recommendations

• Add unit tests to verify null-handling behavior for edge cases
• Document the API contract inconsistency between instance and generic index endpoints

[github-actions]

Code Review Summary

Overall Score: 72/100 — Good defensive programming, but requires attention to a critical null-safety issue before merging.

---

✅ Strengths

• Excellent use of null-safe operators (?.) to handle empty response bodies
• Clear logging added for diagnosing missing job IDs
• Improved code readability and formatting
• More specific TODO comment clarifying the underlying design issue

---

🔴 Critical Issues

Potential NullPointerException in checkIndexStatus call

File: src/org/folio/rest_v2/eureka/kong/Indexes.groovy (Line 82-84)

When jobId is null, the code still calls checkIndexStatus(tenant, jobId) if index.getWaitComplete() is true. This will cause a runtime exception.

Fix:

if (index.getWaitComplete() && jobId) {
  checkIndexStatus(tenant, jobId)
}

---

🟡 Medium Priority Issues

1. Inconsistent Return Type Behavior

The runIndexFlow method returns String for generic indexes but null for instance indexes. This creates a confusing API contract and increases NPE risk in callers.

Recommendation: Consider using Optional<String> or a wrapper class to make nullability explicit and self-documenting.

2. Missing JavaDoc

The method now has complex conditional behavior but lacks documentation explaining when null is returned and how callers should handle it.

Recommendation: Add JavaDoc clearly documenting return behavior for each code path.

---

📋 Top Recommendations

1. [MUST] Add null guard before checkIndexStatus() call
2. Refactor return type to use Optional<String> for clarity
3. Add comprehensive JavaDoc to runIndexFlow method
4. Add unit tests covering null response body and missing 'id' field scenarios