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

Copy the headers from REST requests to the corresponding TransportRequest(s) #6513

Closed

Conversation

Projects
None yet
5 participants
@javanna
Copy link
Member

javanna commented Jun 16, 2014

Introduced the use of the FilterClient in all of the REST actions, which delegates all of the operations to the internal Client, but makes sure that the headers are properly copied if needed from REST requests to TransportRequest(s) when it comes to executing them. Added new abstract handleRequest method to BaseRestHandler with additional Client argument and made private the client instance member (was protected before) to force the use of the client received as argument. The list of headers to be copied over is by default empty but can be extended via plugins.

Replaces #6464 as it provides a better and safer way to copy headers from REST layer to transport layer.

@uboness

View changes

src/main/java/org/elasticsearch/rest/HeadersCopyClient.java Outdated
* but makes sure that the REST headers are copied over from {@link org.elasticsearch.rest.RestRequest} to the
* corresponding {@link org.elasticsearch.action.ActionRequest}(s)
*/
public class HeadersCopyClient extends AbstractClient {

This comment has been minimized.

Copy link
@uboness

uboness Jun 16, 2014

Contributor

I think this one should be an inner class in BaseRestHandler

This comment has been minimized.

Copy link
@uboness

uboness Jun 16, 2014

Contributor

there should probably be a common client for all client types... and then it'll just work with whatever client

This comment has been minimized.

Copy link
@javanna

javanna Jun 17, 2014

Author Member

even after #6517 this is not so easy to achieve keeping type-safety at the same time, because the Client generic type which can either be Client, ClusterAdminClient or IndicesAdminClient. I'm opting for duplicating the execute methods, it's still less LOC than trying to generalize this so that we have a single pair of execute methods.

REST api: made sure that the REST headers are copied over from REST r…
…equests to the corresponding TransportRequest(s)

Introduced the use of a FilterClient in all of the REST actions, which delegates all of the operations to the internal Client, but makes sure that the headers are properly copied from REST requests to TransportRequest(s) when it comes to executing them.

Closes #6513
@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jun 17, 2014

I updated the PR after #6517 was committed, ready for review again...

}

