Increase test coverage

linkedin · Feb 8, 2025 · 1d596a8 · 1d596a8
1 parent f1479e6
commit 1d596a8
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 18 deletions.
diff --git a/...ce-common/src/main/java/com/linkedin/venice/controllerapi/AdminTopicMetadataResponse.java b/...ce-common/src/main/java/com/linkedin/venice/controllerapi/AdminTopicMetadataResponse.java
@@ -51,7 +51,7 @@ public void setAdminOperationProtocolVersion(long adminOperationProtocolVersion)
     this.adminOperationProtocolVersion = adminOperationProtocolVersion;
   }
 
-  public Long getAdminOperationProtocolVersion() {
+  public long getAdminOperationProtocolVersion() {
     return adminOperationProtocolVersion;
   }
 }
diff --git a/...common/src/integrationTest/java/com/linkedin/venice/controller/TestAdminToolEndToEnd.java b/...common/src/integrationTest/java/com/linkedin/venice/controller/TestAdminToolEndToEnd.java
@@ -214,7 +214,7 @@ public void testUpdateAdminOperationVersion() throws Exception {
 
       // Setup the original metadata
       AdminTopicMetadataResponse originalMetadata = controllerClient.getAdminTopicMetadata(Optional.empty());
-      Assert.assertEquals(originalMetadata.getAdminOperationProtocolVersion(), currentVersion);
+      Assert.assertEquals(originalMetadata.getAdminOperationProtocolVersion(), (long) currentVersion);
 
       // Update the admin operation version to newVersion - 80
       String[] updateAdminOperationVersionArgs =
@@ -226,7 +226,7 @@ public void testUpdateAdminOperationVersion() throws Exception {
       // Verify the admin operation metadata version is updated
       TestUtils.waitForNonDeterministicAssertion(30, TimeUnit.SECONDS, () -> {
         AdminTopicMetadataResponse updatedMetadata = controllerClient.getAdminTopicMetadata(Optional.empty());
-        Assert.assertEquals(updatedMetadata.getAdminOperationProtocolVersion(), newVersion);
+        Assert.assertEquals(updatedMetadata.getAdminOperationProtocolVersion(), (long) newVersion);
       });
     }
   }

diff --git a/...troller/src/main/java/com/linkedin/venice/controller/server/AdminTopicMetadataRoutes.java b/...troller/src/main/java/com/linkedin/venice/controller/server/AdminTopicMetadataRoutes.java
@@ -97,19 +97,6 @@ public Route updateAdminTopicMetadata(Admin admin, ClusterAdminOpsRequestHandler
             UpdateAdminTopicMetadataGrpcRequest.newBuilder().setMetadata(adminMetadataBuilder).build());
         responseObject.setCluster(internalResponse.getClusterName());
         responseObject.setName(internalResponse.hasStoreName() ? internalResponse.getStoreName() : null);
