-
Notifications
You must be signed in to change notification settings - Fork 566
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
Remove need to instantiate Application classes twice for OpenAPI support #2829
Conversation
Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few type parameter related issues.
*/ | ||
public class OpenAPISupport implements Service { | ||
public abstract class OpenAPISupport<T extends OpenAPISupport<T, B>, B extends OpenAPISupport.Builder<T, B>> implements Service { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see the type B
used anywhere. (it also should not be).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type B
is used as the return type of Builder#me()
and all the public builder methods defined on the abstract class. Otherwise there is no way for the builder methods that are implemented on the abstract class to return this
correctly typed to the builder subclass without using unchecked casts. Or at least I have not seen one. If there is I would love to do away with me()
.
Returning the abstract Builder
type instead of the specific subclass type B
would lead to problems chaining calls of methods that are defined only on the subclass.
private final OpenApiStaticFile openApiStaticFile; | ||
private final Supplier<List<? extends IndexView>> indexViewsSupplier; | ||
|
||
protected OpenAPISupport(Builder builder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be Builder<?, ?>
so we do not have a Raw
use of class with type parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should not use the raw type. For the moment I've changed it to Builder<T, B>
to strongly type the parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ort (#2829) * Remove need to instantiate Application classes twice Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
…ort (helidon-io#2829) * Remove need to instantiate Application classes twice Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
Resolves #2799
If multiple
Application
classes exist in the user's app, we build separate OpenAPI models for each one. As part of that, we need to invoke eachApplication
'sgetClasses
andgetSingletons
methods. The OpenAPI CDI extension needs to act before the server's CDI extension actually creates theApplication
instances because the OpenAPI extension needs to adjust the routing to include the OpenAPI endpoint. But at that time, the server had not created theApplication
instances. So to invokegetClasses
andgetSingletons
the OpenAPI CDI extension would use CDI to create unmanaged instances of eachApplication
, invoke those methods, and then dispose of the unmanaged instances.This caused
@PostConstruct
methods to be invoked twice, once for the unmanaged instances and again when the server created the "live" instances. Developers found this disconcerting.Avoiding the double-instantiation involves splitting the OpenAPI CDI extension's work into two steps: first, creating the
OpenAPISupport
object so its routing can be plugged in correctly, and second later on when theApplication
instances are available for use in building the in-memory OpenAPI model.To do that involved refactoring the model-building logic from the
Builder
class to theOpenAPISupport
class.Allowing the MP CDI extension to prepare the model during start-up (rather than lazily upon first request for the OpenAPI document), but without exposing a
prepareModel()
method aspublic
involved creating SE and MP variants of theOpenAPISupport
class. The now-abstract superclassOpenAPISupport
exposes aprotected prepareModel()
method which the MP variant overrides, also asprotected
, so the MP CDI extension in the same package can invoke it.And doing that in a type-safe way requires using type parameters on the
OpenAPISupport
class and its builder, which brought along another set of changes.As a result, there is a seemingly large number of changes required for a conceptually simple improvement in user experience, but doing it this way avoids expanding the public surface area more than we want.
Signed-off-by: tim.quinn@oracle.com tim.quinn@oracle.com