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

Re-enable MutinyTest #11828

Merged
merged 2 commits into from
Sep 4, 2020
Merged

Re-enable MutinyTest #11828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e77962
Re-enable MutinyTest
kenfinnigan Sep 2, 2020
389aea5
Re-enable Mongodb Reactive with Panache test
kenfinnigan Sep 4, 2020
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
5 changes: 5 additions & 0 deletions ...client/deployment/src/main/java/io/quarkus/restclient/deployment/RestClientProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ void setupProviders(BuildProducer<NativeImageResourceBuildItem> resources,
resources.produce(new NativeImageResourceBuildItem(PROVIDERS_SERVICE_FILE));
}

@BuildStep
void setupClientBuilder(BuildProducer<NativeImageResourceBuildItem> resources) {
resources.produce(new NativeImageResourceBuildItem("META-INF/services/javax.ws.rs.client.ClientBuilder"));
}

@Record(ExecutionTime.STATIC_INIT)
@BuildStep
BeanContainerListenerBuildItem fixExtension(RestClientRecorder restClientRecorder) {
Expand Down
29 changes: 20 additions & 9 deletions ...s/rest-client/runtime/src/main/java/io/quarkus/restclient/runtime/RestClientRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,25 +59,36 @@ public InjectorFactory getInjectorFactory() {
}
};

if (useBuiltIn) {
RegisterBuiltin.register(clientProviderFactory);
registerProviders(clientProviderFactory, contributedProviders, false);
registerProviders(clientProviderFactory, useBuiltIn, providersToRegister, contributedProviders);

if (ResteasyProviderFactory.peekInstance() != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Does the execution order play a role here? @jaikiran reported that sometimes the server one was initialized first and sometimes the client one.

I think we should have a build item ensuring that the order of operations is fully determined.

Copy link
Member Author

Choose a reason for hiding this comment

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

To my understanding, the only time there's a problem is when the server one is created first and the REST Client one clobbers it.

I don't think the reverse situation poses any problems.

In addition, there are many times where only the server or only REST Client is present, so I'm not sure how a BuildItem helps in that situation

Copy link
Member Author

Choose a reason for hiding this comment

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

Another point to note is that the provider factory for server or client side are actually different instances of the same class, where the "server" instance has a no-op for all client operations, and the "client" instance the reverse.

Copy link
Member

Choose a reason for hiding this comment

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

We could have a build item in one of the artifacts common to the two, the server would produce it and the client optionally consume it.

This way, if the server is present, we would be sure the client code would be executed after the server code.

Obviously, we can also do the opposite if it makes more sense.

Copy link
Member Author

@kenfinnigan kenfinnigan Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Another option, which will take some time, is to add the functionality we have for RestClientBuilderImpl to ResteasyClientBuilderImpl that can use a static method to set the provider directly into the instance.

Then ordering isn't an issue.

I've raised https://issues.redhat.com/browse/RESTEASY-2684 to discuss this option with the RESTEasy team

Copy link
Member

Choose a reason for hiding this comment

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

Again, I want to be sure we properly control the execution order.

@kenfinnigan I can do the work and push an additional commit to your PR if you don't think it's useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see it as necessary or useful

Copy link
Member Author

Choose a reason for hiding this comment

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

To reiterate, with this change it's not necessary to control the execution order. And I'm looking to properly fix it in RESTEasy so this wouldn't be needed long term either.

I don't see the need to further complicate the code for a change that isn't permanent

Copy link
Member

Choose a reason for hiding this comment

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

And I'm looking to properly fix it in RESTEasy so this wouldn't be needed long term either.

Yeah but that won't be for 1.8 and we need a fix now.

And I really think having a deterministic execution order for things that interact with each other is important.

If just for the sake of not spending hours debugging weird issues.

I think adding the build item should be very easy. I'll take a look tomorrow. Either it's easy and don't add boilerplate or I will give up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it won't be done for 1.8, but why add more code that needs to be removed when it's there in 1.9?

When the change is done in RESTEasy, there is no longer a need for a build item

ResteasyProviderFactory serverProviderFactory = ResteasyProviderFactory.getInstance();
registerProviders(serverProviderFactory, useBuiltIn, providersToRegister, contributedProviders);
} else {
providersToRegister.removeAll(contributedProviders);
registerProviders(clientProviderFactory, providersToRegister, true);
registerProviders(clientProviderFactory, contributedProviders, false);
ResteasyProviderFactory.setInstance(clientProviderFactory);
}

RestClientBuilderImpl.setProviderFactory(clientProviderFactory);
ResteasyProviderFactory.setInstance(clientProviderFactory);
providerFactory = clientProviderFactory;
}

