[GitHub] [sling-org-apache-sling-models-impl] paul-bjorkstrand commented on a change in pull request #17: SLING-8706 - Injections for java.util.Optional<> should be automatically optional

[GitBox <gi...@apache.org>]

paul-bjorkstrand commented on a change in pull request #17: SLING-8706 - Injections for java.util.Optional<> should be automatically optional
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/17#discussion_r347150037
 
 

 ##########
 File path: src/main/java/org/apache/sling/models/impl/injectors/ValueMapInjector.java
 ##########
 @@ -98,6 +99,11 @@ public Object getValue(@NotNull Object adaptable, String name, @NotNull Type typ
                 return null;
             }
             Class<?> collectionType = (Class<?>) pType.getRawType();
+
+            if (collectionType.equals(Optional.class)) {
+                return Optional.ofNullable(map.get(name));
 
 Review comment:
   In addition to [my other concern], this call to `map.get(..)` would need to use the actual type extracted from the `ParameterizedType` of the field, similar to how it is done on [line 70 in this PR]. You can see the extraction of the actual type argument in lines [95][line 95 in this PR]-101 in this file in this PR, or starting on [line 1004][line 1004 of ModelAdapterFactory].
   
   This implementation would likely fail for situations where the type in the JCR does not match the expected injection site type. Example: if you have a `String` field in the model, but the value stored in the JCR is `String[]` or `Long`. Using just `map.get(name)` returns the actual type stored as `Object`. If that is a `String[]`, you cannot (in java) cast to `String`.
   
   [my other concern]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/17#issuecomment-554768731
   [line 70 in this PR]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/17/files#diff-c249fe75556c30b251535a7eb4309b7dR70
   [line 95 in this PR]: https://github.com/apache/sling-org-apache-sling-models-impl/pull/17/files#diff-c249fe75556c30b251535a7eb4309b7dR95
   [line 1004 of ModelAdapterFactory]: https://github.com/apache/sling-org-apache-sling-models-impl/blob/master/src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java#L1004

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services