Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Lombok version 1.18.28 and higher adds a breaking change with NPE checks on builder #3717

Open
raviagarwal7 opened this issue Aug 8, 2024 · 0 comments

Comments

@raviagarwal7
Copy link

Describe the bug
This commit adds a breaking change (or probably fixes a bug) which now generates NPE checks on builder methods for NotNull properties.

		for (JCAnnotation ann : anns) {
-			for (String nn : NONNULL_ANNOTATIONS) if (typeMatches(nn, node, ann)) return true;
+			String annotationTypeName = getTypeName(ann.annotationType);
+			for (String nn : NONNULL_ANNOTATIONS) if (typeMatches(nn, node, annotationTypeName)) return true;
		}

method call with ann was replaced with ann.annotationType which results in the behaviour.

To Reproduce
This project has the example steps (tests fail) to reporo the issue.
If lombok version is downgraded to 1.18.26 the issue doesn't happen (tests passes)

Example class

package org.example;

import lombok.*;

import javax.annotation.Nonnull;

@Data
@NoArgsConstructor
@Builder(toBuilder = true)
public class SimplePojo {
    @Nonnull
    private String nonNullProperty;

    private String nullableProperty;

    public SimplePojo(String nonNullProperty, String nullableProperty) {
        this.nonNullProperty = nonNullProperty;
        this.nullableProperty = nullableProperty;
    }
}

Generated builder class with 1.18.26

    public static class SimplePojoBuilder {
        private String nonNullProperty;
        private String nullableProperty;

        SimplePojoBuilder() {
        }

        public SimplePojoBuilder nonNullProperty(@Nonnull String nonNullProperty) {
            this.nonNullProperty = nonNullProperty;
            return this;
        }

        public SimplePojoBuilder nullableProperty(String nullableProperty) {
            this.nullableProperty = nullableProperty;
            return this;
        }

        public SimplePojo build() {
            return new SimplePojo(this.nonNullProperty, this.nullableProperty);
        }
  } 

Generated builder class with 1.18.34

    @Generated
    public static class SimplePojoBuilder {
        @Generated
        private String nonNullProperty;
        @Generated
        private String nullableProperty;

        @Generated
        SimplePojoBuilder() {
        }

        @Generated
        public SimplePojoBuilder nonNullProperty(@Nonnull String nonNullProperty) {
            if (nonNullProperty == null) {
                throw new NullPointerException("nonNullProperty is marked non-null but is null");
            } else {
                this.nonNullProperty = nonNullProperty;
                return this;
            }
        }

        @Generated
        public SimplePojoBuilder nullableProperty(String nullableProperty) {
            this.nullableProperty = nullableProperty;
            return this;
        }

        @Generated
        public SimplePojo build() {
            return new SimplePojo(this.nonNullProperty, this.nullableProperty);
        }
  }

Below code will throw NPE in 1.18.34 but not in 1.18.26

    @Test
    public void testBuilder() {
        SimplePojo sp = SimplePojo.builder().nonNullProperty(null).build();
        System.out.println(sp);
    }

    @Test
    public void testToBuilder() {
        SimplePojo sp = new SimplePojo();
        SimplePojo spNew = sp.toBuilder().build();
        System.out.println(spNew);
    }

Expected behavior
NPE shouldn't be thrown in public SimplePojoBuilder nonNullProperty( which was the behaviour in 1.18.26

Version info (please complete the following information):

  • Lombok version - 1.18.34
  • Platform (javac or eclipse, and if so, what is the output of javac -version / the version listed in the about... dialog of eclipse. - javac 11.0.15

Additional context
The issue only happens with import javax.annotation.Nonnull; annotation. When using lombok.NonNull both lombok versions generate an NPE check.
Also, it is an issue if the user has a custom AllArgsConstructor. If it is generated by lombok then the issue doesn't manifest since the constructor has null checks in both old and new versions. This means build method will eventually fail when constructing the object thereby preventing any major runtime issues.

Ideally it is the responsibility of the user to ensure NullChecks were added correctly in the AllArgsConstructor and can be fixed during lombok upgrade. But, we are trying to upgrade a large codebase which is currently using 1.18.8 version of lombok (5 years old) and we want to reduce the blast radius.

Would appreciate any feedback on the path forward:
Option 1) Add a flag to disable NPE check generation in builder set method, which can then be used by a user to slowly enable
Option 2) No changes in lombok and we fix all the bad usages.

In either case it would be good to callout this change as a breaking change for other developers to know in lombok's changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant