Randgalt / record-builder

Record builder generator for Java records
Apache License 2.0
756 stars 55 forks source link

Validation and null checks are missing for withers. This PR adds them. #48

Closed Randgalt closed 3 years ago

Randgalt commented 3 years ago

Closes #47

cc @danp11

danp11 commented 3 years ago

Hi @Randgalt and thanks for making this PR.

When I'm looking at the generated code I have to questions/concerns:

    ```default RequiredRecord2 withHey(@NotNull String hey) {
        RequiredRecord2 r = RequiredRecord2Builder._downcast(this);
        return (RequiredRecord2)RecordBuilderValidator.validate(new RequiredRecord2(hey, r.i()));
    }

    default RequiredRecord2 withI(@NotNull int i) {
        RequiredRecord2 r = RequiredRecord2Builder._downcast(this);
        return (RequiredRecord2)RecordBuilderValidator.validate(new RequiredRecord2(r.hey(), i));
    }```

1) Eventhough records are recommended to use "as DTO's and Value types" I believe that we in the future will work with large records with many variables. The current generated code will therefore, IMO, have a severe impact on performance if the validation has to be done on all the properties even if we just want to use a with-method on one (1) property.

2) If the parameter is a primitive, does it really make sence to do an extra validation then?

Just my initial thoughts,

Regards Dan

danp11 commented 3 years ago

Wouldn´t it be more appropriated to do the Jsr-303 validation on a property level instead as you suggested here:

Yes, I agree. For validation, you can validate individual fields (referenced as "properties" in the spec) via: https://stackoverflow.com/a/21961677/2048051. A PR would be appreciated.

Thanks.

Randgalt commented 3 years ago

@danp11 I started using property validation and quickly realized that that doesn't work. What if there are custom validations that assert @Min and @Max based on other fields of the record? So, you must validate the entire record every time as any single change might affect other validations. The With has methods to change multiple fields at once. It's assumed if a single withXXX() is called the intent is to change only 1 component otherwise one of the multi-field with methods would be used.

danp11 commented 3 years ago

@Randgalt Ah, yes, you're correct.

Then the code additions in this PR lgtm.