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

Add support for parsing inline script (#23824) #26846

Merged
merged 2 commits into from
Oct 11, 2017

Conversation

liketic
Copy link

@liketic liketic commented Sep 30, 2017

Closes #23824

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@s1monw
Copy link
Contributor

s1monw commented Sep 30, 2017

@elasticmachine ok to test

} else {
throw newConfigurationException(TYPE, processorTag, null, "Could not initialize script");
}
Arrays.asList("id", "source", "inline", "lang", "params", "options").forEach(config::remove);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed since we are not verifying any input-properties of the script configuration, and letting Script.parse do all the handling and validation.

@talevy
Copy link
Contributor

talevy commented Oct 9, 2017

thanks for the PR @liketic! Looks like what I had in mind given how we parse things in Ingest Processors. Just left one comment, otherwise looks good to me!

@talevy talevy added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v6.1.0 v7.0.0 >enhancement labels Oct 9, 2017
@liketic
Copy link
Author

liketic commented Oct 11, 2017

@talevy Really thanks for reviewing this. Why I have to put the code:

https://github.com/liketic/elasticsearch/blob/9a8822c9a64d4aa44199b7b941382d707f8f6caa/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/ScriptProcessor.java#L102

here, is because ConfigurationUtils.readProcessor will check if there are any properties left in config:

And Script.parse will not remove properties in config. So either I remove properties in config in ScriptProcessor, or don't check if config is empty if the type is script. I'm not sure if there is a better solution.

@talevy
Copy link
Contributor

talevy commented Oct 11, 2017

Ah, you're right. I forgot this was being done within the readProcessor umbrella. I would leave it as is!

I'll go ahead and merge this in. thank you!

@talevy talevy merged commit 2e36f19 into elastic:master Oct 11, 2017
jasontedor added a commit that referenced this pull request Oct 12, 2017
* master: (35 commits)
  Create weights lazily in filter and filters aggregation (#26983)
  Use a dedicated ThreadGroup in rest sniffer (#26897)
  Fire global checkpoint sync under system context
  Update by Query is modified to accept short `script` parameter. (#26841)
  Cat shards bytes (#26952)
  Add support for parsing inline script (#23824) (#26846)
  Change default value to true for transpositions parameter of fuzzy query (#26901)
  Adding unreleased 5.6.4 version number to Version.java
  Rename TCPTransportTests to TcpTransportTests (#26954)
  Fix NPE for /_cat/indices when no primary shard (#26953)
  [DOCS] Fixed indentation of the definition list.
  Fix formatting in channel close test
  Check for closed connection while opening
  Clarify systemd overrides
  [DOCS] Plugin Installation for Windows (#21671)
  Painless: add tests for cached boxing (#24163)
  Don't detect source's XContentType in DocumentParser.parseDocument() (#26880)
  Fix handling of paths containing parentheses
  Allow only a fixed-size receive predictor (#26165)
  Add Homebrew instructions to getting started
  ...
@liketic liketic deleted the issues/23824 branch October 14, 2017 12:50
jasontedor added a commit to olcbean/elasticsearch that referenced this pull request Oct 16, 2017
* master: (356 commits)
  Do not set SO_LINGER on server channels (elastic#26997)
  Fix inconsistencies in the rest api specs for *_script (elastic#26971)
  fix inconsistencies in the rest api specs for cat.snapshots (elastic#26996)
  Add docs on full_id parameter in cat nodes API
  [TEST] Add test that replicates versioned updates with random flushes
  Use internal searcher for all indexing related operations in the engine
  Reformat paragraph in template docs to 80 columns
  Clarify settings and template on create index
  Fix reference to TcpTransport in documentation
  Allow Uid#decodeId to decode from a byte array slice (elastic#26987)
  Fix a typo in the similarity docs (elastic#26970)
  Use separate searchers for "search visibility" vs "move indexing buffer to disk (elastic#26972)
  Create weights lazily in filter and filters aggregation (elastic#26983)
  Use a dedicated ThreadGroup in rest sniffer (elastic#26897)
  Fire global checkpoint sync under system context
  Update by Query is modified to accept short `script` parameter. (elastic#26841)
  Cat shards bytes (elastic#26952)
  Add support for parsing inline script (elastic#23824) (elastic#26846)
  Change default value to true for transpositions parameter of fuzzy query (elastic#26901)
  Adding unreleased 5.6.4 version number to Version.java
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement v6.1.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants