[#1745] improvement(api): Add errorprone plugin to format exception message #1746

coolderli · 2024-01-28T04:48:12Z

What changes were proposed in this pull request?

close #1745
Format exception message by String.format

Why are the changes needed?

Format the message of exception for better coding

Fix: #1745

Does this PR introduce any user-facing change?

no

How was this patch tested?

no need, just compile passed is OK.

yuqi1129 · 2024-01-28T06:22:20Z

@coolderli , Thanks for your contribution, can you provide the effection this PRs introduces?

coolderli · 2024-01-28T09:05:22Z

@coolderli , Thanks for your contribution, can you provide the effection this PRs introduces?

@yuqi1129 When concating the message of exception, we have to use + to concat the constants and variables, such as:

      catch (EntityAlreadyExistsException e1) {
      throw new CatalogAlreadyExistsException("Catalog " + ident + " already exists");
    }

In this PR, we can format the message like this:

catch (EntityAlreadyExistsException e1) {
      throw new CatalogAlreadyExistsException("Catalog %s already exists", ident);
    }

We have already done this on RESTException

But the annotation @FormatMethod and @FormatString will not work unless we apply an error-prone plugin to the project.

coolderli · 2024-01-29T03:50:47Z

@yuqi1129 what do you think about this change? It's just a code refactor.

yuqi1129 · 2024-01-29T03:58:30Z

@yuqi1129 what do you think about this change? It's just a code refactor.

Is this https://github.com/tbroyer/gradle-errorprone-plugin error-prone plugin? I'm afraid it won't work well with JDK8 and JDK 17, though I agree with this optimization. Have you been able to test the plugin with JDK8, JDK11 and JDK17?

coolderli · 2024-01-29T08:25:55Z

@yuqi1129 what do you think about this change? It's just a code refactor.

Is this https://github.com/tbroyer/gradle-errorprone-plugin error-prone plugin? I'm afraid it won't work well with JDK8 and JDK 17, though I agree with this optimization. Have you been able to test the plugin with JDK8, JDK11 and JDK17?

@yuqi1129 Yes. I already tried to test this plugin but failed to make it work. I need to try again when I have time. Do we really need this plugin? What about I remove the annotation @FormatMethod and @FormatString and just receive the code refactor?

yuqi1129 · 2024-01-29T08:40:08Z

@yuqi1129 what do you think about this change? It's just a code refactor.

Is this tbroyer/gradle-errorprone-plugin error-prone plugin? I'm afraid it won't work well with JDK8 and JDK 17, though I agree with this optimization. Have you been able to test the plugin with JDK8, JDK11 and JDK17?

@yuqi1129 Yes. I already tried to test this plugin but failed to make it work. I need to try again when I have time. Do we really need this plugin? What about I remove the annotation @FormatMethod and @FormatString and just receive the code refactor?

I believe the optimization is effective and meaningful, but it should work with the plugin. Can you spare some time to test the plugins?

coolderli · 2024-01-29T08:45:42Z

@yuqi1129 what do you think about this change? It's just a code refactor.

Is this tbroyer/gradle-errorprone-plugin error-prone plugin? I'm afraid it won't work well with JDK8 and JDK 17, though I agree with this optimization. Have you been able to test the plugin with JDK8, JDK11 and JDK17?

@yuqi1129 Yes. I already tried to test this plugin but failed to make it work. I need to try again when I have time. Do we really need this plugin? What about I remove the annotation @FormatMethod and @FormatString and just receive the code refactor?

I believe the optimization is effective and meaningful, but it should work with the plugin. Can you spare some time to test the plugins?

Ok, I got it. I will try my best.

jerryshao · 2024-02-01T12:59:45Z

We definitely need this, this is a good improvement. @coolderli can you please continue your work when you have time.

coolderli · 2024-02-07T02:23:58Z

build.gradle.kts

+      options.errorprone.isEnabled.set(true)
+      options.errorprone.disableAllChecks.set(true)
+      options.errorprone.enable(
+        "AnnotateFormatMethod"


Only enable @FormatMethod, because we enable -Werror, Or there are a lot of warnings that are treated as an error.

good improvement！I had the same problem last night.

…tion message

coolderli · 2024-02-07T08:34:33Z

.../src/main/java/com/datastrato/gravitino/catalog/mysql/converter/MysqlExceptionConverter.java

@@ -15,21 +15,22 @@
 /** Exception converter to gravitino exception for MySQL. */
 public class MysqlExceptionConverter extends JdbcExceptionConverter {

+  @SuppressWarnings("FormatStringAnnotation")


The string that is annotationed by @FormatString must be a final string or a compile time constant value. a variable is not allowed, such as

new NoSuchEntityException(se.getMessage)

So I just ignore it by @SuppressWarnings("FormatStringAnnotation")

coolderli · 2024-02-07T08:35:54Z

core/src/main/java/com/datastrato/gravitino/storage/kv/KvEntityStore.java

@@ -287,13 +285,13 @@ public <E extends Entity & HasIdentifier> E get(
                    () -> {
                      byte[] key = entityKeyEncoder.encode(ident, entityType, true);
                      if (key == null) {
-                        throw new NoSuchEntityException(ident.toString());
+                        throw new NoSuchEntityException("No such entity:%s", ident.toString());


Another fix is like this. We use the format string.

coolderli · 2024-02-07T08:36:41Z

...atastrato/gravitino/integration/test/catalog/lakehouse/iceberg/IcebergRESTServiceBaseIT.java

@@ -34,6 +35,7 @@
 * <p>Referred from spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
 */

+@SuppressWarnings("FormatStringAnnotation")


Too many changes, just ignore them.

coolderli · 2024-02-07T08:40:39Z

api/src/main/java/com/datastrato/gravitino/exceptions/GravitinoRuntimeException.java

-  public GravitinoRuntimeException(String message, Throwable cause) {
-    super(message, cause);
+  @FormatMethod
+  public GravitinoRuntimeException(Throwable cause, @FormatString String message, Object... args) {


The Throwable must be the first argument. Or it will throw an exception. I think it is conflict with the variable parameters in the overloaded function

@FormatMethod public GravitinoRuntimeException(@FormatString String message, Object... args) { super(String.format(message, args)); }

coolderli · 2024-02-07T08:51:22Z

...atastrato/gravitino/integration/test/catalog/lakehouse/iceberg/IcebergRESTServiceBaseIT.java

@@ -115,6 +117,7 @@ private void stopSparkEnv() {
    }
  }

+  @FormatMethod


If a method uses string.format(), it must be annotated by @FormatMethod, or there will be an error.

coolderli · 2024-02-07T08:55:41Z

If there are some format problems, some errors will be thrown like below:

/home/mi/IdeaProjects/gravitino/core/src/test/java/com/datastrato/gravitino/TestEntityStore.java:95: 错误: [FormatStringAnnotation] Format strings must be either literals or variables. Other expressions are not valid.
              throw new NoSuchEntityException("Entity " + ident + " does not exist");
                    ^
  Invalid format string: "Entity " + ident + " does not exist"
    (see https://errorprone.info/bugpattern/FormatStringAnnotation)
/home/mi/IdeaProjects/gravitino/core/src/test/java/com/datastrato/gravitino/TestEntityStore.java:114: 错误: [FormatStringAnnotation] Format strings must be either literals or variables. Other expressions are not valid.
        throw new NoSuchEntityException("Entity " + ident + " does not exist");

/home/mi/IdeaProjects/gravitino/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveTableOperations.java:93: 错误: [FormatStringAnnotation] extra format arguments: used 1, provided 2
      throw new NoSuchTableException(
            ^
    (see https://errorprone.info/bugpattern/FormatStringAnnotation)
/home/mi/IdeaProjects/gravitino/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveTableOperations.java:97: 错误: [FormatStringAnnotation] extra format arguments: used 1, provided 2
      throw new NoSuchPartitionException(

coolderli · 2024-02-07T08:56:43Z

@jerryshao @yuqi1129 Finally, I finished this. Please take a review when you have time. Thanks.

yuqi1129 · 2024-02-07T09:12:14Z

@jerryshao @yuqi1129 Finally, I finished this. Please take a review when you have time. Thanks.

Thank you for your hard work, we will review it soon.

justinmclean · 2024-02-08T06:23:18Z

If it helps (and I've not gone through them all) these checks that currently pass:

      "AlwaysThrows",
        "AnnotateFormatMethod",
        "ArrayEquals",
        "ArrayToString",
        "ArraysAsListPrimitiveArray",
        "ArrayFillIncompatibleType",
        "BoxedPrimitiveEquality",
        "ChainingConstructorIgnoresParameter",
        "CheckNotNullMultipleTimes",
        "CheckReturnValue",
        "CollectionIncompatibleType",
        "CollectionToArraySafeParameter",
        "ComparingThisWithNull",
        "ComparisonOutOfRange",
        "CompatibleWithAnnotationMisuse",
        "CompileTimeConstant",
        "ConditionalExpressionNumericPromotion",
        "ConstantOverflow",
        "DangerousLiteralNull",
        "DeadException",
        "DeadThread",
        "DoNotCall",
        "DoNotMock",
        "DuplicateMapKeys",
        "EqualsNaN",
        "EqualsNull",
        "EqualsReference",
        "EqualsWrongThing",
        "ForOverride",
        "FormatString",
        "FormatStringAnnotation",
        "GetClassOnAnnotation",
        "GetClassOnClass",
        "HashtableContains",
        "IdentityBinaryExpression",
        "IdentityHashMapBoxing",
        "Immutable",
        "Incomparable",
        "IncompatibleArgumentType",
        "IndexOfChar",
        "InfiniteRecursion",
        "InvalidJavaTimeConstant",
        "InvalidPatternSyntax",
        "IsInstanceIncompatibleType",
        "JUnit4ClassAnnotationNonStatic",
        "JUnit4SetUpNotRun",
        "JUnit4TearDownNotRun",
        "JUnit4TestNotRun",
        "JUnitAssertSameCheck",
        "LockOnBoxedPrimitive",
        "LoopConditionChecker",
        "LossyPrimitiveCompare",
        "MathRoundIntLong",
        "MissingSuperCall",
        "ModifyingCollectionWithItself",
        "NonCanonicalStaticImport",
        "NonFinalCompileTimeConstant",
        "NonRuntimeAnnotation",
        "NullTernary",
        "OptionalEquality",
        "PackageInfo",
        "ParametersButNotParameterized",
        "RandomCast",
        "RandomModInteger",
        "SelfAssignment",
        "SelfComparison",
        "SelfEquals",
        "SizeGreaterThanOrEqualsZero",
        "StreamToString",
        "StringBuilderInitWithChar",
        "SubstringOfZero",
        "ThrowNull",
        "TruthSelfEquals",
        "TryFailThrowable",
        "TypeParameterQualifier",
        "UnnecessaryCheckNotNull",
        "UnnecessaryTypeArgument",
        "UnsafeWildcard",
        "UnusedAnonymousClass",
        "UnusedCollectionModifiedInPlace",
        "VarTypeName",
        "XorPower"

So you could enable all of these

And these ones (and probably more) currently fail:

        "ArgumentSelectionDefectChecker",
        "BadImport",
        "ComparableType",
        "DefaultCharset",
        "DoubleBraceInitialization",
        "EqualsGetClass",
        "EmptyBlockTag",
        "InconsistentCapitalization",
        "JavaUtilDate",
        "MissingOverride",
        "MissingSummary",
        "NonOverridingEquals",
	"ReturnValueIgnored",
        "SameNameButDifferent",
        "StringSplitter",
	"ThrowIfUncheckedKnownChecked",
        "UnnecessaryParentheses",
        "UnusedVariable"

Which we can raise as separate issues.

coolderli · 2024-02-09T23:51:56Z

If it helps (and I've not gone through them all) these checks that currently pass:

      "AlwaysThrows",
        "AnnotateFormatMethod",
        "ArrayEquals",
        "ArrayToString",
        "ArraysAsListPrimitiveArray",
        "ArrayFillIncompatibleType",
        "BoxedPrimitiveEquality",
        "ChainingConstructorIgnoresParameter",
        "CheckNotNullMultipleTimes",
        "CheckReturnValue",
        "CollectionIncompatibleType",
        "CollectionToArraySafeParameter",
        "ComparingThisWithNull",
        "ComparisonOutOfRange",
        "CompatibleWithAnnotationMisuse",
        "CompileTimeConstant",
        "ConditionalExpressionNumericPromotion",
        "ConstantOverflow",
        "DangerousLiteralNull",
        "DeadException",
        "DeadThread",
        "DoNotCall",
        "DoNotMock",
        "DuplicateMapKeys",
        "EqualsNaN",
        "EqualsNull",
        "EqualsReference",
        "EqualsWrongThing",
        "ForOverride",
        "FormatString",
        "FormatStringAnnotation",
        "GetClassOnAnnotation",
        "GetClassOnClass",
        "HashtableContains",
        "IdentityBinaryExpression",
        "IdentityHashMapBoxing",
        "Immutable",
        "Incomparable",
        "IncompatibleArgumentType",
        "IndexOfChar",
        "InfiniteRecursion",
        "InvalidJavaTimeConstant",
        "InvalidPatternSyntax",
        "IsInstanceIncompatibleType",
        "JUnit4ClassAnnotationNonStatic",
        "JUnit4SetUpNotRun",
        "JUnit4TearDownNotRun",
        "JUnit4TestNotRun",
        "JUnitAssertSameCheck",
        "LockOnBoxedPrimitive",
        "LoopConditionChecker",
        "LossyPrimitiveCompare",
        "MathRoundIntLong",
        "MissingSuperCall",
        "ModifyingCollectionWithItself",
        "NonCanonicalStaticImport",
        "NonFinalCompileTimeConstant",
        "NonRuntimeAnnotation",
        "NullTernary",
        "OptionalEquality",
        "PackageInfo",
        "ParametersButNotParameterized",
        "RandomCast",
        "RandomModInteger",
        "SelfAssignment",
        "SelfComparison",
        "SelfEquals",
        "SizeGreaterThanOrEqualsZero",
        "StreamToString",
        "StringBuilderInitWithChar",
        "SubstringOfZero",
        "ThrowNull",
        "TruthSelfEquals",
        "TryFailThrowable",
        "TypeParameterQualifier",
        "UnnecessaryCheckNotNull",
        "UnnecessaryTypeArgument",
        "UnsafeWildcard",
        "UnusedAnonymousClass",
        "UnusedCollectionModifiedInPlace",
        "VarTypeName",
        "XorPower"

So you could enable all of these

And these ones (and probably more) currently fail:

        "ArgumentSelectionDefectChecker",
        "BadImport",
        "ComparableType",
        "DefaultCharset",
        "DoubleBraceInitialization",
        "EqualsGetClass",
        "EmptyBlockTag",
        "InconsistentCapitalization",
        "JavaUtilDate",
        "MissingOverride",
        "MissingSummary",
        "NonOverridingEquals",
	"ReturnValueIgnored",
        "SameNameButDifferent",
        "StringSplitter",
	"ThrowIfUncheckedKnownChecked",
        "UnnecessaryParentheses",
        "UnusedVariable"

Which we can raise as separate issues.

Thanks for your reply.Because we enabled -Weeror.If I enabled all check ，I have to fix all of them in this PR. I
am agree with your suggestion ，I think we can fix them in another issue. What dou you think ？

justinmclean · 2024-02-10T00:41:02Z

Yes, we can fix these in another issue. I've already raised some issues for them.

justinmclean

LGTM - thanks for all your work on this

coolderli force-pushed the add-exception-format branch from 8f31b5a to 3cc2761 Compare January 28, 2024 04:58

coolderli closed this Jan 28, 2024

coolderli reopened this Jan 28, 2024

coolderli marked this pull request as draft January 29, 2024 10:27

coolderli force-pushed the add-exception-format branch from 86bed67 to 2a8ae96 Compare February 6, 2024 08:54

coolderli changed the title ~~[#1745] improvement(api): Format exception message by String.format~~ [#1745] improvement(api): Add errorprone plugin to format exception message Feb 6, 2024

coolderli commented Feb 7, 2024

View reviewed changes

coolderli added 2 commits February 7, 2024 16:18

[apache#1745] improvement(api): Add errorprone plugin to Format excep…

bb2ae53

…tion message

rebase and fix

36b8c68

coolderli force-pushed the add-exception-format branch from 67877e7 to 36b8c68 Compare February 7, 2024 08:26

coolderli commented Feb 7, 2024

View reviewed changes

coolderli marked this pull request as ready for review February 7, 2024 08:56

justinmclean approved these changes Feb 10, 2024

View reviewed changes

This was referenced Feb 12, 2024

[Improvement] Improper String Formatting in TrinoITContainer.java #2174

Closed

[FEATURE] Use git hooks to auto apply or check for common issues before commit or push #2175

Open

justinmclean merged commit 3617a79 into apache:main Feb 16, 2024
12 checks passed

This was referenced Feb 17, 2024

[EPIC] enable all error-prone check #2225

Open

[Subtask] Improvement(build) : enable some safe error-prone checks #2273

Closed

[Improvement] In StorageLayoutException.java and StorageLayoutException.java no passing final variable in format string #2152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1745] improvement(api): Add errorprone plugin to format exception message #1746

[#1745] improvement(api): Add errorprone plugin to format exception message #1746

coolderli commented Jan 28, 2024

yuqi1129 commented Jan 28, 2024

coolderli commented Jan 28, 2024 •

edited

Loading

coolderli commented Jan 29, 2024

yuqi1129 commented Jan 29, 2024

coolderli commented Jan 29, 2024 •

edited

Loading

yuqi1129 commented Jan 29, 2024

coolderli commented Jan 29, 2024

jerryshao commented Feb 1, 2024

coolderli Feb 7, 2024

YxAc Feb 10, 2024

coolderli Feb 7, 2024

coolderli Feb 7, 2024

coolderli Feb 7, 2024

coolderli Feb 7, 2024

coolderli Feb 7, 2024

coolderli commented Feb 7, 2024

coolderli commented Feb 7, 2024

yuqi1129 commented Feb 7, 2024

justinmclean commented Feb 8, 2024

coolderli commented Feb 9, 2024

justinmclean commented Feb 10, 2024

justinmclean left a comment

[#1745] improvement(api): Add errorprone plugin to format exception message #1746

[#1745] improvement(api): Add errorprone plugin to format exception message #1746

Conversation

coolderli commented Jan 28, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

yuqi1129 commented Jan 28, 2024

coolderli commented Jan 28, 2024 • edited Loading

coolderli commented Jan 29, 2024

yuqi1129 commented Jan 29, 2024

coolderli commented Jan 29, 2024 • edited Loading

yuqi1129 commented Jan 29, 2024

coolderli commented Jan 29, 2024

jerryshao commented Feb 1, 2024

coolderli Feb 7, 2024

Choose a reason for hiding this comment

YxAc Feb 10, 2024

Choose a reason for hiding this comment

coolderli Feb 7, 2024

Choose a reason for hiding this comment

coolderli Feb 7, 2024

Choose a reason for hiding this comment

coolderli Feb 7, 2024

Choose a reason for hiding this comment

coolderli Feb 7, 2024

Choose a reason for hiding this comment

coolderli Feb 7, 2024

Choose a reason for hiding this comment

coolderli commented Feb 7, 2024

coolderli commented Feb 7, 2024

yuqi1129 commented Feb 7, 2024

justinmclean commented Feb 8, 2024

coolderli commented Feb 9, 2024

justinmclean commented Feb 10, 2024

justinmclean left a comment

Choose a reason for hiding this comment

coolderli commented Jan 28, 2024 •

edited

Loading

coolderli commented Jan 29, 2024 •

edited

Loading