Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken collection assignability check #36197

Merged
merged 1 commit into from
Sep 28, 2023
Merged

Fix broken collection assignability check #36197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 5 additions & 19 deletions ...c/main/java/io/quarkus/jaxrs/client/reactive/deployment/JaxrsClientReactiveProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
import static org.jboss.jandex.Type.Kind.PRIMITIVE;
import static org.jboss.resteasy.reactive.client.impl.RestClientRequestContext.DEFAULT_CONTENT_TYPE_PROP;
import static org.jboss.resteasy.reactive.common.processor.EndpointIndexer.extractProducesConsumesValues;
import static org.jboss.resteasy.reactive.common.processor.JandexUtil.*;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.COLLECTION;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.COMPLETION_STAGE;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.CONSUMES;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.ENCODED;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.FORM_PARAM;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.MAP;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.MULTI;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.OBJECT;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.PART_TYPE_NAME;
Expand Down Expand Up @@ -2744,28 +2747,11 @@ private void addQueryParamToWebTarget(BytecodeCreator creator, ResultHandle para
}

private boolean isCollection(Type type, IndexView index) {
if (type.kind() == Type.Kind.PRIMITIVE) {
return false;
}
ClassInfo classInfo = index.getClassByName(type.name());
if (classInfo == null) {
return false;
}
return classInfo.interfaceNames().stream().anyMatch(DotName.createSimple(Collection.class.getName())::equals);
return isAssignableFrom(COLLECTION, type.name(), index);
}

private boolean isMap(Type type, IndexView index) {
if (type.kind() == Type.Kind.PRIMITIVE) {
return false;
}
ClassInfo classInfo = index.getClassByName(type.name());
if (classInfo == null) {
return false;
}
if (ResteasyReactiveDotNames.MAP.equals(classInfo.name())) {
return true;
}
return classInfo.interfaceNames().stream().anyMatch(DotName.createSimple(Map.class.getName())::equals);
return isAssignableFrom(MAP, type.name(), index);
}

private void addHeaderParam(BytecodeCreator invoBuilderEnricher, AssignableResultHandle invocationBuilder,
Expand Down
13 changes: 3 additions & 10 deletions ...nt/src/main/java/io/quarkus/resteasy/reactive/links/deployment/LinksContainerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package io.quarkus.resteasy.reactive.links.deployment;

import static org.jboss.resteasy.reactive.common.processor.JandexUtil.isAssignableFrom;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.COLLECTION;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.COMPLETABLE_FUTURE;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.COMPLETION_STAGE;
import static org.jboss.resteasy.reactive.common.processor.ResteasyReactiveDotNames.MULTI;
Expand All @@ -17,8 +19,6 @@

import org.jboss.jandex.AnnotationInstance;
import org.jboss.jandex.AnnotationValue;
import org.jboss.jandex.ClassInfo;
import org.jboss.jandex.DotName;
import org.jboss.jandex.IndexView;
import org.jboss.jandex.MethodInfo;
import org.jboss.jandex.ParameterizedType;
Expand Down Expand Up @@ -138,14 +138,7 @@ private String getAnnotationValue(AnnotationInstance annotationInstance, String
}

private boolean isCollection(Type type, IndexView index) {
if (type.kind() == Type.Kind.PRIMITIVE) {
return false;
}
ClassInfo classInfo = index.getClassByName(type.name());
if (classInfo == null) {
return false;
}
return classInfo.interfaceNames().stream().anyMatch(DotName.createSimple(Collection.class.getName())::equals);
return isAssignableFrom(COLLECTION, type.name(), index);
}

private Type getNonAsyncReturnType(Type returnType) {
Expand Down
40 changes: 40 additions & 0 deletions ...mmon/processor/src/main/java/org/jboss/resteasy/reactive/common/processor/JandexUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
import java.lang.reflect.Modifier;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -415,4 +417,42 @@ public static boolean isImplementorOf(IndexView index, ClassInfo info, DotName n
return isImplementorOf(index, superClass, name, additionalIgnoredSuperClasses);
}

public static boolean isAssignableFrom(DotName superType, DotName subType, IndexView index) {
// java.lang.Object is assignable from any type
if (superType.equals(DOTNAME_OBJECT)) {
return true;
}
// type1 is the same as type2
if (superType.equals(subType)) {
return true;
}
// type1 is a superclass
return findSupertypes(subType, index).contains(superType);
}

private static Set<DotName> findSupertypes(DotName name, IndexView index) {
Set<DotName> result = new HashSet<>();

Deque<DotName> workQueue = new ArrayDeque<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit I've never seen this sort of implementation. Usually it's a recursive function. I assume you're flattening it on purpose to save stack space?
Also, this is not efficient because you will iterate through the work queue's inheritance entirely to look for all supertypes instead of stopping as soon as you find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pretty much copied this from Arc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can of course optimize, but I didn't want to in the remote case we can at some point merge the various util classes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I figured this came from somewhere :( I also wondered why it was not using the existing JandexUtil from quarkus-core-deployment, but this is not usable from independent-projects, same as for Arc. Interestingly, that last util class has isSubclassOf but not for interfaces, which means it must already exist in many forms elsewhere.

@mkouba any reason why Arc's isAssignableFrom uses findSupertypes instead of stopping on success?

Copy link
Contributor

@Ladicek Ladicek Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote this in ArC actually, so I can comment on this. The important thing that ArC does and this utility class does not is memoization. In ArC, we remember the full set of supertypes for each class, which allows us to later answer the "is subtype of" queries instantly -- we do the traversal just once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See io.quarkus.arc.processor.AssignabilityCheck.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the work queue algorithm is very common in compilers, I just mangled the name a long time ago and it kinda stuck. In compilers, it is commonly known as "worklist".

workQueue.add(name);
while (!workQueue.isEmpty()) {
DotName type = workQueue.poll();
if (result.contains(type)) {
continue;
}
result.add(type);

ClassInfo clazz = index.getClassByName(type);
if (clazz == null) {
continue;
}
if (clazz.superName() != null) {
workQueue.add(clazz.superName());
}
workQueue.addAll(clazz.interfaceNames());
}

return result;
}

}
Loading