Perform additional validation & fail earlier for incorrect arguments #87

Marcono1234 · 2024-09-26T14:34:08Z

Before this PR

Builder methods were not consistently checking for null
This would probably often have lead to delayed NullPointerExceptions, or might have even caused different behavior in case null had internally a special meaning.
Thrown exception types were not consistent
Sometimes IllegalArgumentException was thrown for null; in two cases UnsupportedOperationException was thrown instead of IllegalStateException.
Validation was missing for adding record constructors
Some validation only happened during build(), and not already when calling builder methods
Note: I only changed a few cases; maybe would be worth to investigate this more in depth in the future. But this applies mainly to immutable state of a builder, e.g. a TypeSpec.Builder for class will keep that kind and it cannot be changed to enum. For other validation is probably fine and better to do this in build().

After this PR

NullPointerException is thrown for unexpected null
IllegalStateException is thrown when methods are called on wrong type, e.g. adding record constructor to regular class
Exceptions are thrown for incorrect record constructor usage
Added addModifier(Iterable) overloads, see Fix incomplete toBuilder implementations and add tests #85 (comment)

Possible downsides?

I hope these changes do not accidentally disallow arguments which should be allowed.

MethodSpec and ParameterSpec already had this.

palantirtech · 2024-09-26T14:34:12Z

Thanks for your interest in palantir/javapoet, @Marcono1234! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

changelog-app · 2024-09-26T14:34:12Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Perform additional validation & fail earlier for incorrect arguments

Check the box to generate changelog(s)

Generate changelog entry

Marcono1234 · 2024-09-26T14:37:04Z

javapoet/src/main/java/com/palantir/javapoet/ClassName.java

+        checkState(enclosingClassName == null || enclosingClassName.packageName.equals(packageName),
+                "enclosing class is in wrong package");


I think this is impossible with the public API currently, but I added this check nonetheless to be safe.

Marcono1234 · 2024-09-26T14:44:10Z

javapoet/src/main/java/com/palantir/javapoet/TypeName.java