private static void registerProviders(ResteasyProviderFactory clientProviderFactory, Set<String> providersToRegister,
private static void registerProviders(ResteasyProviderFactory providerFactory, boolean useBuiltIn,
Set<String> providersToRegister,
Set<String> contributedProviders) {
if (useBuiltIn) {
RegisterBuiltin.register(providerFactory);
} else {
providersToRegister.removeAll(contributedProviders);
registerProviders(providerFactory, providersToRegister, true);
}
registerProviders(providerFactory, contributedProviders, false);
}

private static void registerProviders(ResteasyProviderFactory providerFactory, Set<String> providersToRegister,
Boolean isBuiltIn) {
for (String providerToRegister : providersToRegister) {
try {
clientProviderFactory
providerFactory
.registerProvider(Thread.currentThread().getContextClassLoader().loadClass(providerToRegister.trim()),
isBuiltIn);
} catch (ClassNotFoundException e) {
Expand Down
6 changes: 0 additions & 6 deletions ...main/java/io/quarkus/resteasy/server/common/deployment/ResteasyServerCommonProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,8 @@
import io.quarkus.deployment.builditem.CombinedIndexBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageConfigBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageProxyDefinitionBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageResourceBuildItem;
import io.quarkus.deployment.builditem.nativeimage.ReflectiveClassBuildItem;
import io.quarkus.deployment.builditem.nativeimage.ReflectiveHierarchyBuildItem;
import io.quarkus.deployment.builditem.nativeimage.RuntimeInitializedClassBuildItem;
import io.quarkus.gizmo.Gizmo;
import io.quarkus.resteasy.common.deployment.JaxrsProvidersToRegisterBuildItem;
import io.quarkus.resteasy.common.deployment.ResteasyCommonProcessor.ResteasyCommonConfig;
Expand Down Expand Up @@ -164,8 +162,6 @@ public void build(
BuildProducer<ReflectiveClassBuildItem> reflectiveClass,
BuildProducer<ReflectiveHierarchyBuildItem> reflectiveHierarchy,
BuildProducer<NativeImageProxyDefinitionBuildItem> proxyDefinition,
BuildProducer<NativeImageResourceBuildItem> resource,
BuildProducer<RuntimeInitializedClassBuildItem> runtimeClasses,
BuildProducer<BytecodeTransformerBuildItem> transformers,
BuildProducer<ResteasyServerConfigBuildItem> resteasyServerConfig,
BuildProducer<ResteasyDeploymentBuildItem> resteasyDeployment,
Expand All @@ -184,8 +180,6 @@ public void build(
CustomScopeAnnotationsBuildItem scopes) throws Exception {
IndexView index = combinedIndexBuildItem.getIndex();

resource.produce(new NativeImageResourceBuildItem("META-INF/services/javax.ws.rs.client.ClientBuilder"));

Collection<AnnotationInstance> applicationPaths = index.getAnnotations(ResteasyDotNames.APPLICATION_PATH);

// currently we only examine the first class that is annotated with @ApplicationPath so best
Expand Down
2 changes: 0 additions & 2 deletions .../test/java/io/quarkus/it/mongodb/panache/reactive/ReactiveMongodbPanacheResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import javax.ws.rs.sse.SseEventSource;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;

import com.fasterxml.jackson.databind.ObjectMapper;
Expand All @@ -43,7 +42,6 @@

@QuarkusTest
@QuarkusTestResource(MongoTestResource.class)
@Disabled("See https://github.com/quarkusio/quarkus/issues/11711")
Copy link
Member

Choose a reason for hiding this comment

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

There's also NativeMongodbPanacheResourceIT in integration-tests/mongodb-panache which has been disabled for this same reason. That one needs to be re-enabled too, either in this PR or as a separate one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, missed that, thought it was just the one.

I'm loathed to re-trigger the full CI build, can it be in a followup if this is ok?

Copy link
Member

Choose a reason for hiding this comment

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

I'm loathed to re-trigger the full CI build,

Same here.

can it be in a followup if this is ok?

I think that's OK since if this re-enabled regular test passes, it's almost guaranteed the native one too will, given the context of this issue. So we just need to see how this one goes.

class ReactiveMongodbPanacheResourceTest {
private static final TypeRef<List<BookDTO>> LIST_OF_BOOK_TYPE_REF = new TypeRef<List<BookDTO>>() {
};
Expand Down
2 changes: 0 additions & 2 deletions ...gration-tests/resteasy-mutiny/src/test/java/io/quarkus/it/resteasy/mutiny/MutinyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import javax.ws.rs.sse.SseEventSource;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;

import io.quarkus.test.junit.QuarkusTest;
Expand Down Expand Up @@ -83,7 +82,6 @@ public void testMultiWithSerialization() {
}

@Test
@Disabled("https://github.com/quarkusio/quarkus/issues/11687")
public void testSSE() {
Client client = ClientBuilder.newClient();
WebTarget target = client.target("http://localhost:" + RestAssured.port + "/mutiny/pets");
Expand Down