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

Added an option to add arbitrary headers to the client requests #7127

Closed

Conversation

Projects
None yet
3 participants
@uboness
Copy link
Contributor

commented Aug 1, 2014

The headers are key/value pairs defined in the settings under the request.headers namespace.

Also changed test infrastructure's so it'd be possible to define the additional settings for transport clients.

@javanna

View changes

src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java Outdated
// TODO this is a really shitty way to test it, we need to figure out a way to test all the client methods
// without specifying each one (reflection doesn't as each action needs its own special settings, without
// them, request validation will fail before the test is executed. (one option is to enable disabling the
// validation in the settings??? - ugly and conceptually wrong)

This comment has been minimized.

Copy link
@javanna

javanna Aug 1, 2014

Member

yeah...been there...done that... :) even if you disable requests validation you'll run into problems when serializing the request (writeTo method). Can't you use a fake request so that validation and serialization are not a problem, and then call execute and check that requests get set? Not sure it is really needed to check every single request at this point?

I'd love to see a unit test around this though, I don't think this necessarily requires a running node, you could just create your own mock transport service that checks that each request contains the headers (you don't have to send the request for real). You could make it work without even requiring a transport client but by testing InternalTransportClient as I did here: https://github.com/elasticsearch/elasticsearch/blob/master/src/test/java/org/elasticsearch/client/transport/InternalTransportClientTests.java .

Also, I'm a bit hesitant on the changes to the test infrastructure that are needed for this single test. I'd rather prefer to have a manually created transport client here with the settings, or not even require a complete transport client if you go for the unit test.

This comment has been minimized.

Copy link
@uboness

uboness Aug 1, 2014

Author Contributor

yeah...been there...done that... :) even if you disable requests validation you'll run into problems when serializing the request (writeTo method). Can't you use a fake request so that validation and serialization are not a problem, and then call execute and check that requests get set? Not sure it is really needed to check every single request at this point?

actually, without validation reflection approach would work (the mock transport shortcuts execution before requests are serialized). But enabling an option to disable validation, just for testing, feels wrong (there's no scenario anyone should be able to do this outside of testing)

I'd love to see a unit test around this though, I don't think this necessarily requires a running node, you could just create your own mock transport service that checks that each request contains the headers (you don't have to send the request for real). You could make it work without even requiring a transport client but by testing InternalTransportClient as I did here:

fair enough... I'll see if I can change the test to not depend on the cluster at all

Also, I'm a bit hesitant on the changes to the test infrastructure that are needed for this single test. I'd rather prefer to have a manually created transport client here with the settings, or not even require a complete transport client if you go for the unit test.

This is actually a good extension for the testing infra. it should be possible to customize the configuration of transport clients (right now it's not possible). But I agree that if this PR won't depend on the cluster infra, it's better to create a separate PR for it

This comment has been minimized.

Copy link
@javanna

javanna Aug 3, 2014

Member

Regarding the testing infra extension, the one thing I don't get is where we would need to use it. If it's needed in some tests (other than this) I'm +1 on adding it, otherwise I don't quite see the point. Am I missing something?

This comment has been minimized.

Copy link
@uboness

uboness Aug 3, 2014

Author Contributor

well... the point is that configuring nodes and configuring transport clients are two different things. Right now, the transport client configuration is randomized internally without the ability to modify it.. you might want to have your own custom settings for the client. The internal randomization is still there, but at least have the ability to override/force certain settings.

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

I understand the difference between the two...just saying that we are adding an extension point that is currently not used. But if we plan on using it in the future, thats' great. Just reluctant to add complexity when we don't need it.

@javanna

View changes

src/main/java/org/elasticsearch/transport/TransportService.java Outdated
@@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableMap;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchIllegalStateException;
import org.elasticsearch.action.admin.cluster.health.ClusterHealthAction;

This comment has been minimized.

Copy link
@javanna

javanna Aug 1, 2014

Member

I think this import is not needed here?

This comment has been minimized.

Copy link
@uboness

uboness Aug 1, 2014

Author Contributor

oops...

@javanna

View changes

src/test/java/org/elasticsearch/test/InternalTestCluster.java Outdated
@@ -588,6 +589,17 @@ public synchronized Client clientNodeClient() {
return getRandomNodeAndClient(new ClientNodePredicate()).client(random);
}

public synchronized Client startNodeClient(Settings settings) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 1, 2014

Member

this method was only moved up? was any change needed in it?

This comment has been minimized.

Copy link
@uboness

uboness Aug 1, 2014

Author Contributor

yeah.. move it to group together all client related methods

@javanna

View changes

src/main/java/org/elasticsearch/client/support/Headers.java Outdated
headers = resolveHeaders(settings);
}

public void set(ActionRequest request) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 1, 2014

Member

I find this method a bit confusing cause it sets stuff on the argument. I'd maybe add a putHeaders method to ActionRequest and make sure we can do it on a single line where we need to do it. The argument of the putHeader method could either be the Headers object itself, or we could make this Iterable or something and pass the key-vaue pairs to the action request.

@javanna

View changes

src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java Outdated
assertThat(headers.get("key2").toString(), equalTo("val 2"));
}
}
}

