Add support for executing search templates

[newoga]

This pull request adds 3 properties to the SearchOptions.java data object that add support for doing searches based on search templates:

• templateName (String)
• templateType (String)
• templateParams (Map<String, String>)

It also adds these properties to the SearchRequestBuilder in DefaultElasticSearchService.search().

I also added these three properties to the SearchOptionsTest.java

[newoga]

This is related to #13

@adrianluisgonzalez , I wasn't to sure what would be more idiomatic for the templateParams property. On one hand, using Map<String, String> is more idiomatic to ElasticSearch, and provides better type safety. I originally used JsonObject which would may be more natural in vertx, but it could easily get hairy if someone accidentally added other types including arrays or objects which doesn't seem supported by ElasticSearch. Also, an end user of the service could always pass JsonObject.getMap() so it doesn't make it too much more difficult. I'm open to what you think is best.

Another approach that I didn't try is just to create a SearchTempateParams data object that only supported putting and getting Strings. It would still support vertx code generation but only allow setting String types. The only downside, I suppose, is that it would just be another class to maintain in this project.

Let me know what you think

[adrianluisgonzalez]

Do you mind merging (or rebasing) the latest version of develop with ES 1.7.0 into your branch first? The elasticsearch signature for SearchRequestBuilder.setTemplateParams() changed from Map<String, String> to Map<String, Object> between 1.4.5 and 1.5.0.

Cheers.

[newoga]

Sure thing!

[newoga]

Hi @adrianluisgonzalez , I just merged in the latest in development and changed the API back to using JsonObject instead since we can pass the .getMap() ourselves pretty easily.

[newoga]

@adrianluisgonzalez , would you prefer if I squash all of these commits even though it will rewrite the history or should I continue to make new commits?

[adrianluisgonzalez]

Squash would be better to clean up the history. Thanks!

[newoga]

@adrianluisgonzalez , let me know if there's anything else you think could use improvement over this implementation. When you think it's ready, I'll squash the commits!

Thanks!

[adrianluisgonzalez]

Looks good to me. Ready to squash and merge.

[newoga]

Squashed and ready to go! Thanks @adrianluisgonzalez

[adrianluisgonzalez]

@newoga, I was thinking about this a little bit more, and I wonder if the SearchOptions constructor should be changed like this:

        String s = json.getString(JSON_FIELD_TEMPLATE_TYPE);
        if (!Strings.isNullOrEmpty(s)) {
            templateType = ScriptService.ScriptType.valueOf(s.toUpperCase());
        }

If a templateType is provided, but it is not a valid enum, then an IllegalArgumentException is thrown. Seems more explicit. Any concerns with this change?

Added feature branch: https://github.com/englishtown/vertx-elasticsearch-service/tree/feature/templateType

[newoga]

Hey @adrianluisgonzalez , I was debating myself how the service should handle this as well, and ended up writing tests particularly for this scenario in testSetTemplateTypeFromJson() in the SearchOptionsTest.

My initial thinking was to do as you suggested, but I wasn't sure how vertx handled these kind of exceptions for codegen and proxygen. Do you know if these exceptions would only occur on the "server" side if was being used through proxy generation since they're not really being handled and passed through a FailedFuture? I know the project doesn't currently support the proxy generation but I saw that it was something we'd like to do on the README.

Still need to learn a little bit more about the edge cases for this code and proxy generation.

[adrianluisgonzalez]

It actually does support proxy gen. The classes are here: https://github.com/englishtown/vertx-elasticsearch-service/tree/develop/src/main/generated/com/englishtown/vertx/elasticsearch

The resultHandler will receive an AsnycResult that has failed so the client will be notified with the exception.

I need to work on the README.md at some point...

[adrianluisgonzalez]

Correction, the exception is not returned to the resultHandler. This seems like a vert.x bug to me. Let me look at this more tomorrow.

[newoga]

Mmm, I was just beginning to think you were right!

[newoga]

Here's what I started to write...

Oh okay, I'm still wrapping my head around how this some this code generation works.

If I understand correctly now, even if we didn't forcibly catch the IllegalArgumentException in DefaultElasticSearchService.search() implementation, and pass the exception using resultHandler.handle(Future.failedFuture(t)); the proxy generation code would catch it and send it back to the client for it to handle.

[newoga]

I was looking at the ElasticSearchServiceVertxProxyHandler a little more closely, and it appears that if an exception is thrown in that method, it might be handled and sent to the client.

But yeah I agree, I can look some more too. This is certainly forcing me to get a better understanding of this.

[adrianluisgonzalez]

@newoga , I added a modified version of the proxyhandler.templ file. This adds a try/catch and calls msg.fail() if there is an exception to give the behavior I was expecting.

If you like it, I will submit a PR to vertx-service-proxy to see what they think of this change for all proxy classes.

https://github.com/englishtown/vertx-elasticsearch-service/commit/bb189cc15c59be73c353b8a8ec6409596ec105c1

[newoga]

:+1: @adrianluisgonzalez Yeah, my concerns with throwing exceptions in the manner you presented go away if it those exceptions get handled in a try catch at the proxy handler level and sent to the client as a failed future. I haven't had a chance to try this myself and test it, but I don't think it hurts to get some feedback from the core developers in the vertx-service-proxy project. I think from a developer point of view, this behavior seems a lot more natural.

[adrianluisgonzalez]

There is an integration test for this scenario here: https://github.com/englishtown/vertx-elasticsearch-service/blob/feature/templateType/src/test/java/com/englishtown/vertx/elasticsearch/integration/IntegrationTest.java#L167

Here's the PR: https://github.com/vert-x3/vertx-service-proxy/pull/19

[newoga]

Great, sounds good to me.