Remove @Inject from rest handlers #15687
Conversation
nik9000
added
review
:Core/Infra/REST API
|
I'm still running tests now. On a slow machine today.... |
|
We just got rid of RestModule! Why can't the rest actions be created in NetworkModule? |
I guess it could. I pulled them out initially because I wanted to work on rest stuff in isolation from the transport stuff. I can smoosh it back together but I kind of like rest stuff registered in a RestModule and transport stuff NetworkModule. NetworkModule just looks so much more cohesive now. |
The entire point of the refactoring that removed RestModule was Rest is really network. Transport and rest requests aren't really any different, they all have to do with communicating with ES over the network (and while the transport actions are not yet registered in NetworkModule, I mentioned in the PR that it was my intention to do that in a follow up). |
while I agree I think we can do this in a followup? This is already awesome and worth a push as it is? :) |
|
oh nevermind, now I see it's adding RestModule back, yeah I think we should never ever again add modules back, can you just fold it into networkModule? |
| public <T extends BaseRestHandler> void add(Class<T> type, Function<RestGlobalContext, T> builder) { | ||
| requireNonNull(type, "Must define the handler type being registered"); | ||
| requireNonNull(builder, "Must define the builder to register"); | ||
| Object old = actions.put(type, builder); |
There was a problem hiding this comment.
nit: use putIfAbsent then you don't modify even if it's there?
I'll do it but I don't get it. NetworkModule without the rest stuff it just sets up the transport implementations and leaves the ticklish bits to something else. But its a separate thing. I'll put it back. |
|
I believe I've addressed all the comments, @s1monw. |
|
Rebased. Sorry for the trouble. |
|
I'm going to rebase again. Any interest in another review? |
Do this by creating RestModule which has a single @Inject. This gives us a string to pull on: move parameters from injected to constructor. This is an incremental step to cleaning up our reliance on guice and should be minimally invasive. Closes elastic#15685
Moves registration from the REST handlers into NetworkModule. The handlers now declare where they'd like to be registered and NetworkModule picks it up and registers them. This also fixes a bug I found in ordering of startup: if a plugin depended on some guice injected resource to build the rest handler then it wouldn't have the resource ready. So I push the creation of the rest handlers to after all guice injection is complete.
Removed BaseSingleMethodRestHandler and renamed BaseMultiMethodRestHandler to BaseStandardRegistrationsRestHandler. You can now extend just the one class and get either behavior depending on the flavor of constructor used. Reverted the name change to the registerTransportHandler method. Add SettingsFilter to NetworkModule's constructor.
Removes more getters in favor of pushing the HeadersAndContextCopyClient into the global context. BaseRestHandler is now very very simple.
nik9000
force-pushed
the
rest_no_guice
branch
from
31f6b8f to
1ef2eac
Compare
|
Rebased! |
|
This is super out of date now. |
Do this by adding a single @Inject to NetworkModule. This gives us
a string to pull on: move parameters from injected to constructor.
This is an incremental step to cleaning up our reliance on guice and should
be minimally invasive.
Closes #15685