[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17142386#comment-17142386 ] Jörg Hoh commented on SLING-8706: - I think everyone agrees, that even with the best approaches and design you cannot prevent simple errors, if the user/developer is not trained or is in such a hurry that even simple rules are ignored. And yes, you can write failing code with {{Optional}} as well. But my personal impression is that it leads often/most times/often enough to rethink the usage of a field. More often than you think of "oh, that String can be {{Null}}, let's add a Null check". Having said that, I think that it could be a nice extension. Probably not everyone will benefit from it the same way, but some do. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > Time Spent: 2h 40m > Remaining Estimate: 0h > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17142358#comment-17142358 ] Paul Bjorkstrand commented on SLING-8706: - I agree with [~justinedelson] on the HTL side. Just because Sling Models supports this feature doesn't make it required to be supported in HTL (though, only one supporting with the other not supporting {{Optional}} could lead to some confusion). I also have never seen any (Java) serialization of Sling Models, so I don't see that as a barrier either. Besides, serialization [can be customized|https://howtodoinjava.com/java/serialization/custom-serialization-readobject-writeobject/] ([in more than one way|https://dzone.com/articles/how-to-customize-serialization-in-java-using-the-e]) to work around non-serializable types already. Based on the intended use of {{Optional}} (to be used at API boundaries aka method returns), this I feel is a gray area. While a field isn't technically part of an API, they are injected by some external process. One could argue that the fields are equivalent to the return values of that external process. In that argument, it makes sense to capture that intent in the type system itself, to capture the fact that "this field could be null, so you need to check it". This alone makes me think this is a reasonable approach to handling optional injections. That being said, there is nothing stopping a developer from improperly using an {{Optional}}, say, by using {{.get()}} without first checking {{.isPresent()}}, in turn causing a NPE. While bad practice, yes, it happens frequently enough that I almost always have a conversation with my teams on proper {{Optional}} usage. I still believe this idea is reasonable, but I don't buy into the "optional will prevent NPEs" argument. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > Time Spent: 2h 40m > Remaining Estimate: 0h > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17142111#comment-17142111 ] Justin Edelson commented on SLING-8706: --- The question of how HTL interprets methods which return {{Optional}} should really be orthogonal. Whether or not Sling Models supports *injection* of {{Optional}} fields has no bearing on the API exposed by a model class. One could have a Sling Model class *today* used by HTL which had a method which returned an {{Optional}} value. I don't see why the lack of serialization support is a problem, for two reasons -- one, I'm not sure how common is it to use Java serialization for model classes, second, this is (no pun intended) an _optional_ feature so worst case a model developer would need to make a choice between using Java serialization and this method for optional field injection. While yes, {{Optional}} wasn't intended to be used in this way, I see no documentation that its usage as a field is *prohibited*. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > Time Spent: 2h 20m > Remaining Estimate: 0h > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17142083#comment-17142083 ] Jörg Hoh commented on SLING-8706: - [~jmanuelbr] Using Java Optionals would be just another way to express the fact, that a certain field might not be present. You still can change the overall strategy or annotate any field individually with {{@Optional}}, the current behavior and the way annotations are working must not change. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > Time Spent: 2h 10m > Remaining Estimate: 0h > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17142050#comment-17142050 ] Jose Berciano commented on SLING-8706: -- Hi [~joerghoh], just curious how would you go about defining optional as default injection strategy? E.g. {code:java} @Model(adaptables = Resource.class, defaultInjectionStrategy = DefaultInjectionStrategy.OPTIONAL) public class SomeModel { {code} Would this require to implement all class attributes as optional? And if not compilation error? > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > Time Spent: 2h 10m > Remaining Estimate: 0h > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17141917#comment-17141917 ] Jörg Hoh commented on SLING-8706: - I know quite a lot of SlingModels, where fields are marked optional, but later in the postConstruct method the values are consumed in a way as they are always present. Using {{Optional}} we can force the developers to pay attention to the fact that this field cannot be assumed to be filled/available in all cases (I cannot enforce that these fields are always checked for Null!) In there are definitely cases, where {{@Default}} cannot compensate for that. Also i think that the impact on the memory usage and performance is negligible. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > Time Spent: 2h 10m > Remaining Estimate: 0h > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17141790#comment-17141790 ] Tomasz Niedźwiedź commented on SLING-8706: -- Hi [~joerghoh]. I saw your tweet about this feature. It does look interesting but I feel the downsides might outweigh the benefits. As [~sseifert] pointed out, {{Optional}} wasn't really designed to be used in fields. I do see a clear benefit in using {{Optional}} at API boundaries, where another party may consume my code. Inside a Sling model, it looks like a bit of an ovekill. These kind of classes are usually small, easy to understand and easy to control. Existing Sling Models features allow us to provide default values for missing fields. The lack of serialisation may not be more than a code smell (given how Sling Models are used these days) but it would be interesting to assess how using instances of {{Optional}} compares to the use of plain {{null}} references in terms of the amount of memory consumed. Also, how would one go about exposing a field like this in HTL? Would HTL understand the Optional: {code:java} // HTL expected to treat the Optional as falsy? public Optional getMyProperty() { return myProperty; } {code} or would I have to unwrap the {{Optional}}, as in: {code:java} public String getMyProperty() { return myProperty.orElse(""); }{code} in which case, I'd actually prefer to use some static utility method or an operator and handle a simple null check. Or I could use {{@Default}}. [~joerghoh] could you provide some examples of models where this approach would save the developer some effort/errors? I can't think of a model where this would help significantly that wouldn't be solvable via existing Sling features and/or refactoring. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > Time Spent: 2h 10m > Remaining Estimate: 0h > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17141464#comment-17141464 ] Jörg Hoh commented on SLING-8706: - [~sseifert], [~justin] ,[~kwin] : WDYT about the linked PR by [~etugarev]? I would really love to have that feature available because it helps to make sling models more resilient against declared but unimplemented behavior. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > Time Spent: 2h 10m > Remaining Estimate: 0h > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16981178#comment-16981178 ] Evgeny Tugarev commented on SLING-8706: --- [~joerghoh] I made a progress with the implementation, please have a look/comment. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > Time Spent: 2h 10m > Remaining Estimate: 0h > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16975777#comment-16975777 ] Evgeny Tugarev commented on SLING-8706: --- Thanks for the detailed explanation, [~joerghoh] I'll start with the implementation. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16974912#comment-16974912 ] Jörg Hoh commented on SLING-8706: - While I agree on the reasoning of the linked statement provided by [~sseifert], I wonder if it's exactly matching the situation here. Technically nothing prevents you to use Java serialization with a SlingModel, but I would assume that it is a rather uncommon and unusual approach. When you serialize SlingModels, then you are rather using the SlingModelExporter, and that's a very different situation. My intention is to force a developer to explicitly handle the case of an optional property being ```null```. If we extend the SlingModelExporter to handle the ```@Optional``` there is no difference in observable behavior, ```!optionalProp.isPresent()``` can still be treated as a property being null. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16974320#comment-16974320 ] Evgeny Tugarev commented on SLING-8706: --- Thanks, [~sseifert] this exactly what I meant. I'll check for other tickets :) > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16974278#comment-16974278 ] Stefan Seifert commented on SLING-8706: --- there is an stackoverflow article about "Why java.util.Optional is not Serializable" with an answer https://stackoverflow.com/a/24564612 "... the primary design goal for Optional is to be used as the return value of functions when a return value might be absent. ... It was never intended for Optional to be used other ways, such as for optional method arguments or to be stored as a field in an object. And by extension, making Optional serializable would enable it to be stored persistently or transmitted across a network, both of which encourage uses far beyond its original design goal." so the pattern above would somewhat contradict this design goal, although i like the approach as well. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16971748#comment-16971748 ] Evgeny Tugarev commented on SLING-8706: --- I can have a look on implementing this, however I am slightly concerned that _Optional_ class is not serializable. I think this will introduce an artificial limitation and may be problematic in long run. Please correct me if I am wrong. > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (SLING-8706) Injections for java.util.Optional<> should be automatic optional
[ https://issues.apache.org/jira/browse/SLING-8706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16929437#comment-16929437 ] Ahmed Musallam commented on SLING-8706: --- YES! I would love to see this being implemented! > Injections for java.util.Optional<> should be automatic optional > - > > Key: SLING-8706 > URL: https://issues.apache.org/jira/browse/SLING-8706 > Project: Sling > Issue Type: Improvement > Components: Sling Models >Reporter: Jörg Hoh >Priority: Major > > The current approach to support optional injections requires to annotate the > field with {{@Optional}} plus proper handling within the javacode (null > checks etc), which can be forgotten. > So instead of > {code} > @Inject @Optional > String fieldname; > {code} > it should also be possible to use this > {code} > @Inject > Optional fieldname; > {code} > with the very same semantic. But the developer is forced to deal with the > case that the value is not present. -- This message was sent by Atlassian Jira (v8.3.2#803003)