From 47ae6f46db32fd0f5b692176467ec8fe7c6fa532 Mon Sep 17 00:00:00 2001 From: Tomas Langer Date: Tue, 1 Oct 2024 16:04:59 +0200 Subject: [PATCH] Failing on private field injection, private constructor injection, multiple constructor injection A few bugfixes for problems discovered by tests --- .../service/codegen/InjectionExtension.java | 105 +++++++++++++----- .../codegen/InjectionExtensionProvider.java | 3 +- .../helidon/service/codegen/MapCreateFor.java | 73 ------------ .../codegen/src/main/java/module-info.java | 3 +- .../service/inject/ScopedRegistryImpl.java | 3 +- .../tests/inject/configdriven/IService.java | 2 +- .../inject/scopes/CustomScopedProducer.java | 2 +- 7 files changed, 82 insertions(+), 109 deletions(-) delete mode 100644 service/codegen/src/main/java/io/helidon/service/codegen/MapCreateFor.java diff --git a/service/codegen/src/main/java/io/helidon/service/codegen/InjectionExtension.java b/service/codegen/src/main/java/io/helidon/service/codegen/InjectionExtension.java index c9cd1acb22e..90456d69bb9 100644 --- a/service/codegen/src/main/java/io/helidon/service/codegen/InjectionExtension.java +++ b/service/codegen/src/main/java/io/helidon/service/codegen/InjectionExtension.java @@ -211,6 +211,23 @@ private static void toEnumValue(ContentBuilder contentBuilder, Enum enumVa .addContent(enumValue.name()); } + private static void addInterfaceAnnotations(List elementAnnotations, + List declaredElements) { + + for (TypedElements.DeclaredElement declaredElement : declaredElements) { + declaredElement.element() + .annotations() + .forEach(it -> addInterfaceAnnotation(elementAnnotations, it)); + } + } + + private static void addInterfaceAnnotation(List elementAnnotations, Annotation annotation) { + // only add if not already there + if (!elementAnnotations.contains(annotation)) { + elementAnnotations.add(annotation); + } + } + private void generateMain(TypeInfo customMain) { // main class is ONLY generated if required by the user (through Injection.Main annotation) // generate the `ApplicationMain` that just starts the registry (with auto-discovery) @@ -667,30 +684,75 @@ private SuperType superType(TypeInfo typeInfo, Collection services) { // find constructor with @Inject, if none, find the first constructor (assume @Inject) private TypedElementInfo injectConstructor(TypeInfo typeInfo) { - // first @Inject - return typeInfo.elementInfo() + var constructors = typeInfo.elementInfo() .stream() .filter(it -> it.kind() == ElementKind.CONSTRUCTOR) .filter(it -> it.hasAnnotation(ServiceCodegenTypes.INJECTION_INJECT)) - .findFirst() - // or first non-private constructor - .or(() -> typeInfo.elementInfo() - .stream() - .filter(not(ElementInfoPredicates::isPrivate)) - .filter(it -> it.kind() == ElementKind.CONSTRUCTOR) - .findFirst()) - // or default constructor - .orElse(TypedElements.DEFAULT_CONSTRUCTOR.element()); + .collect(Collectors.toUnmodifiableList()); + if (constructors.size() > 1) { + throw new CodegenException("There can only be one constructor annotated with " + + ServiceCodegenTypes.INJECTION_INJECT.fqName() + ", but there were " + + constructors.size(), + typeInfo.originatingElementValue()); + } + if (!constructors.isEmpty()) { + // @Injection.Inject + TypedElementInfo first = constructors.getFirst(); + if (ElementInfoPredicates.isPrivate(first)) { + throw new CodegenException("Constructor annotated with " + ServiceCodegenTypes.INJECTION_INJECT.fqName() + + " must not be private."); + } + return first; + } + + // or first non-private constructor + var allConstructors = typeInfo.elementInfo() + .stream() + .filter(it -> it.kind() == ElementKind.CONSTRUCTOR) + .collect(Collectors.toUnmodifiableList()); + + if (allConstructors.isEmpty()) { + // there is no constructor declared, we can use default + return TypedElements.DEFAULT_CONSTRUCTOR.element(); + } + var nonPrivateConstructors = allConstructors.stream() + .filter(not(ElementInfoPredicates::isPrivate)) + .collect(Collectors.toUnmodifiableList()); + if (nonPrivateConstructors.isEmpty()) { + throw new CodegenException("There is no non-private constructor defined for " + typeInfo.typeName().fqName(), + typeInfo.originatingElementValue()); + } + if (nonPrivateConstructors.size() > 1) { + throw new CodegenException("There are more non-private constructors defined for " + typeInfo.typeName().fqName(), + typeInfo.originatingElementValue()); + } + return nonPrivateConstructors.getFirst(); } private List fieldInjectElements(TypeInfo typeInfo) { - return typeInfo.elementInfo() + var injectFields = typeInfo.elementInfo() .stream() - .filter(not(ElementInfoPredicates::isPrivate)) .filter(not(ElementInfoPredicates::isStatic)) .filter(ElementInfoPredicates::isField) .filter(ElementInfoPredicates.hasAnnotation(ServiceCodegenTypes.INJECTION_INJECT)) .toList(); + var firstFound = injectFields.stream() + .filter(ElementInfoPredicates::isPrivate) + .findFirst(); + if (firstFound.isPresent()) { + throw new CodegenException("Discovered " + ServiceCodegenTypes.INJECTION_INJECT.fqName() + + " annotation on private field(s). We cannot support private field injection.", + firstFound.get().originatingElementValue()); + } + firstFound = injectFields.stream() + .filter(ElementInfoPredicates::isStatic) + .findFirst(); + if (firstFound.isPresent()) { + throw new CodegenException("Discovered " + ServiceCodegenTypes.INJECTION_INJECT.fqName() + + " annotation on static field(s).", + firstFound.get().originatingElementValue()); + } + return injectFields; } private List methodParams(Collection services, @@ -2192,23 +2254,6 @@ private TypeName descriptorInstanceType(TypeName serviceType, TypeName descripto .build(); } - private static void addInterfaceAnnotations(List elementAnnotations, - List declaredElements) { - - for (TypedElements.DeclaredElement declaredElement : declaredElements) { - declaredElement.element() - .annotations() - .forEach(it -> addInterfaceAnnotation(elementAnnotations, it)); - } - } - - private static void addInterfaceAnnotation(List elementAnnotations, Annotation annotation) { - // only add if not already there - if (!elementAnnotations.contains(annotation)) { - elementAnnotations.add(annotation); - } - } - private record GenericTypeDeclaration(String constantName, TypeName typeName) { } diff --git a/service/codegen/src/main/java/io/helidon/service/codegen/InjectionExtensionProvider.java b/service/codegen/src/main/java/io/helidon/service/codegen/InjectionExtensionProvider.java index f0cfacba164..130a59e186d 100644 --- a/service/codegen/src/main/java/io/helidon/service/codegen/InjectionExtensionProvider.java +++ b/service/codegen/src/main/java/io/helidon/service/codegen/InjectionExtensionProvider.java @@ -66,7 +66,8 @@ public Set supportedAnnotations() { return Set.of(ServiceCodegenTypes.INJECTION_INJECT, ServiceCodegenTypes.INJECTION_MAIN, ServiceCodegenTypes.INJECTION_DESCRIBE, - ServiceCodegenTypes.INTERCEPTION_DELEGATE); + ServiceCodegenTypes.INTERCEPTION_DELEGATE, + ServiceCodegenTypes.INJECTION_CREATE_FOR); } @Override diff --git a/service/codegen/src/main/java/io/helidon/service/codegen/MapCreateFor.java b/service/codegen/src/main/java/io/helidon/service/codegen/MapCreateFor.java deleted file mode 100644 index f953063f69a..00000000000 --- a/service/codegen/src/main/java/io/helidon/service/codegen/MapCreateFor.java +++ /dev/null @@ -1,73 +0,0 @@ -/* - * Copyright (c) 2023, 2024 Oracle and/or its affiliates. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package io.helidon.service.codegen; - -import java.util.Collection; -import java.util.Set; - -import io.helidon.codegen.CodegenContext; -import io.helidon.codegen.CodegenOptions; -import io.helidon.codegen.spi.AnnotationMapper; -import io.helidon.codegen.spi.AnnotationMapperProvider; -import io.helidon.common.Weight; -import io.helidon.common.Weighted; -import io.helidon.common.types.Annotation; -import io.helidon.common.types.ElementKind; -import io.helidon.common.types.TypeName; - -import static io.helidon.service.codegen.ServiceCodegenTypes.INJECTION_CREATE_FOR; - -/** - * A {@link java.util.ServiceLoader} provider implementation for {@link io.helidon.codegen.spi.AnnotationMapperProvider} - * that adds service annotation to driven services (as we want to create the instance just once), unless a scope - * is already defined. - */ -@Weight(Weighted.DEFAULT_WEIGHT - 10) -public class MapCreateFor implements AnnotationMapperProvider { - private static final Annotation SERVICE = Annotation.create(ServiceCodegenTypes.INJECTION_INSTANCE); - - /** - * Required default constructor. - * - * @deprecated required by {@link java.util.ServiceLoader} - */ - @Deprecated - public MapCreateFor() { - } - - @Override - public Set supportedAnnotations() { - return Set.of(INJECTION_CREATE_FOR); - } - - @Override - public AnnotationMapper create(CodegenOptions options) { - return new ConfigDrivenMapper(); - } - - private static class ConfigDrivenMapper implements AnnotationMapper { - @Override - public boolean supportsAnnotation(Annotation annotation) { - return annotation.typeName().equals(INJECTION_CREATE_FOR); - } - - @Override - public Collection mapAnnotation(CodegenContext ctx, Annotation original, ElementKind elementKind) { - return Set.of(original, SERVICE); - } - } -} diff --git a/service/codegen/src/main/java/module-info.java b/service/codegen/src/main/java/module-info.java index 35a32a6f041..376a5d7532b 100644 --- a/service/codegen/src/main/java/module-info.java +++ b/service/codegen/src/main/java/module-info.java @@ -40,6 +40,5 @@ io.helidon.service.codegen.ConfigBeanCodegenProvider; provides io.helidon.codegen.spi.AnnotationMapperProvider - with io.helidon.service.codegen.MapNamedByClassProvider, - io.helidon.service.codegen.MapCreateFor; + with io.helidon.service.codegen.MapNamedByClassProvider; } \ No newline at end of file diff --git a/service/inject/inject/src/main/java/io/helidon/service/inject/ScopedRegistryImpl.java b/service/inject/inject/src/main/java/io/helidon/service/inject/ScopedRegistryImpl.java index b1bb6c9774f..cc89e75458a 100644 --- a/service/inject/inject/src/main/java/io/helidon/service/inject/ScopedRegistryImpl.java +++ b/service/inject/inject/src/main/java/io/helidon/service/inject/ScopedRegistryImpl.java @@ -30,6 +30,7 @@ import io.helidon.service.inject.api.ActivationResult; import io.helidon.service.inject.api.Activator; import io.helidon.service.inject.api.Injection; +import io.helidon.service.inject.api.ScopeNotActiveException; import io.helidon.service.inject.api.ScopedRegistry; import io.helidon.service.registry.ServiceInfo; import io.helidon.service.registry.ServiceRegistryException; @@ -167,7 +168,7 @@ private static Comparator> shutdownComparator() { private void checkActive() { if (!active) { - throw new ServiceRegistryException("Injection scope " + scope.fqName() + "[" + id + "] is not active."); + throw new ScopeNotActiveException("Injection scope " + scope.fqName() + "[" + id + "] is not active.", scope); } } } diff --git a/service/tests/inject/config-driven/src/main/java/io/helidon/service/tests/inject/configdriven/IService.java b/service/tests/inject/config-driven/src/main/java/io/helidon/service/tests/inject/configdriven/IService.java index 61ba90b818c..a3fad2df845 100644 --- a/service/tests/inject/config-driven/src/main/java/io/helidon/service/tests/inject/configdriven/IService.java +++ b/service/tests/inject/config-driven/src/main/java/io/helidon/service/tests/inject/configdriven/IService.java @@ -24,7 +24,7 @@ class IService implements IContract { private boolean running; - // note: initially left w/o a ctor here! + // note: intentionally left w/o a ctor here! /** * For Testing. diff --git a/service/tests/inject/scopes/src/main/java/io/helidon/service/tests/inject/scopes/CustomScopedProducer.java b/service/tests/inject/scopes/src/main/java/io/helidon/service/tests/inject/scopes/CustomScopedProducer.java index 853196c4a9b..ac1d85aa5f6 100644 --- a/service/tests/inject/scopes/src/main/java/io/helidon/service/tests/inject/scopes/CustomScopedProducer.java +++ b/service/tests/inject/scopes/src/main/java/io/helidon/service/tests/inject/scopes/CustomScopedProducer.java @@ -23,7 +23,7 @@ import io.helidon.service.registry.Service; @CustomScope -class CustomScopedProducer implements RequestScopedContract { +class CustomScopedProducer implements CustomScopedContract { private static final AtomicInteger COUNTER = new AtomicInteger(); private final int id = COUNTER.incrementAndGet();