This comment has been minimized.

Copy link
@javanna

javanna Aug 1, 2014

Member

I think we are missing a test around the node client and co.? or did I miss it?

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 1, 2014

Left some comments, mainly around testing, which is indeed quite a challenge here but I think we can work on it

@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2014

updated... change the test to no depend on the internal cluster infrastructure, but there's till room for improvement

@uboness uboness added v2.0.0 and removed enhancement labels Aug 2, 2014

headers = resolveHeaders(settings);
}

public void applyTo(ActionRequest request) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 3, 2014

Member

I like applyTo better than set! Out of curiosity, any specific reason why you don't want to move this logic to the ActionRequest?

This comment has been minimized.

Copy link
@uboness

uboness Aug 3, 2014

Author Contributor

not sure where you want to put it... the headers are injected on the clients (it's a client only feature)

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

I see your point, fair enough

@javanna

View changes

src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java Outdated
return;
}
logger.info("*** action: " + action);
handler.handleException(new InternalException(action, request.getHeaders()));

This comment has been minimized.

Copy link
@javanna

javanna Aug 3, 2014

Member

Can't we just return a response instead of an exception? And even move the whole assert logic here so we get rid of both exception and listener?

This comment has been minimized.

Copy link
@uboness

uboness Aug 3, 2014

Author Contributor

with an exception, you're not dependent on the action that is executed, this allows testing different actions regardless of their specific responses

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

Ok I see the problem... I still find this hackish (needing to throw an exception to test things), but there's no easy way around it if we want to test different requests and responses.

I'd consider using a mock request (one per client type actually) instead and give up on testing those real requests and responses. It would be more unit test friendly cause you'd know the request and the response you need to return (unless it's a nodes info request), you don't need an exception and you can assert on the sendRequest directly. Using real requests it feels wrong to only test a few of them anyway and we know that it's the client that injects the headers (execute method), that's what we need to test.

@javanna

View changes

src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java Outdated
}

}
}

This comment has been minimized.

Copy link
@javanna

javanna Aug 3, 2014

Member

We are still missing a test for the node client case, aren't we?

This comment has been minimized.

Copy link
@uboness

uboness Aug 3, 2014

Author Contributor

correct... I just figured a way to test the node client... will updated the PR (quite substantial update as now the two test classes - for node client and transport client - will share code)... PR to come soon

@javanna javanna removed the review label Aug 3, 2014

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 3, 2014

Left a couple comments, again around testing , but this looks already much better ;)

@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2014

@javanna updated... have a look

@javanna

View changes

