Skip to content

Commit

Permalink
[#2471] refactor: Enable InlineFormatString error-prone (#2488)
Browse files Browse the repository at this point in the history 
### What changes were proposed in this pull request?
Enable InlineFormatString error-prone check.

### Why are the changes needed?

Fix: #2471

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Existing integration tests.
  • Loading branch information
@zivali
zivali authored Mar 13, 2024
1 parent ddfc4cf commit b242c8f
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 52 deletions.
3 changes: 2 additions & 1 deletion build.gradle.kts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,8 @@ subprojects {
"VarTypeName",
"XorPower",
"EqualsGetClass",
"DefaultCharset"
"DefaultCharset",
"InlineFormatString"
)
}
}
Expand Down
21 changes: 14 additions & 7 deletions ...e/src/test/java/com/datastrato/gravitino/catalog/hive/integration/test/CatalogHiveIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,18 @@ public class CatalogHiveIT extends AbstractIT {
private static SparkSession sparkSession;
private static FileSystem hdfs;
private static final String SELECT_ALL_TEMPLATE = "SELECT * FROM %s.%s";
private static final String INSERT_WITHOUT_PARTITION_TEMPLATE = "INSERT INTO %s.%s VALUES (%s)";
private static final String INSERT_WITH_PARTITION_TEMPLATE =
"INSERT INTO %s.%s PARTITION (%s) VALUES (%s)";

private static String getInsertWithoutPartitionSql(
String dbName, String tableName, String values) {
return String.format("INSERT INTO %s.%s VALUES (%s)", dbName, tableName, values);
}

private static String getInsertWithPartitionSql(
String dbName, String tableName, String partitionExpressions, String values) {
return String.format(
"INSERT INTO %s.%s PARTITION (%s) VALUES (%s)",
dbName, tableName, partitionExpressions, values);
}

private static final Map<String, String> typeConstant =
ImmutableMap.of(
Expand Down Expand Up @@ -292,15 +301,13 @@ private void checkTableReadWrite(org.apache.hadoop.hive.metastore.api.Table tabl
.map(Object::toString)
.collect(Collectors.joining(","));
if (table.getPartitionKeys().isEmpty()) {
sparkSession.sql(String.format(INSERT_WITHOUT_PARTITION_TEMPLATE, dbName, tableName, values));
sparkSession.sql(getInsertWithoutPartitionSql(dbName, tableName, values));
} else {
String partitionExpressions =
table.getPartitionKeys().stream()
.map(f -> f.getName() + "=" + typeConstant.get(f.getType()))
.collect(Collectors.joining(","));
sparkSession.sql(
String.format(
INSERT_WITH_PARTITION_TEMPLATE, dbName, tableName, partitionExpressions, values));
sparkSession.sql(getInsertWithPartitionSql(dbName, tableName, partitionExpressions, values));
}
Assertions.assertEquals(
count + 1, sparkSession.sql(String.format(SELECT_ALL_TEMPLATE, dbName, tableName)).count());
Expand Down
5 changes: 1 addition & 4 deletions ...ava/com/datastrato/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,6 @@ public class PostgreSqlSchemaOperations extends JdbcDatabaseOperations {
}
});

private static final String GET_SCHEMA_COMMENT_SQL_FORMAT =
"SELECT obj_description('%s'::regnamespace) as comment";

private String database;

@Override
Expand Down Expand Up @@ -141,7 +138,7 @@ protected boolean isSystemDatabase(String dbName) {
}

private String getShowSchemaCommentSql(String schema) {
return String.format(GET_SCHEMA_COMMENT_SQL_FORMAT, schema);
return String.format("SELECT obj_description('%s'::regnamespace) as comment", schema);
}

private String getSchemaComment(String schema, Connection connection) throws SQLException {
Expand Down
31 changes: 25 additions & 6 deletions common/src/test/java/com/datastrato/gravitino/json/TestDTOJsonSerDe.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,28 @@ public class TestDTOJsonSerDe {
private final String columnJson =
"{\"name\":%s,\"type\":%s,\"comment\":%s,\"nullable\":%s,\"autoIncrement\":%s}";

private final String tableJson =
"{\"name\":%s,\"comment\":%s,\"columns\":[%s],\"properties\":%s,\"audit\":%s,\"distribution\":%s,\"sortOrders\":%s,\"partitioning\":%s,\"indexes\":%s}";
private String getExpectedTableJson(
String tableName,
String tableComment,
String columns,
String properties,
String audit,
String distribution,
String sortOrders,
String partitioning,
String indexes) {
return String.format(
"{\"name\":%s,\"comment\":%s,\"columns\":[%s],\"properties\":%s,\"audit\":%s,\"distribution\":%s,\"sortOrders\":%s,\"partitioning\":%s,\"indexes\":%s}",
withQuotes(tableName),
withQuotes(tableComment),
columns,
properties,
audit,
distribution,
sortOrders,
partitioning,
indexes);
}

private String withQuotes(String str) {
return "\"" + str + "\"";
Expand Down Expand Up @@ -259,10 +279,9 @@ public void testTableDTOSerDe() throws Exception {

String serJson = JsonUtils.objectMapper().writeValueAsString(table);
String expectedJson =
String.format(
tableJson,
withQuotes(tableName),
withQuotes(tableComment),
getExpectedTableJson(
tableName,
tableComment,
String.format(
columnJson,
withQuotes(name),
Expand Down
12 changes: 8 additions & 4 deletions integration-test/src/test/java/com/datastrato/gravitino/integration/test/spark/SparkIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,13 @@
@Tag("gravitino-docker-it")
@TestInstance(Lifecycle.PER_CLASS)
public class SparkIT extends SparkEnvIT {
private static String getSelectAllSql(String tableName) {
return String.format("SELECT * FROM %s", tableName);
}

private static final String SELECT_ALL_TEMPLATE = "SELECT * FROM %s";
private static final String INSERT_WITHOUT_PARTITION_TEMPLATE = "INSERT INTO %s VALUES (%s)";
private static String getInsertWithoutPartitionSql(String tableName, String values) {
return String.format("INSERT INTO %s VALUES (%s)", tableName, values);
}

// To generate test data for write&read table.
private static final Map<DataType, String> typeConstant =
Expand Down Expand Up @@ -352,7 +356,7 @@ private void checkTableReadWrite(SparkTableInfo table) {
.map(Object::toString)
.collect(Collectors.joining(","));

sql(String.format(INSERT_WITHOUT_PARTITION_TEMPLATE, name, insertValues));
sql(getInsertWithoutPartitionSql(name, insertValues));

// remove "'" from values, such as 'a' is trans to a
String checkValues =
Expand All @@ -368,7 +372,7 @@ private void checkTableReadWrite(SparkTableInfo table) {
.collect(Collectors.joining(","));

List<String> queryResult =
sql(String.format(SELECT_ALL_TEMPLATE, name)).stream()
sql(getSelectAllSql(name)).stream()
.map(
line ->
Arrays.stream(line)
Expand Down
8 changes: 5 additions & 3 deletions server/src/main/java/com/datastrato/gravitino/server/web/VersioningFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,10 @@ public Enumeration<String> getHeaderNames() {
private static final Pattern ACCEPT_VERSION_REGEX =
Pattern.compile("application/vnd\\.gravitino\\.v(\\d+)\\+json");
private static final String ACCEPT_VERSION_HEADER = "Accept";
private static final String ACCEPT_VERSION = "application/vnd.gravitino.v%d+json";

private static String getAcceptVersion(int version) {
return String.format("application/vnd.gravitino.v%d+json", version);
}

@Override
public void init(FilterConfig filterConfig) throws ServletException {}
Expand Down Expand Up @@ -101,8 +104,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
// If no version accept header not is set, then we need to set the latest version.
MutableHttpServletRequest mutableRequest = new MutableHttpServletRequest(req);
ApiVersion latest = ApiVersion.latestVersion();
mutableRequest.putHeader(
ACCEPT_VERSION_HEADER, String.format(ACCEPT_VERSION, latest.version()));
mutableRequest.putHeader(ACCEPT_VERSION_HEADER, getAcceptVersion(latest.version()));

chain.doFilter(mutableRequest, response);
}
Expand Down
75 changes: 48 additions & 27 deletions server/src/main/java/com/datastrato/gravitino/server/web/rest/ExceptionHandlers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,17 @@ private static class PartitionExceptionHandler extends BaseExceptionHandler {

private static final ExceptionHandler INSTANCE = new PartitionExceptionHandler();

private static final String PARTITION_MSG_TEMPLATE =
"Failed to operate partition(s)%s operation [%s] of table [%s], reason [%s]";
private static String getPartitionErrorMsg(
String partition, String operation, String table, String reason) {
return String.format(
"Failed to operate partition(s)%s operation [%s] of table [%s], reason [%s]",
partition, operation, table, reason);
}

@Override
public Response handle(OperationType op, String partition, String table, Exception e) {
String formatted = StringUtil.isBlank(partition) ? "" : " [" + partition + "]";
String errorMsg =
String.format(PARTITION_MSG_TEMPLATE, formatted, op.name(), table, getErrorMsg(e));
String errorMsg = getPartitionErrorMsg(formatted, op.name(), table, getErrorMsg(e));
LOG.warn(errorMsg, e);

if (e instanceof IllegalArgumentException) {
Expand All @@ -91,14 +94,17 @@ private static class TableExceptionHandler extends BaseExceptionHandler {

private static final ExceptionHandler INSTANCE = new TableExceptionHandler();

private static final String TABLE_MSG_TEMPLATE =
"Failed to operate table(s)%s operation [%s] under schema [%s], reason [%s]";
private static String getTableErrorMsg(
String table, String operation, String schema, String reason) {
return String.format(
"Failed to operate table(s)%s operation [%s] under schema [%s], reason [%s]",
table, operation, schema, reason);
}

@Override
public Response handle(OperationType op, String table, String schema, Exception e) {
String formatted = StringUtil.isBlank(table) ? "" : " [" + table + "]";
String errorMsg =
String.format(TABLE_MSG_TEMPLATE, formatted, op.name(), schema, getErrorMsg(e));
String errorMsg = getTableErrorMsg(formatted, op.name(), schema, getErrorMsg(e));
LOG.warn(errorMsg, e);

if (e instanceof IllegalArgumentException) {
Expand All @@ -123,14 +129,17 @@ private static class SchemaExceptionHandler extends BaseExceptionHandler {

private static final ExceptionHandler INSTANCE = new SchemaExceptionHandler();

private static final String SCHEMA_MSG_TEMPLATE =
"Failed to operate schema(s)%s operation [%s] under catalog [%s], reason [%s]";
private static String getSchemaErrorMsg(
String schema, String operation, String catalog, String reason) {
return String.format(
"Failed to operate schema(s)%s operation [%s] under catalog [%s], reason [%s]",
schema, operation, catalog, reason);
}

@Override
public Response handle(OperationType op, String schema, String catalog, Exception e) {
String formatted = StringUtil.isBlank(schema) ? "" : " [" + schema + "]";
String errorMsg =
String.format(SCHEMA_MSG_TEMPLATE, formatted, op.name(), catalog, getErrorMsg(e));
String errorMsg = getSchemaErrorMsg(formatted, op.name(), catalog, getErrorMsg(e));
LOG.warn(errorMsg, e);

if (e instanceof IllegalArgumentException) {
Expand All @@ -155,14 +164,17 @@ private static class CatalogExceptionHandler extends BaseExceptionHandler {

private static final ExceptionHandler INSTANCE = new CatalogExceptionHandler();

private static final String CATALOG_MSG_TEMPLATE =
"Failed to operate catalog(s)%s operation [%s] under metalake [%s], reason [%s]";
private static String getCatalogErrorMsg(
String catalog, String operation, String metalake, String reason) {
return String.format(
"Failed to operate catalog(s)%s operation [%s] under metalake [%s], reason [%s]",
catalog, operation, metalake, reason);
}

@Override
public Response handle(OperationType op, String catalog, String metalake, Exception e) {
String formatted = StringUtil.isBlank(catalog) ? "" : " [" + catalog + "]";
String errorMsg =
String.format(CATALOG_MSG_TEMPLATE, formatted, op.name(), metalake, getErrorMsg(e));
String errorMsg = getCatalogErrorMsg(formatted, op.name(), metalake, getErrorMsg(e));
LOG.warn(errorMsg, e);

if (e instanceof IllegalArgumentException) {
Expand All @@ -184,13 +196,16 @@ private static class MetalakeExceptionHandler extends BaseExceptionHandler {

private static final ExceptionHandler INSTANCE = new MetalakeExceptionHandler();

private static final String METALAKE_MSG_TEMPLATE =
"Failed to operate metalake(s)%s operation [%s], reason [%s]";
private static String getMetalakeErrorMsg(String metalake, String operation, String reason) {
return String.format(
"Failed to operate metalake(s)%s operation [%s], reason [%s]",
metalake, operation, reason);
}

@Override
public Response handle(OperationType op, String metalake, String parent, Exception e) {
String formatted = StringUtil.isBlank(metalake) ? "" : " [" + metalake + "]";
String errorMsg = String.format(METALAKE_MSG_TEMPLATE, formatted, op.name(), getErrorMsg(e));
String errorMsg = getMetalakeErrorMsg(formatted, op.name(), getErrorMsg(e));
LOG.warn(errorMsg, e);

if (e instanceof IllegalArgumentException) {
Expand All @@ -211,14 +226,17 @@ public Response handle(OperationType op, String metalake, String parent, Excepti
private static class FilesetExceptionHandler extends BaseExceptionHandler {
private static final ExceptionHandler INSTANCE = new FilesetExceptionHandler();

private static final String FILESET_MSG_TEMPLATE =
"Failed to operate fileset(s)%s operation [%s] under schema [%s], reason [%s]";
private static String getFilesetErrorMsg(
String fileset, String operation, String schema, String reason) {
return String.format(
"Failed to operate fileset(s)%s operation [%s] under schema [%s], reason [%s]",
fileset, operation, schema, reason);
}

@Override
public Response handle(OperationType op, String fileset, String schema, Exception e) {
String formatted = StringUtil.isBlank(fileset) ? "" : " [" + fileset + "]";
String errorMsg =
String.format(FILESET_MSG_TEMPLATE, formatted, op.name(), schema, getErrorMsg(e));
String errorMsg = getFilesetErrorMsg(formatted, op.name(), schema, getErrorMsg(e));
LOG.warn(errorMsg, e);

if (e instanceof IllegalArgumentException) {
Expand All @@ -243,17 +261,20 @@ static class BaseExceptionHandler extends ExceptionHandler {

private static final ExceptionHandler INSTANCE = new BaseExceptionHandler();

private static final String BASE_MSG_TEMPLATE =
"Failed to operate object%s operation [%s]%s, reason [%s]";
private static String getBaseErrorMsg(
String object, String operation, String parent, String reason) {
return String.format(
"Failed to operate object%s operation [%s]%s, reason [%s]",
object, operation, parent, reason);
}

@Override
public Response handle(OperationType op, String object, String parent, Exception e) {
String formattedObject = StringUtil.isBlank(object) ? "" : " [" + object + "]";
String formattedParent = StringUtil.isBlank(parent) ? "" : " under [" + parent + "]";

String errorMsg =
String.format(
BASE_MSG_TEMPLATE, formattedObject, op.name(), formattedParent, getErrorMsg(e));
getBaseErrorMsg(formattedObject, op.name(), formattedParent, getErrorMsg(e));
LOG.error(errorMsg, e);
return Utils.internalError(errorMsg, e);
}
Expand Down

0 comments on commit b242c8f

Please sign in to comment.