[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16235909#comment-16235909
]
Konrad Windszus commented on SLING-6609:
Fixed with
https://gitbox.apache.org/repos/asf?p=sling-org-apache-sling-api.git;a=commit;h=b35b5d1af152f0e28d87af6da8eba84df0e72a10.
Sorry for that mistake.
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
>Priority: Major
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15898919#comment-15898919
]
Konrad Windszus commented on SLING-6609:
Right, did that in [r1785808|http://svn.apache.org/r1785808].
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15897939#comment-15897939
]
Felix Meschberger commented on SLING-6609:
--
Thanks, [~kwin]. Looks good except, I'd defer to {{get(String)}} instead of
{{get(String, Class)}} as with the {{defaultValue}} being {{null}} we don't
know the desired type and would have to assume {{Object}}, hence no conversion.
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15897456#comment-15897456
]
Konrad Windszus commented on SLING-6609:
Extended the javadoc with [r1785659|http://svn.apache.org/r1785659].
[~fmeschbe] Could you quickly cross-check?
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15897451#comment-15897451
]
Julian Sedding commented on SLING-6609:
---
bq. Maybe you could add the annotation plus add an implementation note, that
implementations should treat being called with {{defaultValue==null}}
equivalent to {{get(String)}} and thus to check for {{null}}.
That sounds reasonable and strikes a nice balance between backwards
compatibility and clarification/evolution of the API contract.
bq. Of course this is debatable and in hind sight we should have properly
define the API such that defaultValue must not be null (with the respective
runtime check on implementations). But we are where we are.
Indeed. Hind sight is a luxury we rarely have in the beginning ;)
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15897208#comment-15897208
]
Felix Meschberger commented on SLING-6609:
--
Fully agreed with the safety net for compile time check as well as for
documentation of adding this annotation.
The question about historic and so is whether this change actually changes
semantics and/or existing expectations of the API. The documentation is not
very clear right now, unfortunately.
For example, assume that historically we accepted null as can be seen from the
ValueMapDecorator implementation. Now, we describe the API to not take null any
longer. So a new implementation of course will not check for null and run into
an NPE (which is fine because the API is not used according to the contract)
while an old consumer who was assuming null to be ok as it has been so far
might now run into that unexpected NPE.
Maybe you could add the annotation plus add an implementation note, that
implementations should treat being called with {{defaultValue==null}}
equivalent to {{get(String)}} and thus to check for {{null}}.
Of course this is debatable and in hind sight we should have properly define
the API such that defaultValue must not be null (with the respective runtime
check on implementations). But we are where we are.
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15897088#comment-15897088
]
Julian Sedding commented on SLING-6609:
---
I agree with [~kwin] on this one. Especially since we have explicit support for
the {{null}} default value (aka. no default value):
{code}
// assuming "properties" is a ValueMap instance and the property "nonExistant"
does not exist, then
assert null == properties.get("nonExistant", (String)null);
// is equivalent to
assert null == properties.get("nonExistant", String.class);
{code}
Or am I missing something?
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15897075#comment-15897075
]
Konrad Windszus commented on SLING-6609:
Even if it was historically supported, I don't think it was useful with a 2nd
parameter being {{null}}. Also the annotations don't touch the implementation
(so passing {{null}} will still work).
It is just that if you use findbugs or Eclipse with null analysis enabled this
will emit a warning if you pass a {{null}} parameter. The other alternative
would be to annotate the return value to {{@CheckForNull}} which is not true in
99% of the cases and therefore requires you to more often check for a null
return value.
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15897056#comment-15897056
]
Felix Meschberger commented on SLING-6609:
--
Ah, right, but historically, I am not sure, this really was the intent.
Particularly if you look at the [ValueMapDecorator
implementation|https://github.com/apache/sling/blob/trunk/bundles/api/src/main/java/org/apache/sling/api/wrappers/ValueMapDecorator.java#L63]
where there is an explicit {{null}} guard
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15896983#comment-15896983
]
Konrad Windszus commented on SLING-6609:
The default value is also supposed to be not null (therefore carries the
annotation @Nonnull as well).
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (SLING-6609) Fix JSR305 annotations for ValueMap.get
[
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15896981#comment-15896981
]
Felix Meschberger commented on SLING-6609:
--
Actually, I am not entirely sure, whether @Nonnull is technically entirely
correct: The spec says, to return the named property or the {{defaultValue}}.
What if the {{defaultValue}} is {{null}} ?
> Fix JSR305 annotations for ValueMap.get
> ---
>
> Key: SLING-6609
> URL: https://issues.apache.org/jira/browse/SLING-6609
> Project: Sling
> Issue Type: Bug
> Components: API
>Affects Versions: API 2.16.2
>Reporter: Konrad Windszus
>Assignee: Konrad Windszus
> Fix For: API 2.16.4
>
>
> Currently {{ T get(@Nonnull String name, T defaultValue);}} does neither
> define a JSR 305 annotation for the return value nor for the 2nd parameter.
> It makes sense to define them both as {{@Nonnull}}, because if you intend to
> get {{null}} as return value you are supposed to take the other get method
> ({{@CheckForNull T get(@Nonnull String name, @Nonnull Class type)}})
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
