server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (1)

Line range hint 64-92: Create subscription partial rollback
While the code handles exceptions, consider rolling back the newly created subscription if provisioning permanently fails.

entities/src/main/java/com/flipkart/varadhi/entities/VaradhiSubscription.java (2)

167-172: Restoration logic.
Restoring the subscription to CREATED is clear, but consider whether a new “RESTORED” state might improve traceability.

---

174-190: Consider supplying a default or optional for missing property values.
Throwing an exception is valid, though returning an optional or fallback could be more user-friendly in certain scenarios.

common/src/main/java/com/flipkart/varadhi/Constants.java (5)

23-23: Consider making DefaultTopicCapacity immutable.

The DefaultTopicCapacity constant could potentially be modified since TopicCapacityPolicy might be mutable.

Consider making TopicCapacityPolicy immutable or creating an unmodifiable instance:

- public static final TopicCapacityPolicy DefaultTopicCapacity = new TopicCapacityPolicy(100, 400, 2);
+ public static final TopicCapacityPolicy DefaultTopicCapacity = Collections.unmodifiableObject(new TopicCapacityPolicy(100, 400, 2));

---

53-54: Enhance documentation for query parameters.

While the parameters are self-explanatory, adding documentation about valid values and usage would be helpful, especially for deletionType which is crucial for the new deletion feature.

Add parameter documentation:

+    /** Specifies the type of deletion operation (e.g., "soft", "hard") */
     public static final String QUERY_PARAM_DELETION_TYPE = "deletionType";
+    /** When true, allows deletion even if there are dependent resources */
     public static final String QUERY_PARAM_IGNORE_CONSTRAINTS = "ignoreConstraints";

---

83-93: Consider using standard HTTP status code constants.

Instead of defining custom HTTP status code constants, consider using HttpStatus from Spring Framework or similar standard libraries if available in the project.

- public static final int HTTP_RATE_LIMITED = 429;
- public static final int HTTP_UNPROCESSABLE_ENTITY = 422;
+ // Use org.springframework.http.HttpStatus if Spring is available
+ public static final int HTTP_RATE_LIMITED = HttpStatus.TOO_MANY_REQUESTS.value();
+ public static final int HTTP_UNPROCESSABLE_ENTITY = HttpStatus.UNPROCESSABLE_ENTITY.value();

---

96-118: Consider expanding metrics structure for deletion feature.

Since this PR implements deletion functionality, consider adding corresponding metrics.

Example structure:

public static class Deletion {
    public static final String SOFT_DELETION_METER = "deletion.soft";
    public static final String HARD_DELETION_METER = "deletion.hard";
    public static final String RESTORATION_METER = "deletion.restore";
    
    private Deletion() {
        // Private constructor to prevent instantiation
    }
}

---

Line range hint 126-129: Document rationale for size limits.

The maximum sizes for payload and headers would benefit from documentation explaining their rationale.

Add explanatory comments:

+ // 5MB max payload size to balance between large messages and server resource usage
 public static final int PAYLOAD_SIZE_MAX = 5 * 1024 * 1024;
+ // Limited number of headers to prevent header explosion attacks
 public static final int HEADERS_ALLOWED_MAX = 10;
+ // Header size limits aligned with common proxy server defaults
 public static final int HEADER_NAME_SIZE_MAX = 64;
 public static final int HEADER_VALUE_SIZE_MAX = 256;

server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java (4)

45-48: Consider moving test constants to a separate class.

These constants could be reused across other test classes. Consider moving them to a dedicated test constants class for better maintainability and reusability.

---

67-69: Consider using @ExtendWith(MockitoExtension.class).

Instead of manually calling MockitoAnnotations.openMocks(this), you can use the @ExtendWith(MockitoExtension.class) annotation at the class level for a more idiomatic JUnit 5 approach.

