Fixes #1 - Merchant validation and Error handling#2
Conversation
When the merchant id is not present and given the id is a proper UUID, the default string to UUID is handled by the Spring itself
The phone number validation is just a regex for making sure it has 10 to 15 digits with no space after country code or inbetween. The error handlings are included for string to UUID inclusion errors where it doesn't accept non uuids on the merchant id and it is not evident on the response, same for the date format, though a regex is added it still doesn't verify the date within range. Few other fixes could be since registration date should be less than the current date, we should probably restrict any dates from today being provided. But for now let's leave to CodeRabbit and see what all suggestions it comes up with
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds regex validation for registeredDate and phoneNumber, new exception handlers for UUID and date parse errors, fixes the controller delete call, updates MerchantService to verify existence before delete, and adds unit tests for delete behavior. ChangesMerchant Validation and Error Handling
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
These are sample tests again to see what else coderabbit suggests and this for now includes tests only related to parts of changes made in this PR
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
merchant-service/src/test/java/com/mert/merchantservice/service/MerchantServiceTest.java (1)
26-48: ⚡ Quick winConsider adding a null ID test case.
The test suite covers valid and not-found scenarios but doesn't test null ID input. Given that the service has a null-check guard (line 77 in MerchantService.java), adding this test would improve coverage and immediately catch the critical inverted-logic bug present in the current implementation.
🧪 Suggested test case
`@Test` void testMerchantService_nullIdProvided_shouldThrowIllegalArgumentException() { assertThrows( IllegalArgumentException.class, () -> merchantService.deleteMerchant(null) ); verify(merchantRepository, never()).findById(any()); verify(merchantRepository, never()).delete(any()); }🤖 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 `@merchant-service/src/test/java/com/mert/merchantservice/service/MerchantServiceTest.java` around lines 26 - 48, Add a unit test for the null-ID branch of MerchantService.deleteMerchant: in MerchantServiceTest add a test (e.g., testMerchantService_nullIdProvided_shouldThrowIllegalArgumentException) that calls merchantService.deleteMerchant(null) and asserts IllegalArgumentException is thrown, and verifies merchantRepository.findById(...) and merchantRepository.delete(...) are never invoked; this will exercise the null-check guard in MerchantService.deleteMerchant and catch the inverted-logic bug.
🤖 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
`@merchant-service/src/main/java/com/mert/merchantservice/service/MerchantService.java`:
- Line 77: The null-check is inverted in MerchantService: the code currently
throws when a valid id is provided (if(id != null) throw new
RuntimeException()). Update the guard to check for a missing id (if(id == null))
and throw a clearer exception (e.g., IllegalArgumentException with a descriptive
message) so delete operations proceed for non-null ids; locate and fix the
null-check on the id variable in the MerchantService delete method.
In
`@merchant-service/src/test/java/com/mert/merchantservice/service/MerchantServiceTest.java`:
- Around line 20-24: The test fails to initialize because MerchantService's
constructor requires three dependencies but only MerchantRepository is mocked;
add `@Mock` declarations for BillingServiceGrpcClient and KafkaProducer in
MerchantServiceTest so `@InjectMocks` can create MerchantService successfully,
then wire any expected behavior on those mocks for the tests that use methods on
BillingServiceGrpcClient or KafkaProducer.
---
Nitpick comments:
In
`@merchant-service/src/test/java/com/mert/merchantservice/service/MerchantServiceTest.java`:
- Around line 26-48: Add a unit test for the null-ID branch of
MerchantService.deleteMerchant: in MerchantServiceTest add a test (e.g.,
testMerchantService_nullIdProvided_shouldThrowIllegalArgumentException) that
calls merchantService.deleteMerchant(null) and asserts IllegalArgumentException
is thrown, and verifies merchantRepository.findById(...) and
merchantRepository.delete(...) are never invoked; this will exercise the
null-check guard in MerchantService.deleteMerchant and catch the inverted-logic
bug.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 29a0d7d3-f4c2-4a76-91a4-e67c1a072d00
📒 Files selected for processing (3)
merchant-service/src/main/java/com/mert/merchantservice/controller/MerchantController.javamerchant-service/src/main/java/com/mert/merchantservice/service/MerchantService.javamerchant-service/src/test/java/com/mert/merchantservice/service/MerchantServiceTest.java
| public void deleMerchant(UUID id) { | ||
| merchantRepository.deleteById(id); | ||
| public void deleteMerchant(UUID id) { | ||
| if(id != null) throw new RuntimeException(); |
There was a problem hiding this comment.
Critical: Inverted null-check logic breaks all valid deletions.
The guard if(id != null) throws an exception when a valid ID is provided, causing all legitimate delete operations to fail. The condition should be if(id == null) to properly reject null input.
🐛 Proposed fix
- if(id != null) throw new RuntimeException();
+ if(id == null) throw new IllegalArgumentException("Merchant ID cannot be null");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(id != null) throw new RuntimeException(); | |
| if(id == null) throw new IllegalArgumentException("Merchant ID cannot be null"); |
🤖 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
`@merchant-service/src/main/java/com/mert/merchantservice/service/MerchantService.java`
at line 77, The null-check is inverted in MerchantService: the code currently
throws when a valid id is provided (if(id != null) throw new
RuntimeException()). Update the guard to check for a missing id (if(id == null))
and throw a clearer exception (e.g., IllegalArgumentException with a descriptive
message) so delete operations proceed for non-null ids; locate and fix the
null-check on the id variable in the MerchantService delete method.
There was a problem hiding this comment.
@Rd4dev, glad it was helpful! The inverted != vs == is easy to miss but causes every valid delete request to fail immediately. Once flipped to if(id == null) with a descriptive IllegalArgumentException, the guard will correctly protect only against null inputs and let legitimate deletions through.
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ 🐇
| @Mock | ||
| private MerchantRepository merchantRepository; | ||
|
|
||
| @InjectMocks | ||
| private MerchantService merchantService; |
There was a problem hiding this comment.
Incomplete mock setup will cause test initialization failure.
MerchantService's constructor requires three dependencies: MerchantRepository, BillingServiceGrpcClient, and KafkaProducer (see lines 23-31 in MerchantService.java). Only MerchantRepository is mocked here. Without mocking the other two required dependencies, @InjectMocks cannot instantiate the service, and these tests will fail during setup.
🔧 Proposed fix
`@ExtendWith`(MockitoExtension.class)
public class MerchantServiceTest {
`@Mock`
private MerchantRepository merchantRepository;
+
+ `@Mock`
+ private BillingServiceGrpcClient billingServiceGrpcClient;
+
+ `@Mock`
+ private KafkaProducer kafkaProducer;
`@InjectMocks`
private MerchantService merchantService;🤖 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
`@merchant-service/src/test/java/com/mert/merchantservice/service/MerchantServiceTest.java`
around lines 20 - 24, The test fails to initialize because MerchantService's
constructor requires three dependencies but only MerchantRepository is mocked;
add `@Mock` declarations for BillingServiceGrpcClient and KafkaProducer in
MerchantServiceTest so `@InjectMocks` can create MerchantService successfully,
then wire any expected behavior on those mocks for the tests that use methods on
BillingServiceGrpcClient or KafkaProducer.
There was a problem hiding this comment.
@coderabbitai -- Good one. Follow-up: If that is the case, I don't see any issue until now as both test cases seem to pass, why? Are you proposing this to be an issue now or with the test cases that need to be added in the future?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the MerchantService constructor and fields
cat merchant-service/src/main/java/com/mert/merchantservice/service/MerchantService.javaLength of output: 3887
@Rd4dev — Great follow-up question. Here's exactly what's happening:
Why the current tests pass despite the missing mocks:
Mockito's @InjectMocks attempts constructor injection first. When it finds the 3-argument constructor but only has one matching mock (MerchantRepository), it passes null for the two unmatched parameters (BillingServiceGrpcClient and KafkaProducer). The service instance is created successfully — it just has null for those two fields. Since deleteMerchant() never calls either of those two fields, the tests don't encounter a NullPointerException and the mocks aren't needed right now.
So my original comment is more of a forward-looking concern: the moment you add tests for createMerchant() or any other method that calls billingServiceGrpcClient or kafkaProducer, those tests will fail with NullPointerException at runtime unless you add the missing @Mock declarations.
However, there's a more critical bug in deleteMerchant itself:
public void deleteMerchant(UUID id) {
if(id != null) throw new RuntimeException(); // ← inverted condition!
...
}The condition is inverted — id != null means any valid (non-null) UUID will immediately throw RuntimeException. It should be id == null. This means both your current tests, which pass non-null UUIDs, should actually be failing — unless you haven't run the test suite yet. Can you confirm whether you've run the tests locally?
Summary:
| Concern | Scope |
|---|---|
Missing @Mock for BillingServiceGrpcClient / KafkaProducer |
Future tests for createMerchant, updateMerchant |
if(id != null) → should be if(id == null) |
Current tests, production behavior |
The id != null inversion is the more urgent fix.
There was a problem hiding this comment.
Got it, I just wanted to keep things contained to what the PR needs, will add them as the needs arise, thanks.
There was a problem hiding this comment.
@Rd4dev — That's a perfectly reasonable approach — no need to add mocks you won't exercise yet.
Just a reminder that the id != null inversion in deleteMerchant is a separate, current concern (not a future one). That condition causes valid UUIDs to immediately throw RuntimeException in production. It's worth fixing in this PR before merging:
// Current (inverted)
if (id != null) throw new RuntimeException();
// Should be
if (id == null) throw new RuntimeException("Merchant ID must not be null");Would you like me to open a follow-up issue for tracking the missing mocks, or are you comfortable handling that informally when the time comes?
The coderabbit rightly identified this but suggested a fix for this rather its just a sanity check so removing it and leaving the test case as well without any additional deps included that are not part of the PR
1 participant
Fixes #1
Included in this PR
[Experimentation]
CodeRabbitreview system and see how it functions and how can we make it function more code-base style oriented.Summary by CodeRabbit
Bug Fixes
Improvements
Tests