src/test/java/org/elasticsearch/client/AbstractClientHeadersTests.java Outdated
*/
public class AbstractClientHeadersTests extends ElasticsearchTestCase {

protected static final Settings HEADER_SETTGINS = ImmutableSettings.builder()

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

s/HEADER_SETTGINS/HEADER_SETTINGS

@javanna

View changes

src/test/java/org/elasticsearch/client/AbstractClientHeadersTests.java Outdated
/**
*
*/
public class AbstractClientHeadersTests extends ElasticsearchTestCase {

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

this should be an abstract class, not sure if we also need the @Ignore annotation.

This comment has been minimized.

Copy link
@uboness

uboness Aug 4, 2014

Author Contributor

yeah.. should be abstract (no real need for ignore)

This comment has been minimized.

Copy link
@javanna

javanna Aug 5, 2014

Member

I double checked and heard that @Ignore is needed as well, otherwise IntelliJ tries to run this test as well when running all tests from the IDE.

@javanna

View changes

src/test/java/org/elasticsearch/client/AbstractClientHeadersTests.java Outdated
};
}

protected static void testActions(Client client) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

I would maybe move this to be the actual test method, with @Test annotation, and have either an abstract setup method to be implemented by subclasses or even use junit annotations like @Before if possible in the subclasses.

This comment has been minimized.

Copy link
@uboness

uboness Aug 4, 2014

Author Contributor

yeah.. will do

@javanna

View changes

src/main/java/org/elasticsearch/client/support/Headers.java Outdated
import org.elasticsearch.common.settings.Settings;

