Skip to content

Commit

Permalink
Merge pull request #27046 from yrodiere/build-step-no-produces
Browse files Browse the repository at this point in the history 
Remove support for field-based and constructor-based BuildProducers and forbid build steps that produce nothing
  • Loading branch information
@gastaldi
gastaldi authored Aug 1, 2022
2 parents 34f683a + 9a4a383 commit f33b980
Show file tree
Hide file tree
Showing 8 changed files with 59 additions and 86 deletions.
6 changes: 6 additions & 0 deletions core/builder/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
9 changes: 9 additions & 0 deletions core/builder/src/main/java/io/quarkus/builder/BuildStepBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,15 @@ public BuildStepBuilder consumes(Class<? extends BuildItem> type, ConsumeFlags f
*/
public BuildChainBuilder build() {
final BuildChainBuilder chainBuilder = this.buildChainBuilder;
if (produces.isEmpty()) {
throw new IllegalArgumentException(
"Build step '" + buildStep.getId()
+ "' does not produce any build item and thus will never get executed."
+ " Either change the return type of the method to a build item type,"
+ " add a parameter of type BuildProducer<[some build item type]>/Consumer<[some build item type]>,"
+ " or annotate the method with @Produces."
+ " Use @Produce(EmptyBuildItem.class) if you want to always execute this step.");
}
if (BuildChainBuilder.LOG_CONFLICT_CAUSING) {
chainBuilder.addStep(this, new Exception().getStackTrace());
} else {
Expand Down
27 changes: 27 additions & 0 deletions core/builder/src/test/java/io/quarkus/builder/BasicTests.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.quarkus.builder;

import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand Down Expand Up @@ -176,6 +177,32 @@ public void execute(final BuildContext context) {
assertFalse(ran.get());
}

@Test
public void testMissingProduces() {
final BuildChainBuilder builder = BuildChain.builder();
BuildStepBuilder stepBuilder = builder.addBuildStep(new BuildStep() {
@Override
public void execute(final BuildContext context) {
context.produce(new DummyItem());
}

@Override
public String getId() {
return "myBuildStepId";
}
});
stepBuilder.consumes(DummyItem.class);
assertThatThrownBy(stepBuilder::build)
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContainingAll(
"Build step 'myBuildStepId'",
"does not produce any build item and thus will never get executed",
"change the return type of the method to a build item type",
"add a parameter of type BuildProducer<[some build item type]>/Consumer<[some build item type]>",
"annotate the method with @Produces",
"Use @Produce(EmptyBuildItem.class) if you want to always execute this step");
}

@Test
public void testCircular() {
final BuildChainBuilder builder = BuildChain.builder();
Expand Down
93 changes: 9 additions & 84 deletions core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,6 @@ private static Consumer<BuildChainBuilder> loadStepsFromClass(Class<?> clazz,
for (Parameter parameter : ctorParameters) {
Type parameterType = parameter.getParameterizedType();
final Class<?> parameterClass = parameter.getType();
final boolean weak = parameter.isAnnotationPresent(Weak.class);
final boolean overridable = parameter.isAnnotationPresent(Overridable.class);
if (rawTypeExtends(parameterType, SimpleBuildItem.class)) {
final Class<? extends SimpleBuildItem> buildItemClass = rawTypeOf(parameterType)
.asSubclass(SimpleBuildItem.class);
Expand All @@ -349,44 +347,9 @@ private static Consumer<BuildChainBuilder> loadStepsFromClass(Class<?> clazz,
.asSubclass(MultiBuildItem.class);
stepConfig = stepConfig.andThen(bsb -> bsb.consumes(buildItemClass));
ctorParamFns.add(bc -> bc.consumeMulti(buildItemClass));
} else if (isConsumerOf(parameterType, BuildItem.class)) {
deprecatedProducer(parameter);
final Class<? extends BuildItem> buildItemClass = rawTypeOfParameter(parameterType, 0)
.asSubclass(BuildItem.class);
if (overridable) {
if (weak) {
stepConfig = stepConfig
.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.OVERRIDABLE, ProduceFlag.WEAK));
} else {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.OVERRIDABLE));
}
} else {
if (weak) {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.WEAK));
} else {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass));
}
}
ctorParamFns.add(bc -> (Consumer<? extends BuildItem>) bc::produce);
} else if (isBuildProducerOf(parameterType, BuildItem.class)) {
deprecatedProducer(parameter);
final Class<? extends BuildItem> buildItemClass = rawTypeOfParameter(parameterType, 0)
.asSubclass(BuildItem.class);
if (overridable) {
if (weak) {
stepConfig = stepConfig
.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.OVERRIDABLE, ProduceFlag.WEAK));
} else {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.OVERRIDABLE));
}
} else {
if (weak) {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.WEAK));
} else {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass));
}
}
ctorParamFns.add(bc -> (BuildProducer<? extends BuildItem>) bc::produce);
} else if (isConsumerOf(parameterType, BuildItem.class)
|| isBuildProducerOf(parameterType, BuildItem.class)) {
throw unsupportedConstructorOrFieldProducer(parameter);
} else if (isOptionalOf(parameterType, SimpleBuildItem.class)) {
final Class<? extends SimpleBuildItem> buildItemClass = rawTypeOfParameter(parameterType, 0)
.asSubclass(SimpleBuildItem.class);
Expand Down Expand Up @@ -447,8 +410,6 @@ private static Consumer<BuildChainBuilder> loadStepsFromClass(Class<?> clazz,
// next, determine the type
final Type fieldType = field.getGenericType();
final Class<?> fieldClass = field.getType();
final boolean weak = field.isAnnotationPresent(Weak.class);
final boolean overridable = field.isAnnotationPresent(Overridable.class);
if (rawTypeExtends(fieldType, SimpleBuildItem.class)) {
final Class<? extends SimpleBuildItem> buildItemClass = rawTypeOf(fieldType).asSubclass(SimpleBuildItem.class);
stepConfig = stepConfig.andThen(bsb -> bsb.consumes(buildItemClass));
Expand All @@ -464,44 +425,9 @@ private static Consumer<BuildChainBuilder> loadStepsFromClass(Class<?> clazz,
stepConfig = stepConfig.andThen(bsb -> bsb.consumes(buildItemClass));
stepInstanceSetup = stepInstanceSetup
.andThen((bc, o) -> ReflectUtil.setFieldVal(field, o, bc.consumeMulti(buildItemClass)));
} else if (isConsumerOf(fieldType, BuildItem.class)) {
deprecatedProducer(field);
final Class<? extends BuildItem> buildItemClass = rawTypeOfParameter(fieldType, 0).asSubclass(BuildItem.class);
if (overridable) {
if (weak) {
stepConfig = stepConfig
.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.OVERRIDABLE, ProduceFlag.WEAK));
} else {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.OVERRIDABLE));
}
} else {
if (weak) {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.WEAK));
} else {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass));
}
}
stepInstanceSetup = stepInstanceSetup
.andThen((bc, o) -> ReflectUtil.setFieldVal(field, o, (Consumer<? extends BuildItem>) bc::produce));
} else if (isBuildProducerOf(fieldType, BuildItem.class)) {
deprecatedProducer(field);
final Class<? extends BuildItem> buildItemClass = rawTypeOfParameter(fieldType, 0).asSubclass(BuildItem.class);
if (overridable) {
if (weak) {
stepConfig = stepConfig
.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.OVERRIDABLE, ProduceFlag.WEAK));
} else {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.OVERRIDABLE));
}
} else {
if (weak) {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass, ProduceFlag.WEAK));
} else {
stepConfig = stepConfig.andThen(bsb -> bsb.produces(buildItemClass));
}
}
stepInstanceSetup = stepInstanceSetup.andThen(
(bc, o) -> ReflectUtil.setFieldVal(field, o, (BuildProducer<? extends BuildItem>) bc::produce));
} else if (isConsumerOf(fieldType, BuildItem.class)
|| isBuildProducerOf(fieldType, BuildItem.class)) {
throw unsupportedConstructorOrFieldProducer(field);
} else if (isOptionalOf(fieldType, SimpleBuildItem.class)) {
final Class<? extends SimpleBuildItem> buildItemClass = rawTypeOfParameter(fieldType, 0)
.asSubclass(SimpleBuildItem.class);
Expand Down Expand Up @@ -1042,10 +968,9 @@ private static boolean isAnEmptyBuildItemConsumer(Type parameterType) {
|| isConsumerOf(parameterType, EmptyBuildItem.class);
}

private static void deprecatedProducer(final Object element) {
loadLog.warnf(
"Producing values from constructors and fields is no longer supported and will be removed in a future release: %s",
element);
private static IllegalArgumentException unsupportedConstructorOrFieldProducer(final AnnotatedElement element) {
return reportError(element, "Producing values from constructors or fields is no longer supported."
+ " Inject the BuildProducer/Consumer through arguments of relevant @BuildStep methods instead.");
}

protected static List<Method> getMethods(Class<?> clazz) {
Expand Down
2 changes: 1 addition & 1 deletion core/deployment/src/main/java/io/quarkus/deployment/annotations/Overridable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
* Indicate that the given produced item is produced only if no other steps produce that item.
*/
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER })
@Target({ ElementType.METHOD, ElementType.PARAMETER })
public @interface Overridable {
}
2 changes: 1 addition & 1 deletion core/deployment/src/main/java/io/quarkus/deployment/annotations/Weak.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@
* by a step, the step is not included. If applied to a method, the return value of the method is considered "weak".
*/
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER })
@Target({ ElementType.METHOD, ElementType.PARAMETER })
public @interface Weak {
}
3 changes: 3 additions & 0 deletions ...m/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
import io.quarkus.arc.deployment.UnremovableBeanBuildItem.BeanTypeExclusion;
import io.quarkus.arc.deployment.staticmethods.InterceptedStaticMethodsTransformersRegisteredBuildItem;
import io.quarkus.arc.processor.DotNames;
import io.quarkus.builder.item.EmptyBuildItem;
import io.quarkus.datasource.common.runtime.DataSourceUtil;
import io.quarkus.datasource.common.runtime.DatabaseKind;
import io.quarkus.deployment.Capabilities;
Expand All @@ -87,6 +88,7 @@
import io.quarkus.deployment.annotations.BuildProducer;
import io.quarkus.deployment.annotations.BuildStep;
import io.quarkus.deployment.annotations.Consume;
import io.quarkus.deployment.annotations.Produce;
import io.quarkus.deployment.annotations.Record;
import io.quarkus.deployment.builditem.AdditionalApplicationArchiveMarkerBuildItem;
import io.quarkus.deployment.builditem.AdditionalIndexedClassesBuildItem;
Expand Down Expand Up @@ -197,6 +199,7 @@ void registerHibernateOrmMetadataForCoreDialects(
}