+@ExtendWith(MockitoExtension.class)
 class VaradhiTopicServiceTest {
     // ...
     @BeforeEach
     public void setUp() {
-        MockitoAnnotations.openMocks(this);
         varadhiTopicFactory = spy(new VaradhiTopicFactory(storageTopicFactory, REGION, DEFAULT_CAPACITY_POLICY));
         // ...
     }

---

162-179: Enhance topic-in-use test with specific error message assertion.

The test verifies the exception type but doesn't validate the error message. Add an assertion to verify the specific error message for better test coverage.

 Exception exception = assertThrows(
         InvalidOperationForResourceException.class,
         () -> varadhiTopicService.delete(varadhiTopic.getName(), ResourceDeletionType.HARD_DELETE)
 );

 verify(metaStore, never()).deleteTopic(varadhiTopic.getName());
 assertEquals(InvalidOperationForResourceException.class, exception.getClass());
+assertEquals("Cannot delete topic " + varadhiTopic.getName() + " as it has active subscriptions", exception.getMessage());

---

323-330: Add input validation to setupMockTopics.

The method assumes that topicNames and topicStatuses lists have the same size. Add validation to prevent potential IndexOutOfBoundsException.

 private void setupMockTopics(String projectName, List<String> topicNames, List<Boolean> topicStatuses) {
+    if (topicNames.size() != topicStatuses.size()) {
+        throw new IllegalArgumentException("Topic names and statuses lists must have the same size");
+    }
     when(metaStore.getTopicNames(projectName)).thenReturn(topicNames);
     for (int i = 0; i < topicNames.size(); i++) {
         // ...
     }
 }

server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java (6)

15-17: Expanded class-level documentation
The Javadoc succinctly describes the service’s purpose. Consider mentioning soft/hard deletion capabilities for completeness.

---

52-67: Active status check
Throwing ResourceNotFoundException for inactive topics is sensible. Logging the inactive status could improve debugging.

---

69-70: Method documentation
Comments are clear about topic deletion. Optionally referencing the new ResourceDeletionType param would further clarify usage.

---

88-96: Soft delete concurrency
Marking a topic inactive is straightforward. If multiple soft-delete requests arrive concurrently, consider whether extra idempotency checks are needed.

---

117-134: Restore method
Good flow: checks if active, throws if not deleted, updates status to ACTIVE, and saves. Optionally log a final success message.

---

136-154: Validation performance
Scanning all subscription names might affect performance at scale. Consider more direct checks or indexing in future iterations.

server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (5)

116-146: Update subscription logic
Reverting grouped mid-operation can cause confusion. Consider a clearer approach to reduce potential state flip-flops.

---

157-186: Delete subscription
Hard vs. soft deletion is clearly distinguished. Validate concurrency if a subscription is in a transitional state (e.g., provisioning).

---

235-251: getValidatedSubscription duplication
Ensures subscription validity. Consider unifying all state checks in one method to minimize code duplication.

---

283-302: performSubscriptionOperation
A neat approach to reduce duplication. Watch for concurrency issues if subscription states change rapidly.

---

327-335: handleSoftDelete
Straightforward approach. You might add an additional check for already-inactive subscriptions.

server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (3)

144-176: Hierarchy construction
Generating subscription and topic hierarchies is straightforward. Consider caching repeated calls to projectService.getProject if performance becomes a concern.

---

181-185: List endpoint
Fetching all subscription names might be expensive for large projects. Consider pagination or search filters if needed.

---

204-218: Create endpoint
Uses varadhiSubscriptionFactory and checks the subscribed topic. Adding more explicit error logs on creation failure might help troubleshooting.

entities/src/main/java/com/flipkart/varadhi/entities/ResourceDeletionType.java (2)

6-9: Add documentation for enum values.

Consider adding Javadoc for each enum value to clearly document their intended use and behavior.

 public enum ResourceDeletionType {
+    /** Marks the resource as inactive without removing it from the database */
     SOFT_DELETE,
+    /** Permanently removes the resource from the database */
     HARD_DELETE,
+    /** Default deletion type, typically maps to SOFT_DELETE for safety */
     DEFAULT;

---

18-27: Enhance string parsing robustness.

The string parsing could be more robust by:

1. Using a constant for the default value
2. Adding case-insensitive matching

+    /** Default deletion type used when input is invalid */
+    private static final ResourceDeletionType DEFAULT_TYPE = DEFAULT;
+
     public static ResourceDeletionType fromValue(String value) {
         if (value == null || value.isBlank()) {
-            return DEFAULT;
+            return DEFAULT_TYPE;
         }
-        return switch (value.trim().toUpperCase()) {
-            case "SOFT_DELETE" -> SOFT_DELETE;
-            case "HARD_DELETE" -> HARD_DELETE;
-            default -> DEFAULT;
+        try {
+            return valueOf(value.trim().toUpperCase());
+        } catch (IllegalArgumentException e) {
+            return DEFAULT_TYPE;
+        }
     }

spi/src/main/java/com/flipkart/varadhi/spi/db/MetaStore.java (1)

52-52: Add method documentation.

Add Javadoc for the new updateTopic method to document its purpose, parameters, and any exceptions it might throw.

+    /**
+     * Updates an existing topic in the meta store.
+     *
+     * @param topic the topic to update
+     * @throws IllegalArgumentException if the topic is null
+     * @throws ResourceNotFoundException if the topic doesn't exist
+     */
     void updateTopic(VaradhiTopic topic);

entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (2)

20-20: Consider making status field final.

The status field should be made final to ensure immutability, with status changes handled through creating new instances.

-    private Status status;
+    private final Status status;

---

105-110: Add status transition validation.

Consider adding validation for status transitions to prevent invalid state changes (e.g., INACTIVE to INACTIVE).

     public void updateStatus(Status newStatus) {
         if (newStatus == null) {
             throw new IllegalArgumentException("Status cannot be null");
         }
+        if (this.status == newStatus) {
+            throw new IllegalArgumentException("Topic is already in " + newStatus + " state");
+        }
         this.status = newStatus;
     }

entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java (2)

109-118: Enhance status transition test coverage.

Consider adding parameterized tests for status transitions and edge cases.

+    @ParameterizedTest
+    @CsvSource({
+        "ACTIVE,INACTIVE",
+        "INACTIVE,ACTIVE"
+    })
+    void updateStatus_ValidTransitions_Succeeds(Status from, Status to) {
+        VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false);
+        varadhiTopic.updateStatus(from);
+        
+        varadhiTopic.updateStatus(to);
+        
+        assertEquals(to, varadhiTopic.getStatus());
+    }
+
+    @Test
+    void updateStatus_SameState_ThrowsException() {
+        VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false);
+        
+        assertThrows(IllegalArgumentException.class,
+            () -> varadhiTopic.updateStatus(Status.ACTIVE),
+            "Updating to same state should throw exception");
+    }

---

27-29: Consider adding validation in helper method.

The helper method should validate input parameters.

     private VaradhiTopic createDefaultVaradhiTopic(boolean grouped) {
+        Objects.requireNonNull(PROJECT_NAME, "Project name cannot be null");
+        Objects.requireNonNull(TOPIC_NAME, "Topic name cannot be null");
+        Objects.requireNonNull(TOPIC_CAPACITY, "Topic capacity cannot be null");
         return VaradhiTopic.of(PROJECT_NAME, TOPIC_NAME, grouped, TOPIC_CAPACITY);
     }

server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (1)

166-171: Enhance test coverage for deletion types.

The test verifies the basic deletion functionality but doesn't test different deletion types. Consider adding test cases for both soft and hard deletion scenarios.

Apply this diff to add test coverage for different deletion types:

-    @Test
-    void testSubscriptionDelete() throws InterruptedException {
-        HttpRequest<Buffer> request = createRequest(HttpMethod.DELETE, getSubscriptionUrl("sub1", project));
-        SubscriptionResource resource = getSubscriptionResource("sub1", project, topicResource);
-
-        doReturn(project).when(projectService).getCachedProject(project.getName());
-        ArgumentCaptor<String> captorSubName = ArgumentCaptor.forClass(String.class);
-        ArgumentCaptor<Project> captorProject = ArgumentCaptor.forClass(Project.class);
-        doReturn(CompletableFuture.completedFuture(null)).when(subscriptionService)
-                .deleteSubscription(captorSubName.capture(), captorProject.capture(), any(), any());
-
-        sendRequestWithoutBody(request, null);
-        assertEquals(captorSubName.getValue(), resource.getSubscriptionInternalName());
-        assertEquals(captorProject.getValue().getName(), project.getName());
-        verify(subscriptionService, times(1)).deleteSubscription(any(), any(), any(), any());
-    }
+    @Test
+    void testSubscriptionSoftDelete() throws InterruptedException {
+        verifyDeleteRequest("SOFT_DELETE", ResourceDeletionType.SOFT_DELETE);
+    }
+
+    @Test
+    void testSubscriptionHardDelete() throws InterruptedException {
+        verifyDeleteRequest("HARD_DELETE", ResourceDeletionType.HARD_DELETE);
+    }
+
+    @Test
+    void testSubscriptionDeleteDefaultType() throws InterruptedException {
+        verifyDeleteRequest(null, ResourceDeletionType.SOFT_DELETE);
+    }
+
+    private void verifyDeleteRequest(String deletionType, ResourceDeletionType expectedDeletionType)
+            throws InterruptedException {
+        String url = getSubscriptionUrl("sub1", project);
+        if (deletionType != null) {
+            url += "?deletionType=" + deletionType;
+        }
+
+        HttpRequest<Buffer> request = createRequest(HttpMethod.DELETE, url);
+        SubscriptionResource resource = getSubscriptionResource("sub1", project, topicResource);
+
+        doReturn(project).when(projectService).getCachedProject(project.getName());
+        ArgumentCaptor<String> captorSubName = ArgumentCaptor.forClass(String.class);
+        ArgumentCaptor<Project> captorProject = ArgumentCaptor.forClass(Project.class);
+        ArgumentCaptor<ResourceDeletionType> captorDeletionType = ArgumentCaptor.forClass(ResourceDeletionType.class);
+
+        doReturn(CompletableFuture.completedFuture(null)).when(subscriptionService)
+                .deleteSubscription(captorSubName.capture(), captorProject.capture(), any(), captorDeletionType.capture());
+
+        sendRequestWithoutBody(request, null);
+
+        assertEquals(captorSubName.getValue(), resource.getSubscriptionInternalName());
+        assertEquals(captorProject.getValue().getName(), project.getName());
+        assertEquals(expectedDeletionType, captorDeletionType.getValue());
+        verify(subscriptionService, times(1)).deleteSubscription(any(), any(), any(), any());
+    }

server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java (1)

217-229: Improve error message clarity.

The error message could be more descriptive to help users understand the issue better.

Apply this diff to enhance the error message:

-            throw new IllegalArgumentException("Project name in URL and request body do not match.");
+            throw new IllegalArgumentException(
+                String.format("Project name mismatch: '%s' in URL does not match '%s' in request body.",
+                    projectName, topicResource.getProject())
+            );

server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (2)

169-183: Add test coverage for error scenarios.

Consider adding tests for:

1. Non-existent topic
2. Invalid topic name format
3. Topic in deleted state

Would you like me to provide the implementation for these test cases?

---

231-238: Add test coverage for restore error scenarios.

Consider adding tests for:

1. Restoring a non-existent topic
2. Restoring an active topic
3. Restoring a hard-deleted topic

Would you like me to provide the implementation for these test cases?

server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (1)

Line range hint 349-365: Enhance test helpers and mock setup for deletion scenarios.

To improve test maintainability and reduce code duplication for the new deletion scenarios, consider:

1. Adding a helper method for deletion operations:

private CompletableFuture<Void> deleteSubscription(String name, ResourceDeletionType deletionType) {
    return subscriptionService.deleteSubscription(
        name, o1t1p1, requestedBy, deletionType);
}

2. Enhancing mock setup to verify deletion type handling:

@Test
void verifyDeletionTypeHandling() {
    // Setup
    doReturn(unGroupedTopic).when(varadhiMetaStore).getTopic(unGroupedTopic.getName());
    subscriptionService.createSubscription(unGroupedTopic, sub1, o1t1p1);
    
    // Verify that deletion type is correctly passed to the meta store
    deleteSubscription(sub1.getName(), ResourceDeletionType.SOFT_DELETE);
    verify(varadhiMetaStore).markSubscriptionAsDeleted(
        eq(sub1.getName()), eq(ResourceDeletionType.SOFT_DELETE));
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java (1)

125-125: Review test cleanup strategy.

The test cleanup operations consistently use hard delete. Consider documenting this decision in a test utility class or as a comment to explain why hard delete is preferred for test cleanup.

entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (3)

20-41: Add null check in constructor for status parameter.

While the updateStatus method includes null validation, the constructor should also validate the status parameter to maintain consistency and prevent null states.

 private VaradhiTopic(
         String name, int version, boolean grouped, TopicCapacityPolicy capacity,
         Map<String, InternalCompositeTopic> internalTopics, Status status
 ) {
     super(name, version);
+    if (status == null) {
+        throw new IllegalArgumentException("Status cannot be null");
+    }
     this.grouped = grouped;
     this.capacity = capacity;
     this.internalTopics = internalTopics;
     this.status = status;
 }

---

43-54: Enhance factory method documentation.

The documentation should mention that the topic is created with a default Status.ACTIVE state.

 /**
  * Creates a new VaradhiTopic instance.
  *
  * @param project the project associated with the topic
  * @param name the name of the topic
  * @param grouped whether the topic is grouped
  * @param capacity the capacity policy of the topic
+ * @return a new VaradhiTopic instance with Status.ACTIVE state
  */

---

122-128: Consider enhancing Status enum for better lifecycle management.

Given that this implements soft-deletion support, consider:

1. Adding a PENDING_DELETION state to support graceful deletion process
2. Implementing state transition validation to prevent invalid state changes

Example enhancement:

public enum Status {
    ACTIVE,
    PENDING_DELETION,  // Intermediate state during deletion
    INACTIVE;         // Soft-deleted state

    public boolean canTransitionTo(Status newStatus) {
        return switch (this) {
            case ACTIVE -> newStatus == PENDING_DELETION;
            case PENDING_DELETION -> newStatus == INACTIVE;
            case INACTIVE -> newStatus == ACTIVE;  // Support restoration
        };
    }
}

Then update the updateStatus method to use this validation:

public void updateStatus(Status newStatus) {
    if (newStatus == null) {
        throw new IllegalArgumentException("Status cannot be null");
    }
    if (!this.status.canTransitionTo(newStatus)) {
        throw new IllegalStateException(
            "Invalid state transition from " + this.status + " to " + newStatus);
    }
    this.status = newStatus;
}

server/src/testE2E/java/com/flipkart/varadhi/E2EBase.java (2)

137-137: Extract "HARD_DELETE" as a constant.

The string literal "HARD_DELETE" is used in multiple places. Consider extracting it as a class constant to improve maintainability and prevent typos.

+ private static final String HARD_DELETE = "HARD_DELETE";
- makeDeleteRequest(getTopicsUri(project, topicName), "HARD_DELETE", 200);
+ makeDeleteRequest(getTopicsUri(project, topicName), HARD_DELETE, 200);

---

166-167: Improve readability of subscription name parsing.

1. This is another occurrence of the "HARD_DELETE" string literal that should use the suggested constant.
2. The subscription name parsing logic using split makes the code less readable.

Consider extracting the subscription name parsing:

+ private static String parseSubscriptionName(String fullName) {
+     return fullName.split(NAME_SEPARATOR_REGEX)[1];
+ }

  static void cleanupSubscriptionsOnProject(Project project) {
      getSubscriptions(makeListRequest(getSubscriptionsUri(project), 200)).forEach(
-             s -> makeDeleteRequest(
-                     getSubscriptionsUri(project, s.split(NAME_SEPARATOR_REGEX)[1]), "HARD_DELETE", 200));
+             s -> makeDeleteRequest(
+                     getSubscriptionsUri(project, parseSubscriptionName(s)), HARD_DELETE, 200));
  }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java (1)

Line range hint 1-209: Consider adding tests for soft-deletion scenarios.

While the method renaming changes are consistent and maintain existing test coverage, consider adding the following test cases to support the PR's soft-deletion feature:

1. Produce attempt to a soft-deleted topic
2. Produce attempt during topic restoration
3. Produce after successful topic restoration

This will ensure robust validation of the new soft-deletion functionality.

server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java (2)

64-67: Consider consolidating error test cases.

Both test cases verify the same error condition (413 "Entity too large") with different oversized payloads (21 and 30 bytes). Consider using a parameterized test to improve test maintainability.

Example refactor:

-        payload = "012345678901234567890".getBytes();
-        sendRequestWithPayload(
-                request, payload, 413, "Entity too large.",
-                ErrorResponse.class
-        );
-
-        payload = "012345678901234567890123456789".getBytes();
-        sendRequestWithPayload(
-                request, payload, 413, "Entity too large.",
-                ErrorResponse.class
-        );
+        String[][] oversizedPayloads = {
+            {"012345678901234567890", "21 bytes"},
+            {"012345678901234567890123456789", "30 bytes"}
+        };
+        
+        for (String[] testCase : oversizedPayloads) {
+            payload = testCase[0].getBytes();
+            sendRequestWithPayload(
+                    request, payload, 413, "Entity too large.",
+                    ErrorResponse.class
+            );
+        }

Also applies to: 70-73

---

Line range hint 51-74: Enhance test method documentation.

While the test coverage is comprehensive, consider adding clear documentation to describe the test scenarios:

1. Success cases: 10 bytes (well below limit), 19 bytes (just below limit), 20 bytes (at limit)
2. Error cases: 21 bytes (just above limit), 30 bytes (well above limit)

Example improvement:

+    /**
+     * Validates the body handler's size limit enforcement:
+     * - Success cases: Tests payloads of 10, 19, and 20 bytes (limit)
+     * - Error cases: Tests oversized payloads of 21 and 30 bytes
+     * Expected: 413 "Entity too large" for payloads exceeding 20 bytes
+     */
     @Test
-    public void testProduceWithForBodySize() throws InterruptedException {
+    public void testProduceHandlerEnforcesSizeLimit() throws InterruptedException {

server/src/test/java/com/flipkart/varadhi/web/produce/HeaderValidationTest.java (2)

114-115: LGTM! Consider additional edge cases.

The method name update maintains the test's validation of multi-value header counting. Consider adding test cases for:

• Empty multi-value headers
• Null values in multi-value headers
• Maximum size validation for combined multi-value header lengths

---

Line range hint 1-117: Consider additional test coverage for soft-deletion scenarios.

Given that this PR implements soft-deletion support, consider adding test cases to verify header validation behavior for:

• Requests to soft-deleted topics
• Restoration requests
• Hard deletion requests
  This would ensure header validation remains consistent across all resource states.

server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (1)

399-417: Enhance error messages in property validation.

While the validation logic is thorough, consider improving error messages to include:

• The invalid value that was provided
• The expected format or range
• Any additional context that would help users fix the issue

 if (!validator.isValid(value, usePermissible)) {
-    throw new IllegalArgumentException("Invalid value for property: " + key);
+    throw new IllegalArgumentException(String.format(
+        "Invalid value '%s' for property '%s'. %s",
+        value, key, validator.getValidationDescription()
+    ));
 }

server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (4)

46-49: Consider using more descriptive constant names.

The constant names could be more descriptive to better indicate their test context. For example, TEST_TOPIC_NAME, TEST_TEAM_NAME, etc.

-    private static final String TOPIC_NAME = "topic1";
-    private static final String TEAM_NAME = "team1";
-    private static final String ORG_NAME = "org1";
-    private static final String DEFAULT_PROJECT_NAME = "project1";
+    private static final String TEST_TOPIC_NAME = "topic1";
+    private static final String TEST_TEAM_NAME = "team1";
+    private static final String TEST_ORG_NAME = "org1";
+    private static final String TEST_PROJECT_NAME = "project1";

---

84-111: Add documentation for route handlers.

Consider adding Javadoc comments to document the purpose and behavior of each route handler. This would make it easier for other developers to understand the test setup.

+    /**
+     * Sets up all route handlers for topic operations including:
+     * - Create topic (POST /projects/:project/topics)
+     * - Get topic (GET /projects/:project/topics/:topic)
+     * - List topics (GET /projects/:project/topics)
+     * - Delete topic (DELETE /projects/:project/topics/:topic)
+     * - Restore topic (POST /projects/:project/topics/:topic/restore)
+     */
     private void setupRoutes() {

---

212-229: Add verification for topic status after soft deletion.

The deletion tests verify the service call but don't verify the resulting topic status. Consider adding assertions to verify the topic's state after soft deletion.

     @Test
     void deleteTopic_WithSoftDelete_ShouldDeleteTopicSuccessfully() throws InterruptedException {
+        TopicResource topicResource = getTopicResource(project);
+        VaradhiTopic varadhiTopic = topicResource.toVaradhiTopic();
+        String varadhiTopicName = String.join(".", project.getName(), TOPIC_NAME);
+        
+        doReturn(varadhiTopic).when(varadhiTopicService).get(varadhiTopicName);
         verifyDeleteRequest("SOFT_DELETE", ResourceDeletionType.SOFT_DELETE);
+        
+        // Verify topic status after soft deletion
+        TopicResource retrievedTopic = sendRequestWithoutPayload(
+            createRequest(HttpMethod.GET, getTopicUrl(project)),
+            TopicResource.class
+        );
+        assertEquals("DELETED", retrievedTopic.getStatus());
     }

---

261-271: Add input validation for URL construction helpers.

The URL construction helpers don't validate their inputs. Consider adding null checks and validation for the project parameter.

     private String getTopicsUrl(Project project) {
+        if (project == null || project.getName() == null) {
+            throw new IllegalArgumentException("Project and project name must not be null");
+        }
         return String.join("/", "/projects", project.getName(), "topics");
     }

     private String getTopicUrl(Project project) {
+        if (project == null || project.getName() == null) {
+            throw new IllegalArgumentException("Project and project name must not be null");
+        }
         return String.join("/", getTopicsUrl(project), TOPIC_NAME);
     }

server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java (2)

Line range hint 98-147: Consider adding builder pattern for test data creation.

While the implementation is clean, consider introducing a builder pattern for subscription creation to make tests more maintainable and readable. This would allow for easier customization of test data without needing multiple method overloads.

Example implementation:

public static class TestSubscriptionBuilder {
    private String name = "test-subscription";
    private Project project = PROJECT;
    private VaradhiTopic topic;
    private boolean grouped = false;
    private RetryPolicy retryPolicy = DEFAULT_RETRY_POLICY;
    // ... other fields

    public TestSubscriptionBuilder withName(String name) {
        this.name = name;
        return this;
    }

    // ... other builder methods

    public VaradhiSubscription build() {
        return VaradhiSubscription.of(
            SubscriptionResource.buildInternalName(project.getName(), name),
            project.getName(),
            topic.getName(),
            UUID.randomUUID().toString(),
            grouped,
            DEFAULT_ENDPOINT,
            retryPolicy,
            DEFAULT_CONSUMPTION_POLICY,
            DEFAULT_SHARDS,
            DEFAULT_SUBSCRIPTION_PROPERTIES
        );
    }
}

---

150-174: Consider using seeded Random for deterministic tests.

Using a non-seeded Random instance could lead to non-deterministic test behavior. Consider using a seeded Random instance to ensure test reproducibility.

-    private final Random random = new Random();
+    private final Random random = new Random(42L); // Use a fixed seed for reproducibility

This ensures that the random values generated in createDlqMessage and generateRandomBytes are consistent across test runs, making it easier to debug test failures.

server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java (4)

51-58: Good error handling in JSON deserialization!

The error handling is well-implemented with proper exception wrapping. Consider adding logging before throwing the RuntimeException for better debugging capabilities.

 } catch (Exception e) {
+    log.error("JSON deserialization failed", e);
     throw new RuntimeException("Failed to deserialize JSON", e);
 }

---

101-111: Good error handling for unsupported HTTP methods!

Consider using a switch statement or Map-based approach for better maintainability when handling HTTP methods.

 public HttpRequest<Buffer> createRequest(HttpMethod method, String path) {
-    if (POST == method) {
-        return webClient.post(DEFAULT_PORT, DEFAULT_HOST, path);
-    } else if (GET == method) {
-        return webClient.get(DEFAULT_PORT, DEFAULT_HOST, path);
-    } else if (DELETE == method) {
-        return webClient.delete(DEFAULT_PORT, DEFAULT_HOST, path);
-    } else if (PUT == method) {
-        return webClient.put(DEFAULT_PORT, DEFAULT_HOST, path);
-    } else {
-        throw new UnsupportedOperationException("Unsupported HTTP method");
-    }
+    return switch (method) {
+        case POST -> webClient.post(DEFAULT_PORT, DEFAULT_HOST, path);
+        case GET -> webClient.get(DEFAULT_PORT, DEFAULT_HOST, path);
+        case DELETE -> webClient.delete(DEFAULT_PORT, DEFAULT_HOST, path);
+        case PUT -> webClient.put(DEFAULT_PORT, DEFAULT_HOST, path);
+        default -> throw new UnsupportedOperationException("Unsupported HTTP method: " + method);
+    };
 }

---

168-179: Good error response handling!

Consider adding validation for the error response structure to ensure all required fields are present.

 ErrorResponse error = JsonMapper.jsonDeserialize(response.body().getBytes(), ErrorResponse.class);
+assertNotNull(error, "Error response should not be null");
+assertNotNull(error.reason(), "Error reason should not be null");
 assertEquals(expectedStatusMessage, error.reason());

---

220-223: Good addition of PostResponseCapture for response handling!

Consider making the fields volatile since they're accessed from different threads (main thread and callback thread).

 private static class PostResponseCapture<T> {
-    private T response;
-    private Throwable throwable;
+    private volatile T response;
+    private volatile Throwable throwable;
 }

server/src/test/java/com/flipkart/varadhi/web/admin/DlqHandlersTest.java (1)

Line range hint 298-313: Consider simplifying URL building logic using URLBuilder or UriComponentsBuilder.

The current string concatenation approach for building URLs with query parameters could be simplified using a dedicated URL building utility. This would:

• Reduce manual string manipulation
• Handle URL encoding automatically
• Make the code more maintainable

Example using UriComponentsBuilder:

private String getMessagesUrl(String subscriptionName, Project project, long failedAt, String nextPage, int limit) {
    UriComponentsBuilder builder = UriComponentsBuilder
        .fromPath(buildSubscriptionUrl(subscriptionName, project))
        .path("/dlq/messages");
    
    if (failedAt != 0) {
        builder.queryParam("earliestFailedAt", failedAt);
    }
    if (limit != -1) {
        builder.queryParam("limit", limit);
    }
    if (nextPage != null && !nextPage.isBlank()) {
        builder.queryParam("nextPage", nextPage);
    }
    
    return builder.build().toString();
}

entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java (4)

16-36: Document magic numbers in default test constants.

Consider adding comments to explain the significance of magic numbers in the default constants (e.g., 500, 1, 10). This will help other developers understand the test scenarios better.

---

38-56: Add assertion for initial subscription state.

The successful creation test should also verify that the subscription's initial state is set correctly. Consider adding:

 assertNotNull(subscription.getProperties());
+assertEquals(VaradhiSubscription.State.CREATING, subscription.getStatus().getState());
+assertNull(subscription.getStatus().getMessage());

---

103-116: Add test for empty properties map.

Consider adding a test case for an empty properties map to ensure it's handled correctly:

@Test
void createSubscription_EmptyProperties_ThrowsException() {
    IllegalArgumentException exception = assertThrows(
            IllegalArgumentException.class, () -> {
                VaradhiSubscription.of(
                        "sub1", "project1", "topic1", "description", true,
                        DEFAULT_ENDPOINT, DEFAULT_RETRY_POLICY, DEFAULT_CONSUMPTION_POLICY,
                        DEFAULT_SHARDS, Map.of()
                );
            }
    );

    assertEquals("Properties cannot be null or empty", exception.getMessage());
}

---

197-240: Add edge cases for integer property retrieval.

Consider adding tests for:

1. Integer overflow/underflow
2. Empty string value
3. Whitespace-only string value

Example:

@Test
void getIntProperty_IntegerOverflow_ThrowsException() {
    VaradhiSubscription subscription = VaradhiSubscription.of(
            "sub1", "project1", "topic1", "description", true,
            DEFAULT_ENDPOINT, DEFAULT_RETRY_POLICY, DEFAULT_CONSUMPTION_POLICY,
            DEFAULT_SHARDS, Map.of("key", "999999999999999999999999")
    );

    assertThrows(NumberFormatException.class, () -> subscription.getIntProperty("key"));
}

server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (2)

88-154: Consider refactoring the setup method for better organization.

The setup method mixes infrastructure initialization with test data preparation. Consider splitting it into separate methods:

1. setupInfrastructure() for ZK, MetaStore, and service initialization
2. setupTestData() for creating test entities
3. Consider extracting test data values into constants

 @BeforeEach
 void setUp() throws Exception {
+    setupInfrastructure();
+    setupTestData();
+    setupMocks();
 }
+
+private void setupInfrastructure() throws Exception {
     JsonMapper.getMapper().registerSubtypes(
             new NamedType(PulsarStorageTopic.class, "PulsarTopic"),
             new NamedType(PulsarSubscription.class, "PulsarSubscription")
     );
     zkCuratorTestingServer = new TestingServer();
     zkCurator = spy(CuratorFrameworkFactory.newClient(
             zkCuratorTestingServer.getConnectString(), new ExponentialBackoffRetry(1000, 1)
     ));
     zkCurator.start();
     varadhiMetaStore = spy(new VaradhiMetaStore(zkCurator));
     orgService = new OrgService(varadhiMetaStore);
     teamService = new TeamService(varadhiMetaStore);
     meterRegistry = new JmxMeterRegistry(JmxConfig.DEFAULT, Clock.SYSTEM);
     projectService = new ProjectService(varadhiMetaStore, "", meterRegistry);
 }
+
+private static final String TEST_ORG_NAME = "Org";
+private static final String TEST_TEAM_NAME = "Team";
+private static final String TEST_PROJECT1_NAME = "Project1";
+private static final String TEST_PROJECT2_NAME = "Project2";
+
+private void setupTestData() {
     org = Org.of(TEST_ORG_NAME);
     team = Team.of(TEST_TEAM_NAME, org.getName());
     project1 = Project.of(TEST_PROJECT1_NAME, "", team.getName(), team.getOrg());
     project2 = Project.of(TEST_PROJECT2_NAME, "", team.getName(), team.getOrg());
     unGroupedTopic = VaradhiTopic.of("UngroupedTopic", project1.getName(), false, null);
     groupedTopic = VaradhiTopic.of("GroupedTopic", project2.getName(), true, null);
     subscription1 = createUngroupedSubscription("Sub1", project1, unGroupedTopic);
     subscription2 = createUngroupedSubscription("Sub2", project1, unGroupedTopic);
     orgService.createOrg(org);
     teamService.createTeam(team);
     projectService.createProject(project1);
     projectService.createProject(project2);
 }
+
+private void setupMocks() {
     shardProvisioner = mock(ShardProvisioner.class);
     doNothing().when(shardProvisioner).provision(any(), any());
     controllerRestApi = mock(ControllerRestApi.class);
     subscriptionService = new SubscriptionService(shardProvisioner, controllerRestApi, varadhiMetaStore);
 }

---

545-631: Add test coverage for additional deletion scenarios.

The deletion tests are thorough but consider adding the following test cases:

1. Attempting to delete an already soft-deleted subscription
2. Attempting to delete a subscription with active consumers
3. Attempting to hard delete after a soft delete

Example test case:

@Test
void deleteSubscription_AlreadySoftDeleted_ThrowsException() {
    doReturn(unGroupedTopic).when(varadhiMetaStore).getTopic(unGroupedTopic.getName());
    subscriptionService.createSubscription(unGroupedTopic, subscription1, project1);
    
    // First soft delete
    CompletableFuture<SubscriptionState> stoppedState = 
            CompletableFuture.completedFuture(SubscriptionState.forStopped());
    doReturn(stoppedState).when(controllerRestApi)
            .getSubscriptionState(subscription1.getName(), REQUESTED_BY);
    
    subscriptionService.deleteSubscription(
            subscription1.getName(), project1, REQUESTED_BY, 
            ResourceDeletionType.SOFT_DELETE).join();
    
    // Attempt second delete
    InvalidOperationForResourceException exception = assertThrows(
            InvalidOperationForResourceException.class,
            () -> subscriptionService.deleteSubscription(
                    subscription1.getName(), project1, REQUESTED_BY,
                    ResourceDeletionType.SOFT_DELETE).join()
    );
    
    assertEquals(
            "Cannot delete subscription in INACTIVE state",
            exception.getMessage()
    );
}

server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (5)

63-96: Consider refactoring route configuration for better maintainability.

The configureRoutes method could be improved by:

1. Extracting route paths into constants to avoid magic strings
2. Grouping related routes (e.g., lifecycle operations) into separate methods

Example refactor:

+    private static final String BASE_PATH = "/projects/:project/subscriptions";
+    private static final String SUBSCRIPTION_PATH = BASE_PATH + "/:subscription";
+
     private void configureRoutes() {
+        configureBasicOperations();
+        configureLifecycleOperations();
+    }
+
+    private void configureBasicOperations() {
         createRoute(
-                HttpMethod.POST, "/projects/:project/subscriptions",
+                HttpMethod.POST, BASE_PATH,
                 subscriptionHandlers::create, true
         );
         createRoute(
-                HttpMethod.GET, "/projects/:project/subscriptions/:subscription",
+                HttpMethod.GET, SUBSCRIPTION_PATH,
                 subscriptionHandlers::get, false
         );
         // ... other basic operations
+    }
+
+    private void configureLifecycleOperations() {
         createRoute(
-                HttpMethod.POST, "/projects/:project/subscriptions/:subscription/restore",
+                HttpMethod.POST, SUBSCRIPTION_PATH + "/restore",
                 subscriptionHandlers::restore, false
         );
         // ... other lifecycle operations
     }

---

141-143: Enhance error response validation in tests.

Consider extending the error response validation to include:

1. HTTP response headers verification
2. Error message format consistency checks
3. Error code validation where applicable

Example enhancement for one of the error cases:

     ErrorResponse response = sendRequestWithEntity(request, resource, 401, errorMessage, ErrorResponse.class);

+    // Verify error response format
     assertEquals(errorMessage, response.reason());
+    assertEquals("UNAUTHORIZED", response.code());
+    verify(response.headers().contains("X-Error-Code"));

Also applies to: 155-157, 169-171, 184-186, 197-199, 209-211, 221-223

---

258-306: Add tests for concurrent deletion scenarios and resource cleanup.

The deletion tests should be extended to cover:

1. Concurrent deletion attempts
2. Verification of related resource cleanup
3. Rollback scenarios when deletion partially fails

Example additional test:

@Test
void deleteSubscription_ConcurrentDeletion_HandlesGracefully() throws InterruptedException {
    HttpRequest<Buffer> request1 = createRequest(
            HttpMethod.DELETE,
            buildSubscriptionUrl("sub1", PROJECT) + "?deletionType=SOFT_DELETE"
    );
    HttpRequest<Buffer> request2 = createRequest(
            HttpMethod.DELETE,
            buildSubscriptionUrl("sub1", PROJECT) + "?deletionType=SOFT_DELETE"
    );

    doReturn(PROJECT).when(projectService).getCachedProject(PROJECT.getName());
    doReturn(CompletableFuture.completedFuture(null))
        .doThrow(new ConcurrentModificationException("Resource already being deleted"))
        .when(subscriptionService)
        .deleteSubscription(anyString(), eq(PROJECT), any(), eq(ResourceDeletionType.SOFT_DELETE));

    sendRequestWithoutPayload(request1, null);
    ErrorResponse response = sendRequestWithoutPayload(request2, 409, "Resource already being deleted", ErrorResponse.class);

    assertEquals("Resource already being deleted", response.reason());
}

---

328-350: Add tests for version conflicts and partial updates.

The update tests should be extended to cover:

1. Version conflict scenarios
2. Partial update capabilities
3. Update retries with version checks

Example additional test:

@Test
void updateSubscription_VersionConflict_ThrowsConflictException() throws InterruptedException {
    HttpRequest<Buffer> request = createRequest(HttpMethod.PUT, buildSubscriptionUrl("sub1", PROJECT));
    SubscriptionResource resource = createSubscriptionResource("sub1", PROJECT, TOPIC_RESOURCE);
    
    when(subscriptionService.updateSubscription(
            any(), anyInt(), anyString(), anyBoolean(), any(), any(), any(), any()
    )).thenThrow(new ConflictException("Version conflict"));

    ErrorResponse response = sendRequestWithEntity(
        request, resource, 409, "Version conflict", ErrorResponse.class);
    
    assertEquals("Version conflict", response.reason());
}

---

Line range hint 43-472: Consider overall test improvements for better maintainability.

While the test coverage is comprehensive, consider the following improvements:

1. Add JavaDoc comments describing test scenarios and edge cases
2. Implement test data builders to reduce duplication and improve maintainability
3. Group related tests using nested test classes (JUnit 5 @nested)

Example implementation:

@Builder
class SubscriptionTestData {
    private String name;
    private Project project;
    private TopicResource topic;
    private RetryPolicy retryPolicy;
    
    public static SubscriptionTestData defaultData() {
        return builder()
            .name("sub1")
            .project(Project.of("project1", "", "team1", "org1"))
            .topic(TopicResource.builder().build())
            .retryPolicy(RetryPolicy.builder().build())
            .build();
    }
}

@Nested
class SubscriptionCreationTests {
    @Test
    void whenValidInput_thenCreatesSuccessfully() {
        // Test implementation using SubscriptionTestData
    }
}

server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java (1)

125-125: Consider extending test coverage for soft-deleted state.

While the method replacement is correct, given the PR's objective to add soft-deletion support, consider adding test cases for the soft-deleted state alongside the existing Blocked, Throttled, and Replicating states.

server/src/test/java/com/flipkart/varadhi/services/DlqServiceTest.java (2)

182-183: Enhance helper method to support deletion states

The setupSubscriptionForGetMessages helper method should be enhanced to:

1. Support creation of soft-deleted subscriptions
2. Allow specification of subscription state for better test coverage

-private VaradhiSubscription setupSubscriptionForGetMessages() {
+private VaradhiSubscription setupSubscriptionForGetMessages(boolean isDeleted) {
     String consumerId = "consumerId";
     VaradhiTopic vTopic = TOPIC_RESOURCE.toVaradhiTopic();
     VaradhiSubscription subscription = spy(createUngroupedSubscription("sub12", PROJECT, vTopic));
+    when(subscription.isDeleted()).thenReturn(isDeleted);
     // ... rest of the method
     return subscription;
 }

---

Line range hint 1-196: Enhance test coverage for soft-deletion functionality

While the code changes maintain the existing test functionality, the test suite needs to be expanded to thoroughly cover the new soft-deletion feature. Consider adding the following test cases:

1. Lifecycle tests for soft-deleted subscriptions
2. DLQ operations on restored subscriptions
3. Error handling for various deletion states

This will ensure the robustness of the new deletion functionality and prevent potential issues in production.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (2)

251-254: Consider using a constant for the default deletion type.

The default deletion type could be extracted to a constant to improve maintainability and make the default behavior more explicit.

+ private static final ResourceDeletionType DEFAULT_DELETION_TYPE = ResourceDeletionType.SOFT_DELETE;

  ResourceDeletionType deletionType = ctx.queryParam(QUERY_PARAM_DELETION_TYPE).stream()
          .map(ResourceDeletionType::fromValue)
          .findFirst()
-         .orElse(ResourceDeletionType.SOFT_DELETE);
+         .orElse(DEFAULT_DELETION_TYPE);

---

413-418: Enhance property validation error handling.

Consider these improvements to the property validation:

1. Provide more descriptive error messages including the invalid value
2. Collect all validation errors before throwing the exception

+ List<String> validationErrors = new ArrayList<>();
  propertyValidators.forEach((key, validator) -> {
      String value = properties.get(key);
      if (!validator.isValid(value, usePermissible)) {
-         throw new IllegalArgumentException("Invalid value for property: " + key);
+         validationErrors.add(String.format("Invalid value '%s' for property: %s", value, key));
      }
  });
+ if (!validationErrors.isEmpty()) {
+     throw new IllegalArgumentException("Validation failed:\n" + String.join("\n", validationErrors));
+ }

server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (2)

63-107: Consider extracting route configuration to the base class.

The configureRoutes method and its helper createRoute contain common route setup logic that could be reused by other handler tests.

Consider moving this to SubscriptionTestBase:

-    private void configureRoutes() {
+    protected void configureRoutes() {

-    private void createRoute(HttpMethod method, String path, Handler<RoutingContext> handler, boolean requiresBody) {
+    protected void createRoute(HttpMethod method, String path, Handler<RoutingContext> handler, boolean requiresBody) {

---

338-344: Consider using ArgumentMatchers for better readability.

The test uses a mix of argument captors and matchers. Consider using specific argument matchers for better readability.

-        when(subscriptionService.updateSubscription(
-                stringCaptor.capture(), integerCaptor.capture(), anyString(), anyBoolean(), any(), any(), any(),
-                any()
-        )).thenReturn(
+        when(subscriptionService.updateSubscription(
+                eq("project1.sub1"), eq(0), anyString(), anyBoolean(), any(), any(), any(),
+                any()
+        )).thenReturn(

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java (1)

104-118: 🛠️ Refactor suggestion

Implement rollback mechanism for hard delete failures.

If storage topic deletion fails for some regions, the system could be left in an inconsistent state.

Consider implementing a two-phase deletion:

1. Mark all storage topics for deletion
2. Execute deletion with rollback capability

server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (1)

338-355: ⚠️ Potential issue

Implement retry mechanism for hard deletion.

The hard deletion implementation could leave the subscription in an inconsistent state if deProvision fails after updating the status but before completing deletion.

Consider implementing a retry mechanism or transaction to ensure atomicity:

     private void handleHardDelete(
             VaradhiSubscription subscription,
             Project subProject, ResourceActionRequest actionRequest
     ) {
         subscription.markDeleting(actionRequest.actionCode(), actionRequest.message());
         metaStore.updateSubscription(subscription);
 
+        int maxRetries = 3;
+        int retryCount = 0;
         try {
-            shardProvisioner.deProvision(subscription, subProject);
-            metaStore.deleteSubscription(subscription.getName());
-            log.info("Subscription '{}' deleted successfully.", subscription.getName());
+            while (retryCount < maxRetries) {
+                try {
+                    shardProvisioner.deProvision(subscription, subProject);
+                    metaStore.deleteSubscription(subscription.getName());
+                    log.info("Subscription '{}' deleted successfully.", subscription.getName());
+                    return;
+                } catch (Exception e) {
+                    retryCount++;
+                    if (retryCount == maxRetries) throw e;
+                    log.warn("Retry {} of {} for deleting subscription '{}'", 
+                            retryCount, maxRetries, subscription.getName(), e);
+                    Thread.sleep(1000 * retryCount);
+                }
+            }
         } catch (Exception e) {
             log.error("Failed to hard delete subscription '{}'.", subscription.getName(), e);
             subscription.markDeleteFailed(e.getMessage());
             metaStore.updateSubscription(subscription);
             throw e;
         }
     }

server/src/main/java/com/flipkart/varadhi/web/routes/RouteDefinition.java (1)

71-74: LGTM! Consider adding method-level documentation.

The implementation correctly adds PATCH support following the existing pattern. This aligns well with the PR objective of implementing soft deletion functionality, as PATCH is the appropriate HTTP method for partial updates like marking resources as deleted.

Consider adding method-level documentation to explain the purpose and usage, similar to other HTTP method builders:

+    /**
+     * Creates a new Builder instance for PATCH requests.
+     * @param name The name of the route
+     * @param path The path pattern for the route
+     * @return A new Builder instance configured for PATCH method
+     */
     public static Builder patch(String name, String path) {
         return new Builder(name, HttpMethod.PATCH, path);
     }

server/src/test/java/com/flipkart/varadhi/web/entities/TopicResourceTest.java (1)

17-77: Add test coverage for edge cases and error conditions.

While the current tests cover the basic functionality well, consider adding tests for:

1. Edge cases:

   • Topics with special characters in names
   • Maximum length names
   • Empty or null project names

2. Error conditions:

   • Invalid action codes
   • Null capacity policies
   • Invalid conversions

3. Negative scenarios:

   • Validation failures
   • Conversion failures

Here's a sample test for validation failures:

@Test
void from_ThrowsException_WhenInvalidTopicName() {
    assertThrows(IllegalArgumentException.class, () -> {
        VaradhiTopic varadhiTopic = VaradhiTopic.of(
            "projectName", "invalid/topic/name", true, 
            new TopicCapacityPolicy(),
            LifecycleStatus.ActionCode.SYSTEM_ACTION
        );
        TopicResource.from(varadhiTopic);
    });
}

server/src/main/java/com/flipkart/varadhi/utils/VaradhiTopicFactory.java (1)

22-27: Consider creating a tracking issue for the TODO.

The comment indicates that the deploymentRegion field is a temporary solution. To ensure this technical debt is properly tracked and addressed:

1. The region should be derived from TopicResource
2. Regional/HA/BCP-DR policies need to be implemented

Would you like me to help create an issue to track this technical debt?

server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (2)

88-115: Consider breaking down the route setup method for better maintainability.

The setupRoutes method handles multiple responsibilities. Consider extracting each route setup into separate methods for better organization and readability.

-    private void setupRoutes() {
+    private void setupRoutes() {
+        setupCreateTopicRoute();
+        setupGetTopicRoute();
+        setupListTopicsRoute();
+        setupDeleteTopicRoute();
+        setupRestoreTopicRoute();
+        setupFailureHandlers();
+    }
+
+    private void setupCreateTopicRoute() {
         router.post("/projects/:project/topics")
                 .handler(bodyHandler)
                 .handler(ctx -> {
                     topicHandlers.setRequestBody(ctx);
                     ctx.next();
                 })
                 .handler(ctx -> {
                     requestTelemetryConfigurator.addRequestSpanAndLog(ctx, "CreateTopic", TelemetryType.ALL);
                     ctx.next();
                 })
                 .handler(wrapBlocking(topicHandlers::create));
+    }

---

229-247: Consider adding edge cases for deletion type validation.

While the deletion type tests are comprehensive, consider adding test cases for:

1. Empty string deletion type
2. Case-insensitive deletion type handling

@Test
void deleteTopic_WithEmptyDeletionType_ShouldDefaultToDefault() throws InterruptedException {
    verifyDeleteRequest("", ResourceDeletionType.DEFAULT);
}

@Test
void deleteTopic_WithCaseInsensitiveDeletionType_ShouldWork() throws InterruptedException {
    verifyDeleteRequest("soft_delete", ResourceDeletionType.SOFT_DELETE);
}

server/src/test/java/com/flipkart/varadhi/utils/VaradhiTopicFactoryTest.java (1)

90-112: Consider alternatives to testing private methods with reflection.

While the test is well-implemented, using reflection to test private methods is generally not recommended as it:

1. Makes tests brittle and harder to maintain
2. May indicate that the private method should be extracted into a separate class

Consider either:

• Moving the deployment logic to a separate strategy class
• Making the method package-private for testing
• Testing the behavior through public methods instead

server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java (1)

48-55: Enhance error message in JSON deserialization.

While the error handling is good, consider including more context in the error message.

-            throw new RuntimeException("Failed to deserialize JSON", e);
+            throw new RuntimeException(
+                String.format("Failed to deserialize JSON to %s<%s>", collectionClass.getSimpleName(), clazz.getSimpleName()),
+                e
+            );

server/src/test/java/com/flipkart/varadhi/web/entities/SubscriptionResourceTest.java (2)

17-35: Add test cases for edge cases and invalid inputs.

While the happy path is well tested, consider adding test cases for:

• Invalid subscription names (null, empty, special characters)
• Null project or topic
• Edge cases for other properties

This will ensure the validation logic is working correctly.

---

61-73: Add test cases for internal name edge cases.

Consider adding test cases for:

• Project names with special characters
• Subscription names with special characters
• Empty or null project/subscription names
• Maximum length validation

This will ensure the internal name generation is robust against all inputs.

entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (2)

43-61: Consider adding parameter validation.

While the factory method changes are good, consider adding validation for the actionCode parameter to prevent null values.

 public static VaradhiTopic of(
         String project, String name, boolean grouped, TopicCapacityPolicy capacity,
         LifecycleStatus.ActionCode actionCode
 ) {
+    if (actionCode == null) {
+        throw new IllegalArgumentException("Action code cannot be null");
+    }
     return new VaradhiTopic(
             buildTopicName(project, name), INITIAL_VERSION, grouped, capacity, new HashMap<>(),
             new LifecycleStatus(LifecycleStatus.State.ACTIVE, actionCode)
     );
 }

---

107-135: Consider adding parameter validation and thread safety.

The lifecycle management methods look good but consider:

1. Adding null checks for parameters in markActive and markInactive.
2. Adding synchronization or making the methods thread-safe if concurrent modifications are possible.

 public void markActive(LifecycleStatus.ActionCode actionCode, String message) {
+    if (actionCode == null) {
+        throw new IllegalArgumentException("Action code cannot be null");
+    }
+    if (message == null) {
+        throw new IllegalArgumentException("Message cannot be null");
+    }
     this.status.update(LifecycleStatus.State.ACTIVE, message, actionCode);
 }

 public void markInactive(LifecycleStatus.ActionCode actionCode, String message) {
+    if (actionCode == null) {
+        throw new IllegalArgumentException("Action code cannot be null");
+    }
+    if (message == null) {
+        throw new IllegalArgumentException("Message cannot be null");
+    }
     this.status.update(LifecycleStatus.State.INACTIVE, message, actionCode);
 }

If concurrent modifications are possible, consider adding synchronization:

+import java.util.concurrent.locks.ReentrantReadWriteLock;

 public class VaradhiTopic extends AbstractTopic {
+    private final ReentrantReadWriteLock statusLock = new ReentrantReadWriteLock();

     public void markActive(LifecycleStatus.ActionCode actionCode, String message) {
+        statusLock.writeLock().lock();
+        try {
             this.status.update(LifecycleStatus.State.ACTIVE, message, actionCode);
+        } finally {
+            statusLock.writeLock().unlock();
+        }
     }

     public void markInactive(LifecycleStatus.ActionCode actionCode, String message) {
+        statusLock.writeLock().lock();
+        try {
             this.status.update(LifecycleStatus.State.INACTIVE, message, actionCode);
+        } finally {
+            statusLock.writeLock().unlock();
+        }
     }

     @JsonIgnore
     public boolean isActive() {
+        statusLock.readLock().lock();
+        try {
             return this.status.getState() == LifecycleStatus.State.ACTIVE;
+        } finally {
+            statusLock.readLock().unlock();
+        }
     }
 }

server/src/test/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactoryTest.java (2)

25-40: Consider moving test constants to a shared test constants file.

The test constants could be reused across other test classes. Consider extracting them to a shared test constants file to promote reusability and maintainability.

---

41-47: Consider verifying mock initialization.

While the setup is correct, consider adding assertions to verify that the mocks are properly initialized and injected. This can help catch initialization issues early.

Example:

 @Override
 @BeforeEach
 public void setUp() {
     MockitoAnnotations.openMocks(this);
     varadhiSubscriptionFactory =
             new VaradhiSubscriptionFactory(topicService, subscriptionFactory, topicFactory, REGION);
+    assertNotNull(subscriptionFactory, "subscriptionFactory should be initialized");
+    assertNotNull(topicFactory, "topicFactory should be initialized");
+    assertNotNull(topicService, "topicService should be initialized");
 }

entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java (2)

19-31: Consider adding Javadoc to document the helper class and method.

The helper class and method effectively reduce code duplication. Consider adding Javadoc to explain their purpose and parameters.

+/**
+ * A dummy implementation of StorageTopic for testing purposes.
+ */
 @EqualsAndHashCode(callSuper = true)
 public static class DummyStorageTopic extends StorageTopic {

+/**
+ * Creates a VaradhiTopic instance with default parameters.
+ * @param grouped whether the topic is grouped
+ * @return a new VaradhiTopic instance
+ */
 private VaradhiTopic createDefaultVaradhiTopic(boolean grouped) {

---

111-146: Consider adding tests for invalid state transitions.

The tests cover basic state transitions well. Consider adding tests for:

1. Invalid state transitions (e.g., attempting to activate an already active topic)
2. Invalid action codes
3. Empty or null reason messages

+@Test
+void markActive_WhenAlreadyActive_ThrowsException() {
+    VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false);
+    
+    assertThrows(IllegalStateException.class, () ->
+        varadhiTopic.markActive(LifecycleStatus.ActionCode.SYSTEM_ACTION, "Already active"));
+}

+@Test
+void markInactive_WithNullReason_ThrowsException() {
+    VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false);
+    
+    assertThrows(IllegalArgumentException.class, () ->
+        varadhiTopic.markInactive(LifecycleStatus.ActionCode.SYSTEM_ACTION, null));
+}

server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java (3)

55-70: Enhance error message for inactive topics.

The error message could be more descriptive by indicating that the topic exists but is inactive.

-            throw new ResourceNotFoundException("Topic %s not found.".formatted(topicName));
+            throw new ResourceNotFoundException("Topic %s exists but is inactive.".formatted(topicName));

---

81-81: Track TODO for namespace cleanup.

The TODO comment about namespace cleanup should be tracked in the issue tracker.

Would you like me to create an issue to track the namespace cleanup implementation?

---

203-206: Consider pushing filter to database layer.

The current implementation loads all topics and filters in memory. Consider adding a filter parameter to the MetaStore query.

entities/src/main/java/com/flipkart/varadhi/entities/auth/ResourceAction.java (1)

91-118: Consider tightening validation or naming for the nested Action enum
The addition of multiple actions (RESTORE, CONSUME, PRODUCE, etc.) is comprehensive. However, if certain actions require special handling or mandatory parameters at runtime, consider adding validations or distinct methods to prevent misuse. For example, some consumers might attempt to RESTORE or MIGRATE resources without the correct prerequisites.

server/src/main/java/com/flipkart/varadhi/web/entities/TopicResource.java (1)

22-32: Check field mutability versus final usage
Fields capacity (line 26) and actionCode (line 31) are assigned in the constructor but also have @Setter. Confirm if it’s necessary for these fields to be mutable after construction; if not, consider removing the setters to maintain immutability and reduce accidental state changes.

server/src/main/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactory.java (1)

157-196: Improve shard capacity and multi-shard logic
The getMultiShard and getShardCapacity methods (lines 157–196) cleanly separate single vs. multiple shard creation. However, if advanced load balancing or dynamic scaling is introduced in the future, you may want to expand these methods to factor in usage metrics or cluster constraints beyond a simple 1/shardCount capacity split.

entities/src/main/java/com/flipkart/varadhi/entities/ResourceActionRequest.java (1)

14-18: Consider enhancing the error message.

The error message could be more descriptive to help with debugging.

-            throw new NullPointerException("actionCode must not be null");
+            throw new NullPointerException("ResourceActionRequest: actionCode must not be null");

entities/src/test/java/com/flipkart/varadhi/entities/ResourceActionRequestTest.java (1)

10-59: Consider adding tests for Validatable interface.

Since ResourceActionRequest implements Validatable, consider adding tests to verify the validation behavior.

@Test
void validate_ValidatesSuccessfully() {
    ResourceActionRequest request = new ResourceActionRequest(
            LifecycleStatus.ActionCode.SYSTEM_ACTION,
            "Test message"
    );
    request.validate(); // Should not throw any exception
}

entities/src/test/java/com/flipkart/varadhi/entities/LifecycleStatusTest.java (1)

10-68: Consider adding tests for invalid state transitions.

Since this is related to lifecycle management, it would be valuable to add tests that verify the behavior when invalid state transitions are attempted.

@Test
void update_InvalidStateTransition_ThrowsException() {
    LifecycleStatus status = new LifecycleStatus(
            LifecycleStatus.State.DELETED,
            LifecycleStatus.ActionCode.SYSTEM_ACTION
    );
    assertThrows(IllegalStateException.class, () -> 
        status.update(LifecycleStatus.State.CREATING, "Invalid transition")
    );
}

entities/src/testFixtures/java/com/flipkart/varadhi/entities/SubscriptionUtils.java (1)

56-59: Consider adding validation for negative test scenarios.

The subscription creation with LifecycleStatus.ActionCode.SYSTEM_ACTION looks good, but consider adding utility methods for creating subscriptions with different lifecycle states to support negative test cases.

+    /**
+     * Creates a subscription with a specific lifecycle status for testing.
+     *
+     * @param name the subscription name
+     * @param project the project name
+     * @param description the description
+     * @param actionCode the lifecycle status action code
+     * @return the subscription
+     */
+    public static VaradhiSubscription createWithStatus(
+            String name,
+            String project,
+            String description,
+            LifecycleStatus.ActionCode actionCode
+    ) {
+        return TopicResource.unGrouped(name, project, description, actionCode);
+    }

server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java (1)

183-197: Consider adding transaction boundaries for deletion operations.

The deletion logic looks good, but there's a potential race condition between state check and actual deletion. Consider adding transaction boundaries or optimistic locking.

Also, enhance logging to include the deletion type for better traceability:

-        log.info("Subscription '{}' deleted successfully.", subscription.getName());
+        log.info("Subscription '{}' {} successfully.", subscription.getName(), 
+                 deletionType.equals(ResourceDeletionType.HARD_DELETE) ? "hard deleted" : "soft deleted");