/**
*

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

it's a public class, can we add some javadoc?

This comment has been minimized.

Copy link
@uboness

uboness Aug 4, 2014

Author Contributor

public indeed... not public API... but sure.. will add a comment :)

@javanna

View changes

src/test/java/org/elasticsearch/client/AbstractClientHeadersTests.java Outdated
}
if (counter++ > 10) {
// dear god, if we got more than 10 levels down, WTF? just bail
t.printStackTrace();

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

can we use a logger instead?

This comment has been minimized.

Copy link
@uboness

uboness Aug 4, 2014

Author Contributor

mee... I'll just output the stack trace as string

@javanna

View changes

src/test/java/org/elasticsearch/client/AbstractClientHeadersTests.java Outdated
assertThat(headers.get("key2").toString(), equalTo("val 2"));
}

public static Throwable unwrape(Throwable t, Class<? extends Throwable> exceptionType) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

s/unwrape/unwrap

@javanna

View changes

src/test/java/org/elasticsearch/client/node/NodeClientHeadersTests.java Outdated
put(action, new InternalTransportAction(settings, name, threadPool));

} catch (Exception e) {
e.printStackTrace();

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

same as above, can we use a logger instead?

This comment has been minimized.

Copy link
@uboness

uboness Aug 4, 2014

Author Contributor

same as above

@javanna

View changes

src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java Outdated
public class TransportClientHeadersTests extends AbstractClientHeadersTests {

private static final LocalTransportAddress address = new LocalTransportAddress("test");
private static final String clusterName = "clusty";

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

can we maybe use a cluster name that has something to do with headers?

This comment has been minimized.

Copy link
@uboness

uboness Aug 4, 2014

Author Contributor

hmm... wat? it's a mock... no cluster is actually started right?

This comment has been minimized.

Copy link
@javanna

javanna Aug 5, 2014

Member

right, its a mock.. then use ClusterName.DEFAULT and you need one less constant.

@javanna

View changes

src/test/java/org/elasticsearch/client/node/NodeClientHeadersTests.java Outdated
// - NAME - holds the name of the action
//
// this test will fail if any of these static fields is missing. Which is a good
// thing as it will also help to keep consistency around action implementations

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

We don't assume that all actions have the constants, only the ones that we register. This is something that makes me think using mock requests, actions and response would simplify things, as I wrote above.

Yet, if this is a good test for consistency we should have it as a separate test and test all the registered actions, not only a few of them.

This comment has been minimized.

Copy link
@uboness

uboness Aug 4, 2014

Author Contributor

We don't assume that all actions have the constants, only the ones that we register. This is something that makes me think using mock requests, actions and response would simplify things, as I wrote above.

The actions we register are the public actions we're testing, so it still holds (and I still think it's a good sanity check for consistency).

I don't see how mock req, res and actions will help you to test further than this test already does. The same execution path and validation will be applied, except instead of a set of existing actions with a mock action.

This comment has been minimized.

Copy link
@javanna

javanna Aug 5, 2014

Member

Mocks would only help hopefully trimming the test down and not requiring to throw an exception to properly test things, which is what feels wrong here. They won't help testing it further than this test already does.

This comment has been minimized.

Copy link
@javanna

javanna Aug 5, 2014

Member

Regarding the sanity check for actions I agree with you that it's a good test, and if we both think it's valid we should run it against all actions, not only the ones tested on this test. Also thinking if we really need reflection on this specific test, cause you could just pass in instances of the actions instead of the class name (unless I'm missing something).

This comment has been minimized.

Copy link
@uboness

uboness Aug 5, 2014

Author Contributor

Also thinking if we really need reflection on this specific test, cause you could just pass in instances of the actions instead of the class name (unless I'm missing something).

agree on passing the action instances themselves

@javanna

View changes

src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java Outdated
testActions(client);
}

public static class InternalTransportService extends MockTransportService {

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

We are not using any feature provided by MockTransportService right? maybe we should just subclass TransportService then?

This comment has been minimized.

Copy link
@uboness

uboness Aug 4, 2014

Author Contributor

does't matter... but ok...

@javanna

View changes

src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java Outdated
@Override
public <T extends TransportResponse> void sendRequest(DiscoveryNode node, String action, TransportRequest request, TransportRequestOptions options, TransportResponseHandler<T> handler) {
if (NodesInfoAction.NAME.equals(action)) {
((TransportResponseHandler<NodesInfoResponse>) handler).handleResponse(new NodesInfoResponse(new ClusterName(clusterName), new NodeInfo[0]));

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

can we suppress the unchecked warning here?

@javanna

View changes

src/test/java/org/elasticsearch/client/node/NodeClientHeadersTests.java Outdated
private Actions(Settings settings, ThreadPool threadPool, Class[] actionClasses) {
this.settings = settings;
this.threadPool = threadPool;
for (int i = 0; i < actionClasses.length; i++) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2014

Member

this could be a for each loop instead

This comment has been minimized.

Copy link
@uboness

uboness Aug 4, 2014

Author Contributor

sure

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 4, 2014

Getting closer, left some comments, would love it if somebody else could look at it, especially the tests, @jpountz maybe?

@javanna

View changes

src/test/java/org/elasticsearch/client/AbstractClientHeadersTests.java Outdated
@Override
public void onFailure(Throwable t) {
Throwable e = unwrap(t, InternalException.class);
assertThat("exected action [" + action + "] to throw an internal exception", e, notNullValue());

This comment has been minimized.

Copy link
@javanna

javanna Aug 5, 2014

Member

sorry, I can't help it... typo s/exected/expected

This comment has been minimized.

Copy link
@uboness

uboness Aug 5, 2014

Author Contributor

I'm afraid it's me... not you ;)

@javanna

View changes

src/test/java/org/elasticsearch/client/node/NodeClientHeadersTests.java Outdated
Settings settings = HEADER_SETTINGS;

Headers headers = new Headers(settings);
ThreadPool threadPool = new ThreadPool("test");

This comment has been minimized.

Copy link
@javanna

javanna Aug 5, 2014

Member

We need to close this thread pool at the end of the test

Added an option to add arbitrary headers to the client requests
The headers are key/value pairs defined in the settings under the `request.headers` namespace.
@javanna

This comment has been minimized.

Copy link
Member

commented Aug 5, 2014

LGTM

@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2014

merged by 5a2bf32

@uboness uboness closed this Aug 6, 2014

@clintongormley clintongormley changed the title Added an option to add arbitrary headers to the client requests Internal: Added an option to add arbitrary headers to the client requests Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Added an option to add arbitrary headers to the client requests Added an option to add arbitrary headers to the client requests Jun 6, 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.