paul-bjorkstrand commented on a change in pull request #17:
URL:
https://github.com/apache/sling-org-apache-sling-models-impl/pull/17#discussion_r443746315
##########
File path: src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
##########
@@ -495,12 +480,97 @@ public RuntimeException inject(InjectableElement element,
Object value) {
}
}
+ @NotNull
+ private InjectableElement getElement(InjectableElement element, Type type)
{
+ return new InjectableElement() {
+ @Override
+ public AnnotatedElement getAnnotatedElement() {
+ return element.getAnnotatedElement();
+ }
+
+ @Override
+ public Type getType() {
+ return type;
+ }
+
+ @Override
+ public boolean isPrimitive() {
+ return element.isPrimitive();
+ }
+
+ @Override
+ public String getName() {
+ return element.getName();
+ }
+
+ @Override
+ public String getSource() {
+ return element.getSource();
+ }
+
+ @Override
+ public String getVia() {
+ return element.getVia();
+ }
+
+ @Override
+ public Class<? extends ViaProviderType> getViaProviderType() {
+ return element.getViaProviderType();
+ }
+
+ @Override
+ public boolean hasDefaultValue() {
+ return element.hasDefaultValue();
+ }
+
+ @Override
+ public Object getDefaultValue() {
+ return element.getDefaultValue() == null ? Optional.empty() :
element.getDefaultValue();
+ }
+
+ @Override
+ public boolean isOptional(InjectAnnotationProcessor
annotationProcessor) {
+ return true;
+ }
+ };
+ }
+
private
@Nullable
RuntimeException injectElement(final InjectableElement element, final
Object adaptable,
final @NotNull DisposalCallbackRegistry
registry, final InjectCallback callback,
final @NotNull Map<ValuePreparer, Object>
preparedValues,
final @Nullable BundleContext modelContext)
{
+ if (element instanceof InjectableField) {
+ Type genericType = ((InjectableField)
element).getFieldGenericType();
+
+ if (genericType instanceof ParameterizedType) {
+ ParameterizedType pType = (ParameterizedType) genericType;
+
+ if (pType.getRawType().equals(Optional.class)) {
+ InjectableElement el = getElement(element,
pType.getActualTypeArguments()[0]);
+
+ return injectElementInternal(el, adaptable, registry, new
InjectCallback() {
+ @Override
+ public RuntimeException inject(InjectableElement
element1, Object value) {
+ return callback.inject(element,
value.equals(Optional.empty())
+ ? value
+ : Optional.of(value));
Review comment:
This part is perplexing me. Why would you wrap the value in
`Optional.of(..)` only when the parameter `value` is not an empty optional?.
To me, this seems like it should be more like this:
```suggestion
return callback.inject(element1,
Optional.ofNullable(value));
```
##########
File path: src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
##########
@@ -495,12 +480,97 @@ public RuntimeException inject(InjectableElement element,
Object value) {
}
}
+ @NotNull
+ private InjectableElement getElement(InjectableElement element, Type type)
{
+ return new InjectableElement() {
+ @Override
+ public AnnotatedElement getAnnotatedElement() {
+ return element.getAnnotatedElement();
+ }
+
+ @Override
+ public Type getType() {
+ return type;
+ }
+
+ @Override
+ public boolean isPrimitive() {
+ return element.isPrimitive();
+ }
+
+ @Override
+ public String getName() {
+ return element.getName();
+ }
+
+ @Override
+ public String getSource() {
+ return element.getSource();
+ }
+
+ @Override
+ public String getVia() {
+ return element.getVia();
+ }
+
+ @Override
+ public Class<? extends ViaProviderType> getViaProviderType() {
+ return element.getViaProviderType();
+ }
+
+ @Override
+ public boolean hasDefaultValue() {
+ return element.hasDefaultValue();
+ }
+
+ @Override
+ public Object getDefaultValue() {
+ return element.getDefaultValue() == null ? Optional.empty() :
element.getDefaultValue();
+ }
+
+ @Override
+ public boolean isOptional(InjectAnnotationProcessor
annotationProcessor) {
+ return true;
+ }
+ };
+ }
+
private
@Nullable
RuntimeException injectElement(final InjectableElement element, final
Object adaptable,
final @NotNull DisposalCallbackRegistry
registry, final InjectCallback callback,
final @NotNull Map<ValuePreparer, Object>
preparedValues,
final @Nullable BundleContext modelContext)
{
+ if (element instanceof InjectableField) {
+ Type genericType = ((InjectableField)
element).getFieldGenericType();
+
+ if (genericType instanceof ParameterizedType) {
+ ParameterizedType pType = (ParameterizedType) genericType;
+
+ if (pType.getRawType().equals(Optional.class)) {
+ InjectableElement el = getElement(element,
pType.getActualTypeArguments()[0]);
+
+ return injectElementInternal(el, adaptable, registry, new
InjectCallback() {
+ @Override
+ public RuntimeException inject(InjectableElement
element1, Object value) {
+ return callback.inject(element,
value.equals(Optional.empty())
+ ? value
+ : Optional.of(value));
Review comment:
If you wanted to go a step further, you could replace the entire
`InjectCallback` here with a new type, like the ones above in this file:
```java
private class OptionalWrappingCallbackimplements InjectCallback {
private final InjectCallback chainedCallback;
private OptionalWrappingCallback(InjectCallback chainedCallback) {
this.chainedCallback= chainedCallback;
}
@Override
public RuntimeException inject(InjectableElement element, Object
value) {
return chainedCallback.inject(element,
Optional.ofNullable(value));
}
}
```
This makes the call to `injectInternal(..)` simpler:
```java
return injectElementInternal(el, adaptable, registry, new
OptionalWrappingCallback(callback), preparedValues, modelContext);
```
(Note: I wrote these directly in this review, so forgive any syntax errors).
##########
File path: src/main/java/org/apache/sling/models/impl/ModelAdapterFactory.java
##########
@@ -495,12 +480,97 @@ public RuntimeException inject(InjectableElement element,
Object value) {
}
}
+ @NotNull
+ private InjectableElement getElement(InjectableElement element, Type type)
{
+ return new InjectableElement() {
+ @Override
+ public AnnotatedElement getAnnotatedElement() {
+ return element.getAnnotatedElement();
+ }
+
+ @Override
+ public Type getType() {
+ return type;
+ }
+
+ @Override
+ public boolean isPrimitive() {
+ return element.isPrimitive();
+ }
+
+ @Override
+ public String getName() {
+ return element.getName();
+ }
+
+ @Override
+ public String getSource() {
+ return element.getSource();
+ }
+
+ @Override
+ public String getVia() {
+ return element.getVia();
+ }
+
+ @Override
+ public Class<? extends ViaProviderType> getViaProviderType() {
+ return element.getViaProviderType();
+ }
+
+ @Override
+ public boolean hasDefaultValue() {
+ return element.hasDefaultValue();
+ }
+
+ @Override
+ public Object getDefaultValue() {
+ return element.getDefaultValue() == null ? Optional.empty() :
element.getDefaultValue();
Review comment:
Ok, after reading through this again (after making the comment below), I
understand where the confusion comes from. Changing the type here is causing
you to have some perplexing code in the `InjectCallback` below. Instead of
handling Optional in two places, let the default value propagate `null` and use
the null-safe wrapper method, `Optional.ofNullable(..)`.
----------------------------------------------------------------
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:
[email protected]
