[ 
https://issues.apache.org/jira/browse/SLING-6609?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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> 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> T get(@Nonnull String name, @Nonnull Class<T> type)}})



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Reply via email to