-
Notifications
You must be signed in to change notification settings - Fork 55
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
Defaults #110
Comments
The library already supports the Javax validation protocol. Is that not sufficient? |
Well, javax.validation only works when a validator is applied to the builder, and does not prevent the creation of an invalid record with nulls inside. The nulls are my primary concern. Anyways, null-checking beans on creation might not be in the scope of what this library is intended to do. So what about those defaults? It for sure would be nice to not have to set the same values for some fields all the time, but it has to work for Jackson as well of course. Any thoughts? |
I'm trying to address all null issues as part of #111. The null handling of Record-Builder is optional but I'd like it be correct and as useful as possible. |
We can have the Builder do defaults but it won't be possible to change the Record itself. I'd love to find a solution to this. At our company we have to add a compact constructor to every Record to check for nulls. It's a huge PITA but without re-writing the class at runtime I don't know a way to get this behavior. Maybe a lombok style class re-write could be done as an option but that starts to tread into a very ugly territory. |
I do know that immutables does this by forcing jackson to use the builder to deserialize rather than deserializing into the record (since jackson does understand the builder pattern), but not sure if that's something we want to do.. @JsonProperty does have a defaultValue attribute, so if we read that annotation, pluck out the default and initialize the builder variables with those, we do have some semblance of support here. One sad part of that solution is that defaultValue is a string and then we are in a situation where we have to somehow know how to (de)serialize this value, which is not trivial, especially considering how much custom behaviour it's possible to plug into Jackson |
FWIW how my company does this for the case with Jackson is our own annotation processors generates Jackson mixin classes Essentially the builder could be a Mixin for the record but it probably would be best if it was a separate class possibly a static inner class. EDIT (for clarity) the mixin then would have the Now to register all those Mixin you just use the ServiceLoader pattern (to enumerate every mixin that needs registering) and tell the users of record-builder to call some static method on an interface that will setup objectmapper. Lots of annotation processors do this trick. Otherwise I agree with @mads-b that the builder is what should get filled by reflection based tools. I actually tried to explain this on #107 that records are more value objects that are invariants and if a record is built it should be correct and not need validation.
If you make a record without ever having reflection do it directly (e.g. the builder always is the one that gets built with reflection) you can use static analysis to guarantee that the record will not have nulls (and possibly other guarantees if you use something sophisticated like checkerframework). As for the defaults problem in general you could have a mixin interface the builder will implement with default methods that say what is default (this is sort of how Immutables works and our own processors). Or alternatively you could have static methods that generate the default on the record itself with an annotation like |
I'm now using RecordBuilder and find it very convenient, thanks a lot, but defaults definitely would be an improvement. What about an additional annotation like this:
Thinking of a good way to define NOW default for Instant properties.. |
@pragmasoft-ua this library has gotten quite complex over the years so I'm cautious about adding new features that don't dovetail with upcoming JDK features. I have mixed feelings about this. |
Also @pragmasoft-ua - I think you can accomplish this by writing a purpose-built small builder that proxies to the generated builder. |
I'm having second thoughts about adding support for this. I was looking at how Immutables does it: https://immutables.github.io/immutable.html#default-attributes and an implementation occurred to me. What if we allow annotated constants in the record class. RecordBuilder would then use these as default values/initializers. Something like:
The value of wdyt? @pragmasoft-ua, @agentgt, @mads-b |
Good idea, like it |
New component annotation that specifies the name of either a public static final field or a public static method that will be used to initialize each record component in the generated builder so as to support default values. Closes #110
Folks, please comment/review #163 |
New component annotation that specifies the name of either a public static final field or a public static method that will be used to initialize each record component in the generated builder so as to support default values. Closes #110
New component annotation that specifies the name of either a public static final field or a public static method that will be used to initialize each record component in the generated builder so as to support default values. Closes #110
New component annotation that specifies the name of either a public static final field or a public static method that will be used to initialize each record component in the generated builder so as to support default values. Closes #110
New component annotation that specifies the name of either a public static final field or a public static method that will be used to initialize each record component in the generated builder so as to support default values. Closes #110
@Randgalt Looks fine to me. However I would see if you can leverage completions: The idea being to check the outer component (the record class) for static fields with matching type for completion of the string literal ( I can't remember how well this works across IDEs though. Otherwise I think I kind of like the flexibility that Immutables does for two reasons (e.g. annotate the method/field instead of the record component): do not need to use a string literal, and can use default methods from interfaces. I can see a lot of advantages in allow instance methods because you can have a sort of library of default methods on mixin interfaces. Furthermore one could leverage threadlocals or scoped values easier/DRY if instance methods are allowed. public interface UserIdMixin {
@SomeAnnotation("userId" /* optional value */)
default UUID defaultUserId() {
return SomeServiceLocatorLike.getCurrentUserId();
}
}
public record Something(UUID userId) implements UserIdMixin {
} Then you have a configurable prefix like However that may not be in the spirit of EDIT I'm an idiot because the default method won't work since you have to instantiate the record 😄 Given the above you could do something like: @Mixin(mixin=UserIdMixin.class)
public record Something(UUID userId) {
} The builder then implements But it is probably way too complicated so I like your original approach more. Sorry for the EDITS |
@agentgt I don't believe there's an equivalent mapping here. Immutables generates the target class and can take advantage of that to set values post creation in order to call these non-static initializer methods. We can't do this with Records as record components must be as part of construction. So, I don't see how this can work with records unless you have a suggestion. |
@Randgalt I edited my comment so you might have not seen the updates. What I'm saying is make the builder implement a provided interface that has the default values (as methods or I suppose static final fields as well since interfaces support that as well). @Mixin(mixin=UserIdProviderMixin.class)
public record Something(UUID userId) {
} Does record builder have a facility for making the builders implement an interface? I can see almost more usage out of that over even the defaults because then whoever is using the builder can call some static methods to fill the builder. public static void fillDefaults(UserIdMixin builder) {
builder.setUserId(...);
}
SomethingBuilder b = new SomethingBuilder();
UserIdMixin.fillDefaults(b); My concern or why I think interface mixins are useful is because folks that want default behavior is probably to avoid boiler plate. If you have to put a static field/method on every record then it doesn't seem so DRY if it is exceedingly common say like current time. |
@agentgt Records are used mostly as data carriers/models for DB tables, REST APIs, etc. Would people really need a standard library of default values? If I have a |
Current time of creation is one I could see usage for it. @Record.Option(mixin=TimeMixin.class)
public record PersonRecord(Instant createTime) {}
public interface TimeMixin {
default Instant createTime() {
return Instant.now();
}
}
// generated
public class PersonRecordBuilder implements TimeMixin {
private Instant createTime = null;
public Instant createTime() {
if (this.createTime == null) {
return TimeMixin.super.createTime();
}
return createTime;
}
} The big advantage to the interface mixin approach is it can be used for more than just defaults but various other logic perhaps validation. EDIT: Generating the proper call super method could be done later but I do see lots of value of make this generated class implement some interface and if it can do defaults than it is one less ad-hoc feature. That is it is more unified at the expense of more complexity for the Record Builder libary. |
There are two things, then, here. We could add the |
I agree. I'm just brainstorming some possible alternatives. The solution in place is far more KISS. I do think there is value in mixin but it really is a different kind of feature. |
I'm not sure if this will help for API modeling but I did implement this for my templating language that uses the annotation processor: https://jstach.io/doc/jstachio/current/apidocs/io.jstach.jstache/io/jstach/jstache/JStacheInterfaces.html Basically it allows you to add annotations, implements interfaces, or extends classes as well as allowing some level of generics. In this case you can replace "template" with "builder". I have found the above feature useful but I'm not sure how many users use it outside of DI annotation injecting. |
New component annotation that specifies the name of either a public static final field or a public static method that will be used to initialize each record component in the generated builder so as to support default values. Closes #110
New component annotation that specifies the name of either a public static final field or a public static method that will be used to initialize each record component in the generated builder so as to support default values. Closes #110
New component annotation that specifies the name of either a public static final field or a public static method that will be used to initialize each record component in the generated builder so as to support default values. Closes #110
I'd like a way to supply default values, so the record is in the proper state if a field is not set using the builder.
Suggestions:
@default(myvalueHere)
A magic static method that gets called prior to building with the builder as a parameter? This way we could do validation prior to building as well.
The text was updated successfully, but these errors were encountered: