This is an automated email from the ASF dual-hosted git repository.

apucher pushed a commit to branch add-table-auth-cleanup
in repository https://gitbox.apache.org/repos/asf/pinot.git

commit 1bf9931a534913b6a137040d032a4e034335ce50
Author: Alexander Pucher <apuc...@apache.org>
AuthorDate: Tue Aug 16 21:31:53 2022 -0700

    add-table refactor for clean auth
---
 .../controller/api/access/AccessControlUtils.java  | 79 ++++++++++------------
 .../api/access/AuthenticationFilter.java           | 45 ++++++------
 .../PinotAccessControlUserRestletResource.java     | 17 ++---
 .../api/resources/PinotSchemaRestletResource.java  |  5 +-
 .../api/resources/PinotTableRestletResource.java   |  5 +-
 .../api/resources/TableConfigsRestletResource.java |  7 +-
 .../api/access/AuthenticationFilterTest.java       | 23 +++----
 .../pinot/tools/admin/command/AddTableCommand.java | 52 +++++++-------
 8 files changed, 103 insertions(+), 130 deletions(-)

diff --git 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
index c12ba307bd..88d71736ba 100644
--- 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
+++ 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
@@ -19,9 +19,9 @@
 
 package org.apache.pinot.controller.api.access;
 
-import java.util.Optional;
 import javax.ws.rs.core.HttpHeaders;
 import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.StringUtils;
 import 
org.apache.pinot.controller.api.exception.ControllerApplicationException;
 import org.apache.pinot.spi.utils.builder.TableNameBuilder;
 import org.slf4j.Logger;
@@ -31,7 +31,11 @@ import org.slf4j.LoggerFactory;
 /**
  * Utility class to simplify access control validation. This class is simple 
wrapper around AccessControl class.
  */