-        if (storeName.isPresent()) {
-          if (offset.isPresent() || upstreamOffset.isPresent()) {
-            throw new VeniceException("There is no store-level offsets to be updated");
-          }
-        } else {
-          if (!offset.isPresent() || !upstreamOffset.isPresent()) {
-            throw new VeniceException("Offsets must be provided to update cluster-level admin topic metadata");
-          }
-        }
-
-        responseObject.setCluster(clusterName);
-        storeName.ifPresent(responseObject::setName);
-
         admin.updateAdminTopicMetadata(clusterName, executionId, storeName, offset, upstreamOffset);
       } catch (Throwable e) {
         responseObject.setError(e);
@@ -121,7 +108,7 @@ public Route updateAdminTopicMetadata(Admin admin, ClusterAdminOpsRequestHandler
 
   public Route updateAdminOperationProtocolVersion(Admin admin) {
     return (request, response) -> {
-      ControllerResponse responseObject = new ControllerResponse();
+      AdminTopicMetadataResponse responseObject = new AdminTopicMetadataResponse();
       response.type(HttpConstants.JSON);
       try {
         if (!isAllowListUser(request)) {
@@ -136,9 +123,9 @@ public Route updateAdminOperationProtocolVersion(Admin admin) {
         Long adminOperationProtocolVersion = Long.parseLong(request.queryParams(ADMIN_OPERATION_PROTOCOL_VERSION));
 
         responseObject.setCluster(clusterName);
+        responseObject.setAdminOperationProtocolVersion(adminOperationProtocolVersion);
 
         admin.updateAdminOperationProtocolVersion(clusterName, adminOperationProtocolVersion);
-
       } catch (Throwable e) {
         responseObject.setError(e);
         AdminSparkServer.handleError(new VeniceException(e), request, response);

diff --git a/...ler/src/test/java/com/linkedin/venice/controller/server/AdminTopicMetadataRoutesTest.java b/...ler/src/test/java/com/linkedin/venice/controller/server/AdminTopicMetadataRoutesTest.java
@@ -1,6 +1,7 @@
 package com.linkedin.venice.controller.server;
 
 import static com.linkedin.venice.VeniceConstants.CONTROLLER_SSL_CERTIFICATE_ATTRIBUTE_NAME;
+import static com.linkedin.venice.controllerapi.ControllerApiConstants.ADMIN_OPERATION_PROTOCOL_VERSION;
 import static com.linkedin.venice.controllerapi.ControllerApiConstants.CLUSTER;
 import static com.linkedin.venice.controllerapi.ControllerApiConstants.EXECUTION_ID;
 import static com.linkedin.venice.controllerapi.ControllerApiConstants.STORE_NAME;
@@ -215,4 +216,72 @@ public void testUpdateAdminTopicMetadataHandlesException() throws Exception {
     assertNotNull(responseObject.getError());
     assertTrue(responseObject.getError().contains("Internal error"));
   }
+
+  @Test
+  public void testUpdateAdminOperationProtocolVersion() throws Exception {
+    QueryParamsMap paramsMap = mock(QueryParamsMap.class);
+    String adminOperationProtocolVersion = "1";
+    doReturn(new HashMap<>()).when(paramsMap).toMap();
+    doReturn(paramsMap).when(request).queryMap();
+
+    when(request.queryParams(CLUSTER)).thenReturn(TEST_CLUSTER);
+    when(request.queryParams(ADMIN_OPERATION_PROTOCOL_VERSION)).thenReturn(adminOperationProtocolVersion);
+
+    Route route = new AdminTopicMetadataRoutes(false, Optional.empty()).updateAdminOperationProtocolVersion(mockAdmin);
+
+    AdminTopicMetadataResponse responseObject =
+        OBJECT_MAPPER.readValue(route.handle(request, response).toString(), AdminTopicMetadataResponse.class);
+
+    assertEquals(responseObject.getCluster(), TEST_CLUSTER);
+    assertEquals(responseObject.getAdminOperationProtocolVersion(), 1L);
+    assertNull(responseObject.getError());
+  }
+
+  @Test
+  public void testUpdateAdminOperationProtocolVersionHandlesUnauthorizedAccess() throws Exception {
+    DynamicAccessController accessController = mock(DynamicAccessController.class);
+    when(accessController.isAllowlistUsers(any(), any(), any())).thenReturn(false);
+    HttpServletRequest httpServletRequest = mock(HttpServletRequest.class);
+    when(request.raw()).thenReturn(httpServletRequest);
+    X509Certificate certificate = mock(X509Certificate.class);
+    X500Principal principal = new X500Principal("CN=foo");
+    X509Certificate[] certificates = new X509Certificate[] { mock(X509Certificate.class) };
+    when(httpServletRequest.getAttribute(CONTROLLER_SSL_CERTIFICATE_ATTRIBUTE_NAME)).thenReturn(certificates);
+    doReturn(principal).when(certificate).getSubjectX500Principal();
+    doReturn(httpServletRequest).when(request).raw();
+
+    QueryParamsMap paramsMap = mock(QueryParamsMap.class);
+    String adminOperationProtocolVersion = "1";
+    doReturn(new HashMap<>()).when(paramsMap).toMap();
+    doReturn(paramsMap).when(request).queryMap();
+
+    when(request.queryParams(CLUSTER)).thenReturn(TEST_CLUSTER);
+    when(request.queryParams(ADMIN_OPERATION_PROTOCOL_VERSION)).thenReturn(adminOperationProtocolVersion);
+
+    Route route = new AdminTopicMetadataRoutes(false, Optional.of(accessController))
+        .updateAdminOperationProtocolVersion(mockAdmin);
+
+    AdminTopicMetadataResponse responseObject =
+        OBJECT_MAPPER.readValue(route.handle(request, response).toString(), AdminTopicMetadataResponse.class);
+
+    assertNotNull(responseObject.getError());
+    assertTrue(responseObject.getError().contains("Only admin users are allowed"));
+  }
+
+  @Test
+  public void testUpdateAdminOperationProtocolVersionHandlesMissingParams() throws Exception {
+    QueryParamsMap paramsMap = mock(QueryParamsMap.class);
+    doReturn(new HashMap<>()).when(paramsMap).toMap();
+    doReturn(paramsMap).when(request).queryMap();
+
+    when(request.queryParams(CLUSTER)).thenReturn(null); // Missing cluster parameter
+
+    Route route = new AdminTopicMetadataRoutes(false, Optional.empty()).updateAdminOperationProtocolVersion(mockAdmin);
+    AdminTopicMetadataResponse responseObject =
+        OBJECT_MAPPER.readValue(route.handle(request, response).toString(), AdminTopicMetadataResponse.class);
+
+    verify(requestHandler, never()).getAdminTopicMetadata(any());
+    assertNotNull(responseObject.getError());
+    assertTrue(responseObject.getError().contains("cluster_name is a required parameter"));
+  }
 }