You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2020/06/22 18:50:14 UTC
[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
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:
users@infra.apache.org