From 22d0596cc6f9fcf67021f552a749711d46c3790d Mon Sep 17 00:00:00 2001 From: Matthias Mullie Date: Tue, 13 Jan 2015 12:26:18 +0100 Subject: [PATCH 1/5] Set correct script attributes in AbstractSimpleAggregation Existing code would not set 'lang' (making it impossible to e.g. use groovy in those scripts) It would also always add 'params', even if there aren't any --- lib/Elastica/Aggregation/AbstractSimpleAggregation.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/Elastica/Aggregation/AbstractSimpleAggregation.php b/lib/Elastica/Aggregation/AbstractSimpleAggregation.php index af7c1940db..e5bef87550 100644 --- a/lib/Elastica/Aggregation/AbstractSimpleAggregation.php +++ b/lib/Elastica/Aggregation/AbstractSimpleAggregation.php @@ -25,9 +25,8 @@ public function setField($field) public function setScript($script) { if ($script instanceof Script) { - $this->setParam('params', $script->getParams()); - $script = $script->getScript(); + return $this->setParams($script->toArray()); } return $this->setParam('script', $script); } -} \ No newline at end of file +} From e4f971cd491cf326e34ff79a3d71f7b7eec63b8c Mon Sep 17 00:00:00 2001 From: Matthias Mullie Date: Tue, 13 Jan 2015 12:46:55 +0100 Subject: [PATCH 2/5] Don't override existing params I had blindly set params, overriding any existing params. Now they'll be merged properly: only the params known to $script will be overriden. Unrelated existing additional parameters will remain intact. --- lib/Elastica/Aggregation/AbstractSimpleAggregation.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Elastica/Aggregation/AbstractSimpleAggregation.php b/lib/Elastica/Aggregation/AbstractSimpleAggregation.php index e5bef87550..e06754abfe 100644 --- a/lib/Elastica/Aggregation/AbstractSimpleAggregation.php +++ b/lib/Elastica/Aggregation/AbstractSimpleAggregation.php @@ -25,7 +25,8 @@ public function setField($field) public function setScript($script) { if ($script instanceof Script) { - return $this->setParams($script->toArray()); + $params = array_merge($this->getParams(), $script->toArray()); + return $this->setParams($params); } return $this->setParam('script', $script); } From 8d876090d8825e68cf387c35c47c501f7712aede Mon Sep 17 00:00:00 2001 From: Matthias Mullie Date: Wed, 14 Jan 2015 09:02:23 +0100 Subject: [PATCH 3/5] Add tests for AbstractSimpleAggregation::setScript --- .../Elastica/Test/Aggregation/ScriptTest.php | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 test/lib/Elastica/Test/Aggregation/ScriptTest.php diff --git a/test/lib/Elastica/Test/Aggregation/ScriptTest.php b/test/lib/Elastica/Test/Aggregation/ScriptTest.php new file mode 100644 index 0000000000..6ee395a4e2 --- /dev/null +++ b/test/lib/Elastica/Test/Aggregation/ScriptTest.php @@ -0,0 +1,65 @@ +_index = $this->_createIndex( 'script' ); + $docs = array( + new Document('1', array('price' => 5)), + new Document('2', array('price' => 8)), + new Document('3', array('price' => 1)), + new Document('4', array('price' => 3)), + ); + $this->_index->getType( 'test' )->addDocuments( $docs ); + $this->_index->refresh(); + } + + public function testAggregationScript() + { + $agg = new Sum( "sum" ); + // x = (0..1) is groovy-specific syntax, to see if lang is recognized + $script = new Script( "x = (0..1); return doc['price'].value", null, "groovy" ); + $agg->setScript( $script ); + + $query = new Query(); + $query->addAggregation( $agg ); + $results = $this->_index->search( $query )->getAggregation( "sum" ); + + $this->assertEquals( 5 + 8 + 1 + 3, $results['value'] ); + } + + public function testSetScript() { + $aggregation = "sum"; + $string = "doc['price'].value"; + $params = array( + 'param1' => 'one', + 'param2' => 1, + ); + $lang = "groovy"; + + $agg = new Sum( $aggregation ); + $script = new Script( $string, $params, $lang ); + $agg->setScript( $script ); + + $array = $agg->toArray(); + + $expected = array( + $aggregation => array( + 'script' => $string, + 'params' => $params, + 'lang' => $lang, + ) + ); + $this->assertEquals($expected, $array); + } +} From cba710fe308824f39d53cf4d7be432dd2eedd875 Mon Sep 17 00:00:00 2001 From: Matthias Mullie Date: Wed, 14 Jan 2015 10:31:48 +0100 Subject: [PATCH 4/5] Get rid of auto-formatted MediaWiki-like coding standards MediaWiki has spaces around parentheses & my IDE auto-formats source like that. Obviously, don't want this here. --- .../Elastica/Test/Aggregation/ScriptTest.php | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/lib/Elastica/Test/Aggregation/ScriptTest.php b/test/lib/Elastica/Test/Aggregation/ScriptTest.php index 6ee395a4e2..7241a73637 100644 --- a/test/lib/Elastica/Test/Aggregation/ScriptTest.php +++ b/test/lib/Elastica/Test/Aggregation/ScriptTest.php @@ -13,29 +13,29 @@ class ScriptTest extends BaseAggregationTest protected function setUp() { parent::setUp(); - $this->_index = $this->_createIndex( 'script' ); + $this->_index = $this->_createIndex('script'); $docs = array( new Document('1', array('price' => 5)), new Document('2', array('price' => 8)), new Document('3', array('price' => 1)), new Document('4', array('price' => 3)), ); - $this->_index->getType( 'test' )->addDocuments( $docs ); + $this->_index->getType('test')->addDocuments($docs); $this->_index->refresh(); } public function testAggregationScript() { - $agg = new Sum( "sum" ); + $agg = new Sum("sum"); // x = (0..1) is groovy-specific syntax, to see if lang is recognized - $script = new Script( "x = (0..1); return doc['price'].value", null, "groovy" ); - $agg->setScript( $script ); + $script = new Script("x = (0..1); return doc['price'].value", null, "groovy"); + $agg->setScript($script); $query = new Query(); - $query->addAggregation( $agg ); - $results = $this->_index->search( $query )->getAggregation( "sum" ); + $query->addAggregation($agg); + $results = $this->_index->search($query)->getAggregation("sum"); - $this->assertEquals( 5 + 8 + 1 + 3, $results['value'] ); + $this->assertEquals(5 + 8 + 1 + 3, $results['value']); } public function testSetScript() { @@ -47,9 +47,9 @@ public function testSetScript() { ); $lang = "groovy"; - $agg = new Sum( $aggregation ); - $script = new Script( $string, $params, $lang ); - $agg->setScript( $script ); + $agg = new Sum($aggregation); + $script = new Script($string, $params, $lang); + $agg->setScript($script); $array = $agg->toArray(); From 1331583c53cdf582db70e1452ac72795ee267459 Mon Sep 17 00:00:00 2001 From: Matthias Mullie Date: Wed, 14 Jan 2015 10:34:13 +0100 Subject: [PATCH 5/5] Test case for script-as-string My change slightly dropped code coverage: when passing a Script, it used to manually extract the script (as string) from the Script object & set it. Now that I've changed it to use Script::toArray(), that "set script as string" patch was no longer being tested. This adds a separate test for that case. --- test/lib/Elastica/Test/Aggregation/ScriptTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/lib/Elastica/Test/Aggregation/ScriptTest.php b/test/lib/Elastica/Test/Aggregation/ScriptTest.php index 7241a73637..0b31848bd6 100644 --- a/test/lib/Elastica/Test/Aggregation/ScriptTest.php +++ b/test/lib/Elastica/Test/Aggregation/ScriptTest.php @@ -38,6 +38,18 @@ public function testAggregationScript() $this->assertEquals(5 + 8 + 1 + 3, $results['value']); } + public function testAggregationScriptAsString() + { + $agg = new Sum("sum"); + $agg->setScript("doc['price'].value"); + + $query = new Query(); + $query->addAggregation( $agg ); + $results = $this->_index->search($query)->getAggregation("sum"); + + $this->assertEquals(5 + 8 + 1 + 3, $results['value']); + } + public function testSetScript() { $aggregation = "sum"; $string = "doc['price'].value";