@Override
public IndicesAdminClient indices() {

This comment has been minimized.

Copy link
@uboness

uboness Jun 17, 2014

Contributor

Can we keep the refs to these internal clients

This comment has been minimized.

Copy link
@javanna

javanna Jun 17, 2014

Author Member

makes sense, just pushed a new commit that addresses this

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jun 18, 2014

I think this looks awesome. I am hesitating with the BW break here, IMO it would be nicer to deprecate the BaseRestHandler#client and add a method BaseRestHandler#client(RestRequest) instead of adding a new abstract method. We can then make the member private in master and drop the deprecation?

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Jun 18, 2014

does it make sense when we copy over the headers, to not copy over common known HTTP headers that are not really used, like Content-Type, Content-Length, ...

@uboness

This comment has been minimized.

Copy link
Contributor

uboness commented Jun 18, 2014

@s1monw I think it's actually better to pass in the client as an arg to handleRequest as it "encourage" handler implementations to use the client. Nothing really prevents handler impl. from injecting transport actions directly and bypass the client all together. The only bwc breaking we have here, relates to plugins, and we're breaking bwc for plugins almost every other release (e.g. when we upgrade lucene version).. I think this change is important enough to justify it.

@kimchy yea, it makes sense +1

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jun 18, 2014

fair enough... LGTM

@s1monw s1monw removed the review label Jun 18, 2014

@javanna javanna changed the title REST api: made sure that the REST headers are copied over from REST requests to the corresponding TransportRequest(s) REST api: made it possible to copy the REST headers over from REST requests to the corresponding TransportRequest(s) Jun 18, 2014

@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jun 18, 2014

Pushed a new commit containing a static list of REST headers that need to be copied over to the transport.

*/
public abstract class BaseRestHandler extends AbstractComponent implements RestHandler {

protected final Client client;
private static final Set<String> usefulHeaders = new HashSet<>();

This comment has been minimized.

Copy link
@uboness

uboness Jun 18, 2014

Contributor

this should probably be an array

}

private void copyHeaders(ActionRequest request) {
for (Map.Entry<String, String> header : restRequest.headers()) {

This comment has been minimized.

Copy link
@uboness

uboness Jun 18, 2014

Contributor

I'd go the other way around

for (int i = 0; i < usefulHeaders.length; i++) {
   request.putHeader(userfulHeaders[i], restRequest.header(usefulHeader[i]));
}

javanna added some commits Jun 18, 2014

changed the way the headers are copied, iterate over useful headers only
updated tests too to have useful headers that are not part of the rest request
replaced Set<String> with String[], added test to make sure the stati…
…c addUsefulHeader works (from multiple threads as well)
@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jun 18, 2014

Pushed a couple of new commits that address @uboness feedback, it should be close.

@uboness

This comment has been minimized.

Copy link
Contributor

uboness commented Jun 19, 2014

LGTM

javanna added a commit that referenced this pull request Jun 19, 2014

REST api: made it possible to copy the REST headers from REST request…
…s to the corresponding TransportRequest(s)

Introduced the use of the FilterClient in all of the REST actions, which delegates all of the operations to the internal Client, but makes sure that the headers are properly copied if needed from REST requests to TransportRequest(s) when it comes to executing them.

Added new abstract handleRequest method to BaseRestHandler with additional Client argument and made private the client instance member (was protected before) to force the use of the client received as argument.

The list of headers to be copied over is by default empty but can be extended via plugins.

Closes #6513

@javanna javanna closed this in 12fd6ce Jun 19, 2014

@javanna

This comment has been minimized.

Copy link
Member Author

javanna commented Jun 19, 2014

As a side note, this change is breaking for plugins that add their own REST handlers by extending BaseRestHandler. They can easily adapt just by adding an additional Client argument to their existing handleRequest method and use that Client received as argument to execute the request instead of this.client.

@clintongormley clintongormley changed the title REST api: made it possible to copy the REST headers over from REST requests to the corresponding TransportRequest(s) REST API: Copy the headers from REST requests to the corresponding TransportRequest(s) Jul 16, 2014

javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 4, 2014

Internal: deduplicate useful headers that get copied from REST to tra…
…nsport requests

The useful headers are now stored into a `Set` instead of an array so we can easily deduplicate them. A set is also returned instead of an array by the `usefulHeaders` static getter.

Relates to elastic#6513

Closes elastic#7590

javanna added a commit that referenced this pull request Sep 4, 2014

Internal: deduplicate useful headers that get copied from REST to tra…
…nsport requests

The useful headers are now stored into a `Set` instead of an array so we can easily deduplicate them. A set is also returned instead of an array by the `usefulHeaders` static getter.

Relates to #6513

Closes #7590

javanna added a commit that referenced this pull request Sep 4, 2014

Internal: deduplicate useful headers that get copied from REST to tra…
…nsport requests

The useful headers are now stored into a `Set` instead of an array so we can easily deduplicate them. A set is also returned instead of an array by the `usefulHeaders` static getter.

Relates to #6513

Closes #7590

javanna added a commit that referenced this pull request Sep 8, 2014

Internal: deduplicate useful headers that get copied from REST to tra…
…nsport requests

The useful headers are now stored into a `Set` instead of an array so we can easily deduplicate them. A set is also returned instead of an array by the `usefulHeaders` static getter.

Relates to #6513

Closes #7590

javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 9, 2014

Internal: refactor copy headers mechanism
The functionality of copying headers in the REST layer (from REST requests to transport requests) remains the same. Made it a bit nicer by introducing a ClientFactory component that is a singleton and allows to register useful headers without requiring static methods.

Plugins just have to inject the ClientFactory now, and call its `addUsefulHeaders` method that is not static anymore.

Relates to elastic#6513
Closes elastic#7594

javanna added a commit that referenced this pull request Sep 10, 2014

Internal: refactor copy headers mechanism
The functionality of copying headers in the REST layer (from REST requests to transport requests) remains the same. Made it a bit nicer by introducing a RestClientFactory component that is a singleton and allows to register useful headers without requiring static methods.

Plugins just have to inject the RestClientFactory now, and call its `addRelevantHeaders` method that is not static anymore.

Relates to #6513
Closes #7594

javanna added a commit that referenced this pull request Sep 10, 2014

Internal: refactor copy headers mechanism
The functionality of copying headers in the REST layer (from REST requests to transport requests) remains the same. Made it a bit nicer by introducing a RestClientFactory component that is a singleton and allows to register useful headers without requiring static methods.

Plugins just have to inject the RestClientFactory now, and call its `addRelevantHeaders` method that is not static anymore.

Relates to #6513
Closes #7594

@clintongormley clintongormley changed the title REST API: Copy the headers from REST requests to the corresponding TransportRequest(s) Copy the headers from REST requests to the corresponding TransportRequest(s) Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.