-    protected final List<AnnotationSpec> concatAnnotations(List<AnnotationSpec> annotations) {
+    final List<AnnotationSpec> concatAnnotations(List<AnnotationSpec> annotations) {


Changed this because if I understand it correctly TypeName cannot be subclassed by users, so it makes no sense that they see this method if they cannot use it anyway.

Marcono1234 · 2024-09-26T14:45:44Z

javapoet/src/main/java/com/palantir/javapoet/ArrayTypeName.java

    static ArrayTypeName get(ArrayType mirror, Map<TypeParameterElement, TypeVariableName> typeVariables) {
+        checkNotNull(mirror, "mirror == null");


For all these get(... mirror, Map typeVariables) methods in the ...TypeName classes I wasn't completely sure if typeVariables can be null either. So for now I did not add a null check.

Marcono1234 · 2024-09-26T14:47:17Z

javapoet/src/main/java/com/palantir/javapoet/TypeSpec.java

-            // Compact constructor.
+            // Canonical record constructor.


This just writes the canonical constructor, whether it is also "compact" depends on whether MethodSpec.compactConstructorBuilder() was used.

Marcono1234 · 2024-09-26T14:49:31Z

javapoet/src/main/java/com/palantir/javapoet/TypeSpec.java

-                throw new UnsupportedOperationException(kind + " can't have record constructor");
-            }
+            checkState(this.kind == Kind.RECORD, "%s can't have record constructor", this.kind);
+            checkState(this.recordConstructor == null, "record constructor already set to %s", this.recordConstructor);


This "record constructor already set" is probably consistent with most other builder methods, which don't allow overwriting values:

TypeSpec.Builder#superclass

MethodSpec.Builder#defaultValue

FieldSpec.Builder#initializer

(but there are some exceptions)

Marcono1234 · 2024-09-26T14:50:47Z

javapoet/src/main/java/com/palantir/javapoet/TypeSpec.java

@@ -786,19 +810,23 @@ public Builder addEnumConstant(String name) {
        }

        public Builder addEnumConstant(String name, TypeSpec typeSpec) {
+            checkNotNull(name, "name == null");
+            checkNotNull(typeSpec, "typeSpec == null");
+            checkState(kind == Kind.ENUM, "only enums can have enum constants");


Side note: This "only enums can have enum constants" is currently also checked in build() (with message "%s is not enum" there)

Marcono1234 · 2024-09-26T14:52:12Z

javapoet/src/main/java/com/palantir/javapoet/TypeSpec.java

+            checkArgument(methodSpec.defaultValue() == null || kind == Kind.ANNOTATION,
+                    "method with default value is only allowed for annotation type");


Side note: This defaultValue() check already exists in build() as well

Marcono1234 · 2024-09-26T14:56:41Z

javapoet/src/test/java/com/palantir/javapoet/ParameterSpecTest.java

-        a = ParameterSpec.builder(int.class, "i").addModifiers(Modifier.STATIC).build();
-        b = ParameterSpec.builder(int.class, "i").addModifiers(Modifier.STATIC).build();
+        a = ParameterSpec.builder(int.class, "i").addModifiers(Modifier.FINAL).build();
+        b = ParameterSpec.builder(int.class, "i").addModifiers(Modifier.FINAL).build();


This includes the fix from #85 (comment), where it was previously not delegating the call, and this addModifiers overload did not validate the modifiers.

Marcono1234 · 2024-11-24T01:25:24Z

Currently still waiting for an answer to my questions regarding the CLA, see #84 (comment).

Marcono1234 added 4 commits September 26, 2024 16:16

Refactor null checks and add additional ones

bfccac5

Fix ParameterSpec not always validating modifiers

fcb2fa8

Perform additional validation & fail earlier for incorrect arguments

9a740e2

Add addModifiers(Iterable) overload to FieldSpec and TypeSpec

748d602

MethodSpec and ParameterSpec already had this.

Tweak exception messages

73c216c

Marcono1234 commented Sep 26, 2024

View reviewed changes

Marcono1234 mentioned this pull request Sep 26, 2024

Fix incomplete toBuilder implementations and add tests #85

Open

Add more Javadoc to builder methods

a36f20b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform additional validation & fail earlier for incorrect arguments #87

Perform additional validation & fail earlier for incorrect arguments #87

Marcono1234 commented Sep 26, 2024 •

edited

Loading

palantirtech commented Sep 26, 2024

changelog-app bot commented Sep 26, 2024

Marcono1234 Sep 26, 2024

Marcono1234 Sep 26, 2024

Marcono1234 Sep 26, 2024

Marcono1234 Sep 26, 2024

Marcono1234 Sep 26, 2024

Marcono1234 Sep 26, 2024

Marcono1234 Sep 26, 2024

Marcono1234 Sep 26, 2024

Marcono1234 commented Nov 24, 2024

		checkState(enclosingClassName == null \|\| enclosingClassName.packageName.equals(packageName),
		"enclosing class is in wrong package");

		protected final List<AnnotationSpec> concatAnnotations(List<AnnotationSpec> annotations) {
		final List<AnnotationSpec> concatAnnotations(List<AnnotationSpec> annotations) {

		static ArrayTypeName get(ArrayType mirror, Map<TypeParameterElement, TypeVariableName> typeVariables) {
		checkNotNull(mirror, "mirror == null");

		checkArgument(methodSpec.defaultValue() == null \|\| kind == Kind.ANNOTATION,
		"method with default value is only allowed for annotation type");

Perform additional validation & fail earlier for incorrect arguments #87

Are you sure you want to change the base?

Perform additional validation & fail earlier for incorrect arguments #87

Conversation

Marcono1234 commented Sep 26, 2024 • edited Loading

Before this PR

After this PR

Possible downsides?

palantirtech commented Sep 26, 2024

changelog-app bot commented Sep 26, 2024

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marcono1234 commented Nov 24, 2024

Marcono1234 commented Sep 26, 2024 •

edited

Loading

Generate changelog in `changelog/@unreleased`