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

Propagate Headers and Context through to ScriptService #12982

Merged
merged 1 commit into from Sep 2, 2015
Merged

Propagate Headers and Context through to ScriptService #12982

merged 1 commit into from Sep 2, 2015

Conversation

colings86
Copy link
Contributor

At the moment if an index script is used in a request, the spawned request to get the indexed script from the .scripts index does not get the headers and context copied to it from the original request. This change makes the calls to the ScriptService pass in a HasContextAndHeaders object that can provide the headers and context. For the search() method the context and headers are retrieved from SearchContext.current().

Closes #12891

@colings86 colings86 added >bug review :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v2.0.0 labels Aug 19, 2015
@spinscale
Copy link
Contributor

can we add tests to ContextAndHeaderTransportIT for testing this?

@colings86
Copy link
Contributor Author

@spinscale I added a couple of tests to check that the headers and context are propagated for the TemplateQuery and for the reduce phase of aggregations.

@spinscale
Copy link
Contributor

overall this looks good to me, but

  • I couldnt run mvn test due to forbidden API errors, is it me?
  • Can we add a test for the phrase suggesters as it has been changed?
  • Thinking if we can reuse the DelegatingHasContextAndHeaders in our SearchContext (might be tricky with the inheritance though)

@colings86
Copy link
Contributor Author

@spinscale I have implemented the suggested changes and rebased onto master (without squashing commits), are you able to take another look?

@@ -67,7 +69,7 @@
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;

public abstract class SearchContext implements Releasable, HasContextAndHeaders {
public abstract class SearchContext extends DelegatingHasContextAndHeaders implements Releasable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with this change in place, can you try to remove the HasContextAndHeaders delegation methods from PercolateContext and FilteredSearchContext?

@spinscale
Copy link
Contributor

left one last comment, I am not sure if you can remove all the methods (having the asserts in the percolate context seems useful to me), but maybe you can remove some code...

apart from that LGTM

@colings86
Copy link
Contributor Author

I was able to remove the methods from FilteredSearchContext but I kept them in PercolateContext as I think the asserts are important to have.

At the moment if an index script is used in a request, the spawned request to get the indexed script from the `.scripts` index does not get the headers and context copied to it from the original request. This change makes the calls to the `ScriptService` pass in a `HasContextAndHeaders` object that can provide the headers and context. For the `search()` method the context and headers are retrieved from `SearchContext.current()`.

Closes #12891
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v2.0.0-beta2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants