Skip to content

Fixes #1 - Merchant validation and Error handling #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Rd4dev wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fixes #1 - Merchant validation and Error handling #2

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5475b9a
Bad request handling of delete merchant
Rd4dev Jun 5, 2026
40e9491
Phone number regex validation + error handling
Rd4dev Jun 5, 2026
2fff38a
Trial coderabbit review 2
Rd4dev Jun 9, 2026
d2903c0
Added tests for the service layer
Rd4dev Jun 9, 2026
cea8641
Spot right by CR
Rd4dev Jun 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion merchant-service/src/main/java/com/mert/merchantservice/controller/MerchantController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public ResponseEntity<MerchantResponseDTO> updateMerchant(@PathVariable UUID id,
@DeleteMapping("/{id}")
@Operation(summary = "Delete a Merchant")
public ResponseEntity<Void> deleteMerchant(@PathVariable UUID id) {
merchantService.deleMerchant(id);
merchantService.deleteMerchant(id);
return ResponseEntity.noContent().build();
}
}
14 changes: 13 additions & 1 deletion merchant-service/src/main/java/com/mert/merchantservice/dto/MerchantRequestDTO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.mert.merchantservice.dto.validators.CreateMerchantValidationGroup;
import jakarta.validation.constraints.Email;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.Pattern;
import jakarta.validation.constraints.Size;

public class MerchantRequestDTO {
Expand All @@ -17,13 +18,24 @@ public class MerchantRequestDTO {
@Email(message = "Email should be valid")
private String email;

@NotBlank(groups = CreateMerchantValidationGroup.class, message = "Registered date is required")
@NotBlank(
groups = CreateMerchantValidationGroup.class,
message = "Registered date is required"
)
@Pattern(
regexp = "^\\d{4}-\\d{2}-\\d{2}$",
message = "Registered date must be in yyyy-MM-dd format"
)
private String registeredDate;

@NotBlank(message = "Address is required")
private String address;

@NotBlank(message = "Phone Number is required")
@Pattern(
regexp = "^\\+?[0-9]{10,15}$",
message = "Phone number must contain 10 to 15 digits"
)
private String phoneNumber;

public String getMerchantName() {
Expand Down
18 changes: 18 additions & 0 deletions ...hant-service/src/main/java/com/mert/merchantservice/exception/GlobalExceptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
import org.springframework.web.bind.MethodArgumentNotValidException;
import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException;

import java.time.format.DateTimeParseException;
import java.util.HashMap;
import java.util.Map;

Expand All @@ -15,6 +17,14 @@ public class GlobalExceptionHandler {

private static final Logger log = LoggerFactory.getLogger(GlobalExceptionHandler.class);

@ExceptionHandler(MethodArgumentTypeMismatchException.class)
public ResponseEntity<Map<String, String>> handleTypeMismatch(MethodArgumentTypeMismatchException ex) {
Map<String, String> response = new HashMap<>();
response.put("message", "Invalid merchant ID format. Expected UUID");

return ResponseEntity.badRequest().body(response);
}

@ExceptionHandler(MethodArgumentNotValidException.class)
public ResponseEntity<Map<String, String>> handleValidationException(MethodArgumentNotValidException exception) {
Map<String, String> errors = new HashMap<>();
Expand All @@ -24,6 +34,14 @@ public ResponseEntity<Map<String, String>> handleValidationException(MethodArgum
return ResponseEntity.badRequest().body(errors);
}

@ExceptionHandler(DateTimeParseException.class)
public ResponseEntity<Map<String, String>> handleDateTimeParseException(DateTimeParseException ex) {
Map<String, String> error = new HashMap<>();
error.put("registeredDate", "Invalid date. Use a valid date in YYYY-MM-DD format");

return ResponseEntity.badRequest().body(error);
}

@ExceptionHandler(EmailAlreadyExistsException.class)
public ResponseEntity<Map<String, String>> handleEmailAlreadyExistsException(EmailAlreadyExistsException exception) {
log.warn("Email address already exists {}", exception.getMessage());
Expand Down
10 changes: 8 additions & 2 deletions merchant-service/src/main/java/com/mert/merchantservice/service/MerchantService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,13 @@ public MerchantResponseDTO updateMerchant(UUID id, MerchantRequestDTO merchantRe
return MerchantMapper.toMerchantResponseDTO(updatedMerchant);
}

public void deleMerchant(UUID id) {
merchantRepository.deleteById(id);
public void deleteMerchant(UUID id) {
if(id != null) throw new RuntimeException();

@coderabbitai coderabbitai Bot Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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.

Suggested change
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.

@Rd4dev Rd4dev Jun 9, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@coderabbitai coderabbitai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ 🐇

Merchant merchant = merchantRepository.findById(id)
.orElseThrow(() ->
new MerchantNotFoundException(
"Merchant not found with ID: " + id
));
merchantRepository.delete(merchant);
}
}
49 changes: 49 additions & 0 deletions merchant-service/src/test/java/com/mert/merchantservice/service/MerchantServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package com.mert.merchantservice.service;

import com.mert.merchantservice.exception.MerchantNotFoundException;
import com.mert.merchantservice.model.Merchant;
import com.mert.merchantservice.repository.MerchantRepository;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

import java.util.Optional;
import java.util.UUID;

import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.*;

@ExtendWith(MockitoExtension.class)
public class MerchantServiceTest {
@Mock
private MerchantRepository merchantRepository;

@InjectMocks
private MerchantService merchantService;
Comment on lines +20 to +24

@coderabbitai coderabbitai Bot Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

@Rd4dev Rd4dev Jun 9, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@coderabbitai coderabbitai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the MerchantService constructor and fields
cat merchant-service/src/main/java/com/mert/merchantservice/service/MerchantService.java

Length 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.

@Rd4dev Rd4dev Jun 9, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I just wanted to keep things contained to what the PR needs, will add them as the needs arise, thanks.

@coderabbitai coderabbitai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?


@Test
void testMerchantService_validIdProvided_shouldDeleteMerchant() {
UUID id = UUID.randomUUID();

Merchant merchant = new Merchant();
merchant.setId(id);

when(merchantRepository.findById(id)).thenReturn(Optional.of(merchant));
merchantService.deleteMerchant(id);
verify(merchantRepository).delete(merchant);
}

@Test
void testMerchantService_invalidIdProvided_shouldThrowMerchantNotFoundException() {
UUID id = UUID.randomUUID();

when(merchantRepository.findById(id)).thenReturn(Optional.empty());
assertThrows(
MerchantNotFoundException.class,
() -> merchantService.deleteMerchant(id)
);
verify(merchantRepository, never()).delete(any());
}
}