Change methods that return Boolean to return boolean and correct bad …#968
Change methods that return Boolean to return boolean and correct bad …#968fallingrock wants to merge 1 commit into
Conversation
…string comparisons. Fixes meilisearch#967.
📝 WalkthroughWalkthroughThe Meilisearch Java SDK Client class is improved with two types of changes: method return types are simplified from wrapper ChangesClient method improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/meilisearch/sdk/Client.java (1)
513-524:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
optionsbefore dereferencing it.
generateTenantToken(..., TenantTokenOptions options)still throwsNullPointerExceptionwhen callers passnull, becauseoptions.getApiKey()is accessed before any validation. Defaultnulltonew TenantTokenOptions()or throwMeilisearchExceptionhere so this path stays inside the method's documented error model.Suggested fix
public String generateTenantToken( String apiKeyUid, Map<String, Object> searchRules, TenantTokenOptions options) throws MeilisearchException { + if (options == null) { + options = new TenantTokenOptions(); + } + // Validate all fields Date now = new Date(); String secret;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/meilisearch/sdk/Client.java` around lines 513 - 524, The method generateTenantToken currently dereferences options (e.g., options.getApiKey() and options.getExpiresAt()) before checking for null, causing NPEs; modify generateTenantToken to first guard options by either replacing a null with a new TenantTokenOptions() or immediately throwing a MeilisearchException with a clear message, then continue using options.getApiKey(), options.getExpiresAt(), secret etc.; ensure the chosen approach preserves the method's error model and adjust any subsequent logic that assumes options is non-null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/meilisearch/sdk/Client.java`:
- Around line 261-263: Client.isHealthy() must preserve binary
compatibility—change its return type back to Boolean (or provide an overload) so
callers compiled against the old signature don't get NoSuchMethodError; in the
tenant token path, avoid dereferencing options before null-checking: update
generateTenantToken(..., TenantTokenOptions options) to first validate options
!= null (and options.getApiKey() != null) and throw a MeilisearchException with
a clear message when missing; finally, tighten isValidUUID(String) to enforce
UUID v4 (parse with UUID.fromString(...), then assert uuid.version() == 4 and
uuid.variant() == 2) and return/throw a MeilisearchException with a
uuid4-specific error when it fails.
---
Outside diff comments:
In `@src/main/java/com/meilisearch/sdk/Client.java`:
- Around line 513-524: The method generateTenantToken currently dereferences
options (e.g., options.getApiKey() and options.getExpiresAt()) before checking
for null, causing NPEs; modify generateTenantToken to first guard options by
either replacing a null with a new TenantTokenOptions() or immediately throwing
a MeilisearchException with a clear message, then continue using
options.getApiKey(), options.getExpiresAt(), secret etc.; ensure the chosen
approach preserves the method's error model and adjust any subsequent logic that
assumes options is non-null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e11e557e-9890-469c-8102-095d80c26ae9
📒 Files selected for processing (1)
src/main/java/com/meilisearch/sdk/Client.java
| public boolean isHealthy() throws MeilisearchException { | ||
| return this.instanceHandler.isHealthy(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Java, is changing a public method return type from java.lang.Booleanto primitiveboolean binary compatible for already compiled consumers?
💡 Result:
No, changing a public method's return type from the wrapper class java.lang.Boolean to the primitive type boolean is not binary compatible with already compiled consumers [1][2]. According to the Java Language Specification (JLS), the return type of a method is a fundamental part of its signature as far as the Java Virtual Machine (JVM) and binary linking are concerned [3][4][5]. When a consumer is compiled, the method signature—including the exact return type—is recorded in the consumer's bytecode [6][5]. When you change the return type: 1. The method signature in the compiled class file changes [2]. 2. The JVM, at runtime, looks for a method with the original signature (which expects a return type of java.lang.Boolean) [6][5]. 3. Since the new method signature (expecting boolean) does not match, the link fails, typically resulting in a java.lang.NoSuchMethodError when the consumer attempts to invoke the method [7][6][2]. While this change might be source compatible (if the source code of the consumer is recompiled, the compiler will handle the necessary auto-boxing or unboxing to adapt to the change), it is fundamentally binary incompatible [1][2]. Pre-existing binaries will not be able to link against the new version of the class without being recompiled [8][2].
Citations:
- 1: https://www.jot.fm/issues/issue_2017_04/article2.pdf
- 2: https://ar5iv.labs.arxiv.org/html/1408.2607
- 3: https://web.cs.unlv.edu/CSC460/docs/draft5.2-html/13.doc.html
- 4: https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html
- 5: https://www.morling.dev/blog/refining-return-type-java-methods-without-breaking-backwards-compatibility/
- 6: https://stackoverflow.com/questions/20287268/binary-compatibility-issue-an-example
- 7: https://stackoverflow.com/questions/47476509/can-i-change-a-return-type-to-be-a-strict-subtype-and-retain-binary-compatibilit
- 8: https://docs.oracle.com/javase/specs/jls/se6/html/binaryComp.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify exact locations/lines referenced
echo "=== Client.java excerpts ==="
sed -n '230,290p' src/main/java/com/meilisearch/sdk/Client.java
echo "=== Client.generateTenantToken excerpts (approx lines) ==="
sed -n '480,560p' src/main/java/com/meilisearch/sdk/Client.java
echo "=== Client UUID-related methods excerpts (approx lines) ==="
sed -n '560,700p' src/main/java/com/meilisearch/sdk/Client.java
echo "=== InstanceHandler.java excerpt ==="
sed -n '1,120p' src/main/java/com/meilisearch/sdk/InstanceHandler.java
echo "=== TenantTokenOptions.java excerpt ==="
sed -n '1,120p' src/main/java/com/meilisearch/sdk/TenantTokenOptions.javaRepository: meilisearch/meilisearch-java
Length of output: 11342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Ripgrep for isValidUUID and uuid4 message ==="
rg -n "isValidUUID|uuid4|UUID\.fromString|generateTenantToken|TenantTokenOptions" src/main/java/com/meilisearch/sdk/Client.java
echo "=== Ripgrep for apiKey null initialization in TenantTokenOptions ==="
rg -n "apiKey" src/main/java/com/meilisearch/sdk/TenantTokenOptions.javaRepository: meilisearch/meilisearch-java
Length of output: 959
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/main/java/com/meilisearch/sdk/Client.java | sed -n '1,120p'Repository: meilisearch/meilisearch-java
Length of output: 4468
Fix binary compatibility and validation gaps in tenant token + health APIs
Client.isHealthy()usingbooleaninstead ofBooleanis binary-incompatible for already-compiled consumers (e.g.,NoSuchMethodError)—keepBooleanor treat as a breaking (major) change.generateTenantToken(..., TenantTokenOptions options)dereferencesoptions(options.getApiKey()) before any null check, sooptions == nullyields aNullPointerExceptioninstead of aMeilisearchException.- The UUIDv4 requirement is not enforced:
isValidUUID()only usesUUID.fromString(...)(syntax), while the error message requiresuuid4(version/variant).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/meilisearch/sdk/Client.java` around lines 261 - 263,
Client.isHealthy() must preserve binary compatibility—change its return type
back to Boolean (or provide an overload) so callers compiled against the old
signature don't get NoSuchMethodError; in the tenant token path, avoid
dereferencing options before null-checking: update generateTenantToken(...,
TenantTokenOptions options) to first validate options != null (and
options.getApiKey() != null) and throw a MeilisearchException with a clear
message when missing; finally, tighten isValidUUID(String) to enforce UUID v4
(parse with UUID.fromString(...), then assert uuid.version() == 4 and
uuid.variant() == 2) and return/throw a MeilisearchException with a
uuid4-specific error when it fails.
1 participant
Pull Request
Related issue
Fixes #967
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary
This PR updates the meilisearch-java client code to improve code quality by using primitive boolean types instead of Boolean wrapper objects and correcting invalid string equality comparisons.
Changes to Client.java
Method signature change:
isHealthy()method now returns primitivebooleaninstead ofBooleanString comparison fix in
generateTenantToken()method:== ""with properisEmpty()method call when validating the API key parameterAdditional validation enhancements in
generateTenantToken()method:secret) is non-null and longer than 8 characterssearchRulesparameter is non-nullapiKeyUidis non-null, non-empty, and complies with UUIDv4 format before JWT creation/encryptionImpact
These changes follow Java best practices by favoring primitive types over wrapper objects for boolean values and ensure proper string validation logic. The method behavior is maintained while improving code correctness and clarity.