-public class AccessControlUtils {
+public final class AccessControlUtils {
+  private AccessControlUtils() {
+    // left blank
+  }
+
   private static final Logger LOGGER = 
LoggerFactory.getLogger(AccessControlUtils.class);
 
   /**
@@ -43,9 +47,28 @@ public class AccessControlUtils {
    * @param endpointUrl the request url for which this access control is called
    * @param accessControl AccessControl object which does the actual validation
    */
-  public void validatePermission(String tableName, AccessType accessType, 
HttpHeaders httpHeaders, String endpointUrl,
-      AccessControl accessControl) {
-    validatePermission(Optional.of(tableName), accessType, httpHeaders, 
endpointUrl, accessControl);
+  public static void validatePermission(String tableName, AccessType 
accessType, HttpHeaders httpHeaders,
+      String endpointUrl, AccessControl accessControl) {
+    String accessTypeToEndpointMsg =
+        String.format("access type '%s' to the endpoint '%s' for table '%s'", 
accessType, endpointUrl, tableName);
+    try {
+      if (StringUtils.isBlank(tableName)) {
+        if (!accessControl.hasAccess(accessType, httpHeaders, endpointUrl)) {
+          accessDenied(accessTypeToEndpointMsg);
+        }
+      } else {
+        String rawTableName = TableNameBuilder.extractRawTableName(tableName);
+        if (!accessControl.hasAccess(rawTableName, accessType, httpHeaders, 
endpointUrl)) {
+          accessDenied(accessTypeToEndpointMsg);
+        }
+      }
+    } catch (Exception e) {
+      if (!(e instanceof ControllerApplicationException)) {
+        throw new ControllerApplicationException(LOGGER,
+            "Caught exception while validating permission for " + 
accessTypeToEndpointMsg,
+            Response.Status.INTERNAL_SERVER_ERROR, e);
+      }
+    }
   }
 
   /**
@@ -56,55 +79,25 @@ public class AccessControlUtils {
    * @param endpointUrl the request url for which this access control is called
    * @param accessControl AccessControl object which does the actual validation
    */
-  public void validatePermission(AccessType accessType, HttpHeaders 
httpHeaders, String endpointUrl,
+  public static void validatePermission(AccessType accessType, HttpHeaders 
httpHeaders, String endpointUrl,
       AccessControl accessControl) {
-    validatePermission(Optional.empty(), accessType, httpHeaders, endpointUrl, 
accessControl);
+    validatePermission(null, accessType, httpHeaders, endpointUrl, 
accessControl);
   }
 
   /**
    * Validate permission for the given access type against the given table
    *
-   * @param tableNameOpt name of the table to be accessed; if `none`, it's a 
non-table level endpoint.
-   * @param accessType type of the access
    * @param httpHeaders HTTP headers containing requester identity required by 
access control object
    * @param endpointUrl the request url for which this access control is called
-   * @param accessControl AccessControl object which does the actual validation
    */
-  public void validatePermission(Optional<String> tableNameOpt, AccessType 
accessType, HttpHeaders httpHeaders,
-      String endpointUrl, AccessControl accessControl) {
-    boolean hasPermission;
-    String accessTypeToEndpointMsg =
-        String.format("access type '%s' to the endpoint '%s'", accessType, 
endpointUrl) + tableNameOpt
-            .map(name -> String.format(" for table '%s'", name)).orElse("");
-    try {
-      if (tableNameOpt.isPresent()) {
-        String rawTableName = 
TableNameBuilder.extractRawTableName(tableNameOpt.get());
-        hasPermission = accessControl.hasAccess(rawTableName, accessType, 
httpHeaders, endpointUrl);
-      } else {
-        hasPermission = accessControl.hasAccess(accessType, httpHeaders, 
endpointUrl);
-      }
-    } catch (Exception e) {
-      throw new ControllerApplicationException(LOGGER,
-          "Caught exception while validating permission for " + 
accessTypeToEndpointMsg,
-          Response.Status.INTERNAL_SERVER_ERROR, e);
-    }
-    if (!hasPermission) {
-      throw new ControllerApplicationException(LOGGER, "Permission is denied 
for " + accessTypeToEndpointMsg,
-          Response.Status.FORBIDDEN);
+  public static void validatePermission(HttpHeaders httpHeaders, String 
endpointUrl, AccessControl accessControl) {
+    if (!accessControl.hasAccess(httpHeaders)) {
+      accessDenied(endpointUrl);
     }
   }
 
-  /**
-   * Validate permission for the given access type against the given table
-   *
-   * @param httpHeaders HTTP headers containing requester identity required by 
access control object
-   * @param endpointUrl the request url for which this access control is called
-   */
-  public void validatePermission(HttpHeaders httpHeaders, String endpointUrl,
-      AccessControl accessControl) {
-    if (!accessControl.hasAccess(httpHeaders)) {
-      throw new ControllerApplicationException(LOGGER, "Permission is denied 
for " + endpointUrl,
-          Response.Status.FORBIDDEN);
-    }
+  private static void accessDenied(String resource) {
+    throw new ControllerApplicationException(LOGGER, "Permission is denied for 
" + resource,
+        Response.Status.FORBIDDEN);
   }
 }
diff --git 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java
 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java
index b25dbdcc9b..8ebd1a2883 100644
--- 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java
+++ 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AuthenticationFilter.java
@@ -24,7 +24,6 @@ import java.io.IOException;
 import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.HashSet;
-import java.util.Optional;
 import java.util.Set;
 import javax.inject.Inject;
 import javax.inject.Provider;
@@ -49,6 +48,9 @@ import org.glassfish.grizzly.http.server.Request;
 public class AuthenticationFilter implements ContainerRequestFilter {
   private static final Set<String> UNPROTECTED_PATHS =
       new HashSet<>(Arrays.asList("", "help", "auth/info", "auth/verify", 
"health"));
+  private static final String KEY_TABLE_NAME = "tableName";
+  private static final String KEY_TABLE_NAME_WITH_TYPE = "tableNameWithType";
+  private static final String KEY_SCHEMA_NAME = "schemaName";
 
   @Inject
   Provider<Request> _requestProvider;
@@ -86,49 +88,50 @@ public class AuthenticationFilter implements 
ContainerRequestFilter {
     //     - "tableNameWithType", or
     //     - "schemaName"
     // If table name is not available, it means the endpoint is not a 
table-level endpoint.
-    Optional<String> tableName = extractTableName(uriInfo.getPathParameters(), 
uriInfo.getQueryParameters());
+    String tableName = extractTableName(uriInfo.getPathParameters(), 
uriInfo.getQueryParameters());
     AccessType accessType = extractAccessType(endpointMethod);
-    new AccessControlUtils().validatePermission(tableName, accessType, 
_httpHeaders, endpointUrl, accessControl);
+    AccessControlUtils.validatePermission(tableName, accessType, _httpHeaders, 
endpointUrl, accessControl);
   }
 
   @VisibleForTesting
   AccessType extractAccessType(Method endpointMethod) {
-    // default access type
-    AccessType accessType = AccessType.READ;
     if (endpointMethod.isAnnotationPresent(Authenticate.class)) {
-      accessType = endpointMethod.getAnnotation(Authenticate.class).value();
+      return endpointMethod.getAnnotation(Authenticate.class).value();
     } else {
       // heuristically infer access type via javax.ws.rs annotations
       if (endpointMethod.getAnnotation(POST.class) != null) {
-        accessType = AccessType.CREATE;
+        return AccessType.CREATE;
       } else if (endpointMethod.getAnnotation(PUT.class) != null) {
-        accessType = AccessType.UPDATE;
+        return AccessType.UPDATE;
       } else if (endpointMethod.getAnnotation(DELETE.class) != null) {
-        accessType = AccessType.DELETE;
+        return AccessType.DELETE;
       }
     }
-    return accessType;
+
+    return AccessType.READ;
   }
 
   @VisibleForTesting
-  Optional<String> extractTableName(MultivaluedMap<String, String> 
pathParameters,
+  static String extractTableName(MultivaluedMap<String, String> pathParameters,
       MultivaluedMap<String, String> queryParameters) {
-    Optional<String> tableName = extractTableName(pathParameters);
-    if (tableName.isPresent()) {
+    String tableName = extractTableName(pathParameters);
+    if (tableName != null) {
       return tableName;
     }
     return extractTableName(queryParameters);
   }
 
-  private Optional<String> extractTableName(MultivaluedMap<String, String> 
mmap) {
-    String tableName = mmap.getFirst("tableName");
-    if (tableName == null) {
-      tableName = mmap.getFirst("tableNameWithType");
-      if (tableName == null) {
-        tableName = mmap.getFirst("schemaName");
-      }
+  private static String extractTableName(MultivaluedMap<String, String> mmap) {
+    if (mmap.containsKey(KEY_TABLE_NAME)) {
+      return mmap.getFirst(KEY_TABLE_NAME);
+    }
+    if (mmap.containsKey(KEY_TABLE_NAME_WITH_TYPE)) {
+      return mmap.getFirst(KEY_TABLE_NAME_WITH_TYPE);
+    }
+    if (mmap.containsKey(KEY_SCHEMA_NAME)) {
+      return mmap.getFirst(KEY_SCHEMA_NAME);
     }
-    return Optional.ofNullable(tableName);
+    return null;
   }
 
   private static boolean isBaseFile(String path) {
diff --git 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotAccessControlUserRestletResource.java
 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotAccessControlUserRestletResource.java
index c5e5c3a75b..b1d3b9a437 100644
--- 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotAccessControlUserRestletResource.java
+++ 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotAccessControlUserRestletResource.java
@@ -93,11 +93,8 @@ public class PinotAccessControlUserRestletResource {
     @Inject
     PinotHelixResourceManager _pinotHelixResourceManager;
 
-
     @Inject
     AccessControlFactory _accessControlFactory;
-    AccessControlUtils _accessControlUtils = new AccessControlUtils();
-
 
     @GET
     @Produces(MediaType.APPLICATION_JSON)
@@ -106,8 +103,7 @@ public class PinotAccessControlUserRestletResource {
     public String listUers(@Context HttpHeaders httpHeaders, @Context Request 
request) {
         try {
             String endpointUrl = request.getRequestURL().toString();
-            _accessControlUtils
-                .validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
+            AccessControlUtils.validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
             ZkHelixPropertyStore<ZNRecord> propertyStore = 
_pinotHelixResourceManager.getPropertyStore();
             Map<String, UserConfig> allUserInfo = 
ZKMetadataProvider.getAllUserInfo(propertyStore);
             return JsonUtils.newObjectNode().set("users", 
JsonUtils.objectToJsonNode(allUserInfo)).toString();
@@ -124,8 +120,7 @@ public class PinotAccessControlUserRestletResource {
         @Context HttpHeaders httpHeaders, @Context Request request) {
         try {
             String endpointUrl = request.getRequestURL().toString();
-            _accessControlUtils
-                .validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
+            AccessControlUtils.validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
             ZkHelixPropertyStore<ZNRecord> propertyStore = 
_pinotHelixResourceManager.getPropertyStore();
             ComponentType componentType = 
Constants.validateComponentType(componentTypeStr);
             String usernameWithType = username + "_" + componentType.name();
@@ -150,8 +145,7 @@ public class PinotAccessControlUserRestletResource {
             userConfig = JsonUtils.stringToObject(userConfigStr, 
UserConfig.class);
             username = userConfig.getUserName();
             String endpointUrl = request.getRequestURL().toString();
-            _accessControlUtils
-                .validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
+            AccessControlUtils.validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
             if (username.contains(".") || username.contains(" ")) {
                 throw new IllegalStateException("Username: " + username + " 
containing '.' or space is not allowed");
             }
@@ -189,7 +183,7 @@ public class PinotAccessControlUserRestletResource {
             userExist = _pinotHelixResourceManager.hasUser(username, 
componentTypeStr);
 
             String endpointUrl = request.getRequestURL().toString();
-            _accessControlUtils.validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
+            AccessControlUtils.validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
 
             _pinotHelixResourceManager.deleteUser(usernameWithComponentType);
             if (userExist) {
@@ -224,8 +218,7 @@ public class PinotAccessControlUserRestletResource {
         String usernameWithComponentType = username + "_" + componentTypeStr;
         try {
             String endpointUrl = request.getRequestURL().toString();
-            _accessControlUtils
-                .validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
+            AccessControlUtils.validatePermission(httpHeaders, endpointUrl, 
_accessControlFactory.create());
 
             userConfig = JsonUtils.stringToObject(userConfigString, 
UserConfig.class);
             if (passwordChanged) {
diff --git 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
index bee1161465..2bb05d0553 100644
--- 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
+++ 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
@@ -96,7 +96,6 @@ public class PinotSchemaRestletResource {
 
   @Inject
   AccessControlFactory _accessControlFactory;
-  AccessControlUtils _accessControlUtils = new AccessControlUtils();
 
   @GET
   @Produces(MediaType.APPLICATION_JSON)
@@ -219,7 +218,7 @@ public class PinotSchemaRestletResource {
     Schema schema = schemaAndUnrecognizedProps.getLeft();
     String endpointUrl = request.getRequestURL().toString();
     validateSchemaName(schema.getSchemaName());
-    _accessControlUtils.validatePermission(schema.getSchemaName(), 
AccessType.CREATE, httpHeaders, endpointUrl,
+    AccessControlUtils.validatePermission(schema.getSchemaName(), 
AccessType.CREATE, httpHeaders, endpointUrl,
         _accessControlFactory.create());
     SuccessResponse successResponse = addSchema(schema, override);
     return new ConfigSuccessResponse(successResponse.getStatus(), 
schemaAndUnrecognizedProps.getRight());
@@ -251,7 +250,7 @@ public class PinotSchemaRestletResource {
     Schema schema = schemaAndUnrecognizedProperties.getLeft();
     String endpointUrl = request.getRequestURL().toString();
     validateSchemaName(schema.getSchemaName());
-    _accessControlUtils.validatePermission(schema.getSchemaName(), 
AccessType.CREATE, httpHeaders, endpointUrl,
+    AccessControlUtils.validatePermission(schema.getSchemaName(), 
AccessType.CREATE, httpHeaders, endpointUrl,
         _accessControlFactory.create());
     SuccessResponse successResponse = addSchema(schema, override);
     return new ConfigSuccessResponse(successResponse.getStatus(), 
schemaAndUnrecognizedProperties.getRight());
diff --git 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
index 3daa463b54..5d5030a4e6 100644
--- 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
+++ 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
@@ -152,7 +152,6 @@ public class PinotTableRestletResource {
 
   @Inject
   AccessControlFactory _accessControlFactory;
-  AccessControlUtils _accessControlUtils = new AccessControlUtils();
 
   @Inject
   Executor _executor;
@@ -186,7 +185,7 @@ public class PinotTableRestletResource {
       // validate permission
       tableName = tableConfig.getTableName();
       String endpointUrl = request.getRequestURL().toString();
-      _accessControlUtils.validatePermission(tableName, AccessType.CREATE, 
httpHeaders, endpointUrl,
+      AccessControlUtils.validatePermission(tableName, AccessType.CREATE, 
httpHeaders, endpointUrl,
           _accessControlFactory.create());
 
       Schema schema = 
_pinotHelixResourceManager.getSchemaForTableConfig(tableConfig);
@@ -372,7 +371,7 @@ public class PinotTableRestletResource {
 
       // validate if user has permission to change the table state
       String endpointUrl = request.getRequestURL().toString();
-      _accessControlUtils
+      AccessControlUtils
           .validatePermission(tableName, AccessType.UPDATE, httpHeaders, 
endpointUrl, _accessControlFactory.create());
 
       ArrayNode ret = JsonUtils.newArrayNode();
diff --git 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java
 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java
index 75c5e930c7..d238756783 100644
--- 
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java
+++ 
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java
@@ -99,7 +99,6 @@ public class TableConfigsRestletResource {
 
   @Inject
   AccessControlFactory _accessControlFactory;
-  AccessControlUtils _accessControlUtils = new AccessControlUtils();
 
   /**
    * List all {@link TableConfigs}, where each is a group of the offline table 
config, realtime table config and
@@ -193,18 +192,18 @@ public class TableConfigsRestletResource {
     try {
       String endpointUrl = request.getRequestURL().toString();
       AccessControl accessControl = _accessControlFactory.create();
-      _accessControlUtils
+      AccessControlUtils
           .validatePermission(schema.getSchemaName(), AccessType.CREATE, 
httpHeaders, endpointUrl, accessControl);
 
       if (offlineTableConfig != null) {
         tuneConfig(offlineTableConfig, schema);
-        _accessControlUtils
+        AccessControlUtils
             .validatePermission(offlineTableConfig.getTableName(), 
AccessType.CREATE, httpHeaders, endpointUrl,
                 accessControl);
       }
       if (realtimeTableConfig != null) {
         tuneConfig(realtimeTableConfig, schema);
-        _accessControlUtils
+        AccessControlUtils
             .validatePermission(realtimeTableConfig.getTableName(), 
AccessType.CREATE, httpHeaders, endpointUrl,
                 accessControl);
       }
diff --git 
a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/AuthenticationFilterTest.java
 
b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/AuthenticationFilterTest.java
index 45c8f8daa2..36ba895b79 100644
--- 
a/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/AuthenticationFilterTest.java
+++ 
b/pinot-controller/src/test/java/org/apache/pinot/controller/api/access/AuthenticationFilterTest.java
@@ -20,7 +20,6 @@
 package org.apache.pinot.controller.api.access;
 
 import java.lang.reflect.Method;
-import java.util.Optional;
 import javax.ws.rs.DELETE;
 import javax.ws.rs.GET;
 import javax.ws.rs.POST;
@@ -30,6 +29,7 @@ import javax.ws.rs.core.MultivaluedMap;
 import org.testng.annotations.Test;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
 
 
 public class AuthenticationFilterTest {
@@ -45,8 +45,7 @@ public class AuthenticationFilterTest {
     queryParams.putSingle("tableName", "D");
     queryParams.putSingle("tableNameWithType", "E");
     queryParams.putSingle("schemaName", "F");
-    Optional<String> actual = _authFilter.extractTableName(pathParams, 
queryParams);
-    assertEquals(actual, Optional.of("A"));
+    assertEquals(AuthenticationFilter.extractTableName(pathParams, 
queryParams), "A");
   }
 
   @Test
@@ -58,8 +57,7 @@ public class AuthenticationFilterTest {
     queryParams.putSingle("tableName", "D");
     queryParams.putSingle("tableNameWithType", "E");
     queryParams.putSingle("schemaName", "F");
-    Optional<String> actual = _authFilter.extractTableName(pathParams, 
queryParams);
-    assertEquals(actual, Optional.of("B"));
+    assertEquals(AuthenticationFilter.extractTableName(pathParams, 
queryParams), "B");
   }
 
   @Test
@@ -70,8 +68,7 @@ public class AuthenticationFilterTest {
     queryParams.putSingle("tableName", "D");
     queryParams.putSingle("tableNameWithType", "E");
     queryParams.putSingle("schemaName", "F");
-    Optional<String> actual = _authFilter.extractTableName(pathParams, 
queryParams);
-    assertEquals(actual, Optional.of("C"));
+    assertEquals(AuthenticationFilter.extractTableName(pathParams, 
queryParams), "C");
   }
 
   @Test
@@ -81,8 +78,7 @@ public class AuthenticationFilterTest {
     queryParams.putSingle("tableName", "D");
     queryParams.putSingle("tableNameWithType", "E");
     queryParams.putSingle("schemaName", "F");
-    Optional<String> actual = _authFilter.extractTableName(pathParams, 
queryParams);
-    assertEquals(actual, Optional.of("D"));
+    assertEquals(AuthenticationFilter.extractTableName(pathParams, 
queryParams), "D");
   }
 
   @Test
@@ -91,8 +87,7 @@ public class AuthenticationFilterTest {
     MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
     queryParams.putSingle("tableNameWithType", "E");
     queryParams.putSingle("schemaName", "F");
-    Optional<String> actual = _authFilter.extractTableName(pathParams, 
queryParams);
-    assertEquals(actual, Optional.of("E"));
+    assertEquals(AuthenticationFilter.extractTableName(pathParams, 
queryParams), "E");
   }
 
   @Test
@@ -100,16 +95,14 @@ public class AuthenticationFilterTest {
     MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
     MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
     queryParams.putSingle("schemaName", "F");
-    Optional<String> actual = _authFilter.extractTableName(pathParams, 
queryParams);
-    assertEquals(actual, Optional.of("F"));
+    assertEquals(AuthenticationFilter.extractTableName(pathParams, 
queryParams), "F");
   }
 
   @Test
   public void testExtractTableNameWithEmptyParams() {
     MultivaluedMap<String, String> pathParams = new MultivaluedHashMap<>();
     MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
-    Optional<String> actual = _authFilter.extractTableName(pathParams, 
queryParams);
-    assertEquals(actual, Optional.empty());
+    assertNull(AuthenticationFilter.extractTableName(pathParams, queryParams));
   }
 
   @Test
diff --git 
a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AddTableCommand.java
 
b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AddTableCommand.java
index 8792f960f4..dba0a87180 100644
--- 
a/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AddTableCommand.java
+++ 
b/pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AddTableCommand.java
@@ -21,14 +21,16 @@ package org.apache.pinot.tools.admin.command;
 import com.fasterxml.jackson.databind.JsonNode;
 import java.io.File;
 import java.io.IOException;
-import java.util.Collections;
-import org.apache.pinot.common.utils.FileUploadDownloadClient;
+import java.util.concurrent.Callable;
 import org.apache.pinot.spi.auth.AuthProvider;
+import org.apache.pinot.spi.config.TableConfigs;
+import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.data.Schema;
 import org.apache.pinot.spi.utils.CommonConstants;
 import org.apache.pinot.spi.utils.JsonUtils;
 import org.apache.pinot.spi.utils.NetUtils;
 import org.apache.pinot.spi.utils.builder.ControllerRequestURLBuilder;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
 import org.apache.pinot.tools.Command;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -157,32 +159,10 @@ public class AddTableCommand extends 
AbstractBaseAdminCommand implements Command
     return this;
   }
 
-  public void uploadSchema()
-      throws Exception {
-    File schemaFile;
-    Schema schema;
-    try {
-      schemaFile = new File(_schemaFile);
-      schema = Schema.fromFile(schemaFile);
-    } catch (Exception e) {
-      LOGGER.error("Got exception while reading Pinot schema from file: [" + 
_schemaFile + "]");
-      throw e;
-    }
-    try (FileUploadDownloadClient fileUploadDownloadClient = new 
FileUploadDownloadClient()) {
-      fileUploadDownloadClient.addSchema(FileUploadDownloadClient
-              .getUploadSchemaURI(_controllerProtocol, _controllerHost, 
Integer.parseInt(_controllerPort)),
-          schema.getSchemaName(), schemaFile, 
makeAuthHeaders(makeAuthProvider(_authProvider, _authTokenUrl, _authToken,
-                  _user, _password)), Collections.emptyList());
-    } catch (Exception e) {
-      LOGGER.error("Got Exception to upload Pinot Schema: " + 
schema.getSchemaName(), e);
-      throw e;
-    }
-  }
-
   public boolean sendTableCreationRequest(JsonNode node)
       throws IOException {
     String res = AbstractBaseAdminCommand
-        .sendRequest("POST", 
ControllerRequestURLBuilder.baseUrl(_controllerAddress).forTableCreate(), 
node.toString(),
+        .sendRequest("POST", 
ControllerRequestURLBuilder.baseUrl(_controllerAddress).forTableConfigsCreate(),
 node.toString(),
             makeAuthHeaders(makeAuthProvider(_authProvider, _authTokenUrl, 
_authToken, _user, _password)));
     LOGGER.info(res);
     return res.contains("successfully added");
@@ -204,10 +184,24 @@ public class AddTableCommand extends 
AbstractBaseAdminCommand implements Command
 
     LOGGER.info("Executing command: " + toString());
 
-    // Backward compatible
-    if (_schemaFile != null) {
-      uploadSchema();
+    TableConfig tableConfig = attempt(() -> JsonUtils.fileToObject(new 
File(_tableConfigFile), TableConfig.class),
+        "Failed reading table config " + _tableConfigFile);
+
+    Schema schema = attempt(() -> JsonUtils.fileToObject(new 
File(_schemaFile), Schema.class),
+        "Failed reading schema " + _schemaFile);
+
+    String tableName = 
TableNameBuilder.extractRawTableName(tableConfig.getTableName());
+    TableConfigs tableConfigs =  new TableConfigs(tableName, schema, 
tableConfig, null);
+
+    return sendTableCreationRequest(JsonUtils.objectToJsonNode(tableConfigs));
+  }
+
+  private static <T> T attempt(Callable<T> callable, String errorMessage) {
+    try {
+      return callable.call();
+    } catch (Throwable t) {
+      LOGGER.error(errorMessage, t);
+      throw new IllegalStateException(t);
     }
-    return sendTableCreationRequest(JsonUtils.fileToJsonNode(new 
File(_tableConfigFile)));
   }
 }


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Reply via email to