@BuildStep
@Produce(EmptyBuildItem.class)
void checkTransactionsSupport(Capabilities capabilities) {
// JTA is necessary for blocking Hibernate ORM but not necessarily for Hibernate Reactive
if (capabilities.isMissing(Capability.TRANSACTIONS)
Expand Down
3 changes: 3 additions & 0 deletions ...eployment/src/main/java/io/quarkus/resteasy/reactive/links/deployment/LinksProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@
import org.jboss.resteasy.reactive.common.util.RestMediaType;

import io.quarkus.arc.deployment.AdditionalBeanBuildItem;
import io.quarkus.builder.item.EmptyBuildItem;
import io.quarkus.deployment.Capabilities;
import io.quarkus.deployment.Capability;
import io.quarkus.deployment.Feature;
import io.quarkus.deployment.GeneratedClassGizmoAdaptor;
import io.quarkus.deployment.annotations.BuildProducer;
import io.quarkus.deployment.annotations.BuildStep;
import io.quarkus.deployment.annotations.Produce;
import io.quarkus.deployment.annotations.Record;
import io.quarkus.deployment.builditem.BytecodeTransformerBuildItem;
import io.quarkus.deployment.builditem.FeatureBuildItem;
Expand Down Expand Up @@ -83,6 +85,7 @@ AdditionalBeanBuildItem registerRestLinksProviderProducer() {
}

@BuildStep
@Produce(EmptyBuildItem.class)
void validateJsonNeededForHal(Capabilities capabilities,
ResteasyReactiveResourceMethodEntriesBuildItem resourceMethodEntriesBuildItem) {
boolean isHalSupported = capabilities.isPresent(Capability.HAL);
Expand Down

0 comments on commit f33b980

Please sign in to comment.