[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=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=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=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=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=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=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=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=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=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=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=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=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=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=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=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)
