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.

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

Use CDI to instantiate Application classes to invoke getClasses #2325

Merged
merged 3 commits into from
Sep 4, 2020
Merged

Use CDI to instantiate Application classes to invoke getClasses #2325

merged 3 commits into from
Sep 4, 2020

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Sep 3, 2020

Resolves #2324

Originally, the OpenAPI code instantiated Application classes directly so as to invoke their getClasses method.

This does not work if the Application injects a bean and uses the bean in the getClasses method.

This PR uses CDI's Unmanaged constructs to temporarily instantiate and inject each Application class, invoke getClasses, then dispose of the instance.

Thanks to @ljnelson for the idea of the solution.

…pplication instances during OpenAPI processing

Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
@tjquinno tjquinno self-assigned this Sep 3, 2020
Unmanaged.UnmanagedInstance<? extends Application> unmanagedInstance = unmanagedApp.newInstance();
Application app = unmanagedInstance.produce().inject().postConstruct().get();
unmanagedInstance.preDestroy().dispose();
return Optional.of(app);
Copy link
Member

@ljnelson ljnelson Sep 3, 2020

Choose a reason for hiding this comment

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

Since disposal will do nothing, this is probably OK, but it looks weird. I wonder if you can move this Unmanaged stuff elsewhere, like where you call something like getClasses() on the Application instance, and do the disposal afterwards. Like up around line 123 or so for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds as if your concern is that the use of the instance (the app) outlives the unmanagedInstance that gave rise to it.

I see how that does look a bit odd. But the app instance also outlives the code around line 123 and in fact is used not only in the same MPOpenAPIBuilder but also in a separate class OpenApiCdiExtension.

I'll take a look at refactoring things to address that admittedly odd-looking code without altering too much the way the consuming code operates. Thanks for the catch.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the main problem is that dependent objects that may get injected into the Application instance (and of course it's user-supplied so you don't have any control over it) may get flushed/released/disposed when that dispose() call is made. So you don't want them to go away obviously while the Application instance is in use somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest push rearranges the code so it uses any unmanaged instance as much as needed, and only then disposes of it.

…ication class, we use the instance as much as we need and then dispose of it
@tjquinno tjquinno requested a review from ljnelson September 4, 2020 15:51
Copy link
Member

@ljnelson ljnelson left a comment

Choose a reason for hiding this comment

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

Very clean; LGTM

@tjquinno tjquinno merged commit 721a185 into helidon-io:master Sep 4, 2020
@tjquinno tjquinno deleted the openapi-bug branch September 4, 2020 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MPOpenAPIBuilder calls Application.getClasses() with unresolved Injection points
2 participants