Skip to content

Commit

Permalink
Generic correct merging of local params (#939)
Browse files Browse the repository at this point in the history
* Bugfix for facet queries with local parameters

* Generic correct merging of local params

* Missing test coverage

* Handle arrays in Helper::qparser() the same as in AbstractRequestBuilder::renderLocalParams()

* Rendering booleans and 0 in local params

* Remove LocalParameters::render()

* Style fixes

Co-authored-by: Markus Kalkbrenner <git@kalki.de>
  • Loading branch information
thomascorthals and mkalkbrenner committed Jul 5, 2021
1 parent 08b839f commit 2493816
Show file tree
Hide file tree
Showing 12 changed files with 413 additions and 214 deletions.
41 changes: 25 additions & 16 deletions src/Component/RequestBuilder/FacetSet.php
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,10 @@ public function addFacetField(Request $request, FacetField $facet, bool $useLoca
} else {
$request->addParam(
'facet.field',
sprintf('%s%s', $facet->getLocalParameters()->render(), $field)
$this->renderLocalParams(
$field,
$facet->getLocalParameters()->getParameters()
)
);

$request->addParam("f.$field.facet.limit", $facet->getLimit());
Expand All @@ -187,18 +190,12 @@ public function addFacetField(Request $request, FacetField $facet, bool $useLoca
*/
public function addFacetQuery($request, $facet): void
{
$localParams = $facet->getLocalParameters()->render();
$query = $facet->getQuery();

// add local params to those already present in the query, e.g. a qparser
if (null !== $localParams && null !== $query && 0 === strpos($query, '{!')) {
$localParams = strstr($query, '}', true).' '.substr($localParams, 2);
$query = substr($query, strpos($query, '}') + 1);
}

$request->addParam(
'facet.query',
sprintf('%s%s', $localParams, $query)
$this->renderLocalParams(
$facet->getQuery(),
$facet->getLocalParameters()->getParameters()
)
);
}

Expand Down Expand Up @@ -227,7 +224,10 @@ public function addFacetRange($request, $facet): void

$request->addParam(
'facet.range',
sprintf('%s%s', $facet->getLocalParameters()->render(), $field)
$this->renderLocalParams(
$field,
$facet->getLocalParameters()->getParameters()
)
);

$request->addParam("f.$field.facet.range.start", $facet->getStart());
Expand All @@ -247,7 +247,10 @@ public function addFacetRange($request, $facet): void
if (null !== $pivot = $facet->getPivot()) {
$request->addParam(
'facet.pivot',
sprintf('%s%s', $pivot->getLocalParameters()->render(), implode(',', $pivot->getFields()))
$this->renderLocalParams(
implode(',', $pivot->getFields()),
$pivot->getLocalParameters()->getParameters()
)
);
}
}
Expand All @@ -273,7 +276,10 @@ public function addFacetPivot($request, $facet): void

$request->addParam(
'facet.pivot',
sprintf('%s%s', $facet->getLocalParameters()->render(), implode(',', $facet->getFields()))
$this->renderLocalParams(
implode(',', $facet->getFields()),
$facet->getLocalParameters()->getParameters()
)
);
$request->addParam('facet.pivot.mincount', $facet->getMinCount(), true);
$request->addParam('facet.pivot.limit', $facet->getLimit(), true);
Expand All @@ -291,12 +297,15 @@ public function addFacetInterval($request, $facet): void

$request->addParam(
'facet.interval',
sprintf('%s%s', $facet->getLocalParameters()->render(), $field)
$this->renderLocalParams(
$field,
$facet->getLocalParameters()->getParameters()
)
);

foreach ($facet->getSet() as $key => $setValue) {
if (\is_string($key)) {
$setValue = '{!key="'.$key.'"}'.$setValue;
$setValue = $this->renderLocalParams($setValue, ['key' => '"'.$key.'"']);
}
$request->addParam("f.$field.facet.interval.set", $setValue);
}
Expand Down
11 changes: 8 additions & 3 deletions src/Component/RequestBuilder/Stats.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@
use Solarium\Component\Stats\Stats as StatsComponent;
use Solarium\Core\Client\Request;
use Solarium\Core\ConfigurableInterface;
use Solarium\Core\Query\AbstractRequestBuilder as BaseRequestBuilder;

/**
* Add select component stats to the request.
*/
class Stats implements ComponentRequestBuilderInterface
class Stats extends BaseRequestBuilder implements ComponentRequestBuilderInterface
{
/**
* Add request settings for the stats component.
Expand All @@ -33,10 +34,14 @@ public function buildComponent(ConfigurableInterface $component, Request $reques

// add fields
foreach ($component->getFields() as $field) {
$statsField = $field->getKey();
$pivots = $field->getPivots();

$prefix = (\count($pivots) > 0) ? '{!tag='.implode(',', $pivots).'}' : '';
$request->addParam('stats.field', $prefix.$field->getKey());
if (0 !== \count($pivots)) {
$statsField = $this->renderLocalParams($statsField, ['tag' => $pivots]);
}

$request->addParam('stats.field', $statsField);

// add field specific facet stats
foreach ($field->getFacets() as $facet) {
Expand Down
20 changes: 11 additions & 9 deletions src/Component/RequestBuilder/SubRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@

namespace Solarium\Component\RequestBuilder;

use Solarium\Core\Query\AbstractRequestBuilder as BaseRequestBuilder;

/**
* Class for describing a sub request.
*/
class SubRequest implements RequestParamsInterface
class SubRequest extends BaseRequestBuilder implements RequestParamsInterface
{
use RequestParamsTrait;

Expand Down Expand Up @@ -50,18 +52,18 @@ public function setQueryParser(string $value): self
/**
* returns the complete sub request as string.
*
* @param string $separator
*
* @return string
*/
public function getSubQuery(string $separator = ' '): string
public function getSubQuery(): string
{
$queryString = '';
foreach ($this->getParams() as $key => $value) {
$queryString .= $separator.$key.'='.$value;
}
if ($queryString) {
$queryString = '{!'.$this->getQueryParser().$queryString.'}';
$params = $this->getParams();

if (0 !== \count($params)) {
$queryString = $this->getHelper()->qparser(
$this->getQueryParser(),
$params
);
}

return $queryString;
Expand Down
14 changes: 11 additions & 3 deletions src/Core/Query/AbstractRequestBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,20 +70,28 @@ public function build(AbstractQuery $query): Request
public function renderLocalParams(string $value, array $localParams = []): string
{
$params = '';

if (0 === strpos($value, '{!')) {
$params = substr($value, 2, strpos($value, '}') - 2).' ';
$value = substr($value, strpos($value, '}') + 1);
}

foreach ($localParams as $paramName => $paramValue) {
if (empty($paramValue)) {
if (null === $paramValue || '' === $paramValue || [] === $paramValue) {
continue;
}

if (\is_array($paramValue)) {
$paramValue = implode(',', $paramValue);
} elseif (\is_bool($paramValue)) {
$paramValue = $paramValue ? 'true' : 'false';
}

$params .= $paramName.'='.$paramValue.' ';
}

if ('' !== $params) {
$value = '{!'.trim($params).'}'.$value;
if ('' !== $params = trim($params)) {
$value = sprintf('{!%s}%s', $params, $value);
}

return $value;
Expand Down
6 changes: 6 additions & 0 deletions src/Core/Query/Helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,12 @@ public function qparser(string $name, array $params = [], bool $dereferenced = f
$output = '{!'.$name;
foreach ($params as $key => $value) {
if (!$dereferenced || $forceKeys || \is_int($key)) {
if (\is_array($value)) {
$value = implode(',', $value);
} elseif (\is_bool($value)) {
$value = $value ? 'true' : 'false';
}

$output .= ' '.$key.'='.$value;
}
}
Expand Down
29 changes: 17 additions & 12 deletions src/Core/Query/LocalParameters/LocalParameters.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,6 @@ class LocalParameters implements \ArrayAccess
*/
private $parameters = [];

/**
* @return string|null
*/
public function render(): ?string
{
if ('' === $value = implode(' ', array_filter(array_map('strval', $this->parameters)))) {
return null;
}

return sprintf('{!%s}', $value);
}

/**
* @param string $key
*
Expand Down Expand Up @@ -1024,4 +1012,21 @@ private function getParameter(string $type): LocalParameterInterface

return $this->parameters[$type];
}

/**
* Get all local parameters in a key => value format.
*
* @return array
*/
public function getParameters(): array
{
$params = [];

/** @var LocalParameterInterface $parameter */
foreach ($this->parameters as $parameter) {
$params[$parameter->getType()] = $parameter->getValues();
}

return $params;
}
}
10 changes: 8 additions & 2 deletions src/QueryType/Select/RequestBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ public function build(AbstractQuery $query): Request
// add basic params to request
$request->addParam(
'q',
sprintf('%s%s', $query->getLocalParameters()->render(), $query->getQuery())
$this->renderLocalParams(
$query->getQuery(),
$query->getLocalParameters()->getParameters()
)
);
$request->addParam('start', $query->getStart());
$request->addParam('rows', $query->getRows());
Expand All @@ -57,7 +60,10 @@ public function build(AbstractQuery $query): Request
$filterQueries = $query->getFilterQueries();
if (0 !== \count($filterQueries)) {
foreach ($filterQueries as $filterQuery) {
$fq = sprintf('%s%s', $filterQuery->getLocalParameters()->render(), $filterQuery->getQuery());
$fq = $this->renderLocalParams(
$filterQuery->getQuery(),
$filterQuery->getLocalParameters()->getParameters()
);

$request->addParam('fq', $fq);
}
Expand Down
4 changes: 4 additions & 0 deletions tests/Component/RequestBuilder/StatsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ public function testBuildComponent()
$component = new Component();
$component->createField('fieldA')->addFacet('fieldFacetA');
$component->createField('fieldB');
$component->createField('fieldC')->addPivots(['piv1', 'piv2']);
$component->createField('{!mean=true}fieldD')->addPivots(['piv1', 'piv2']);
$component->addFacets(['facetA', 'facetB']);

$request = $builder->buildComponent($component, $request);
Expand All @@ -31,6 +33,8 @@ public function testBuildComponent()
'stats.field' => [
'fieldA',
'fieldB',
'{!tag=piv1,piv2}fieldC',
'{!mean=true tag=piv1,piv2}fieldD',
],
'f.fieldA.stats.facet' => 'fieldFacetA',
],
Expand Down
4 changes: 2 additions & 2 deletions tests/Core/Query/HelperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,8 @@ public function testQparserNoParams()
public function testQparser()
{
$this->assertSame(
'{!parser a=1 b=test}',
$this->helper->qparser('parser', ['a' => 1, 'b' => 'test'])
'{!parser a=1 b=0 c=test d=tag1,tag2 e=true f=false}',
$this->helper->qparser('parser', ['a' => 1, 'b' => 0, 'c' => 'test', 'd' => ['tag1', 'tag2'], 'e' => true, 'f' => false])
);
}

Expand Down
89 changes: 89 additions & 0 deletions tests/Core/Query/LocalParameters/LocalParameterTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
<?php

declare(strict_types=1);

namespace Solarium\Tests\Core\Query\LocalParameters;

use PHPUnit\Framework\TestCase;
use Solarium\Core\Query\LocalParameters\LocalParameter;

/**
* LocalParameterTest.
*/
class LocalParameterTest extends TestCase
{
/**
* @var LocalParameter
*/
protected $parameter;

public function setUp(): void
{
$this->parameter = new LocalParameter(LocalParameter::TYPE_KEY);
}

public function testGetType(): void
{
$this->assertSame(LocalParameter::TYPE_KEY, $this->parameter->getType());
}

public function testSetValues(): void
{
$this->parameter->setValues(['value1', 'value2']);
$this->assertSame(['value1', 'value2'], $this->parameter->getValues());

$this->parameter->setValues(['value3', 'value4']);
$this->assertSame(['value3', 'value4'], $this->parameter->getValues());
}

public function testAddValue(): void
{
$this->parameter->addValue('value1');
$this->assertSame(['value1'], $this->parameter->getValues());

$this->parameter->addValue('value2');
$this->assertSame(['value1', 'value2'], $this->parameter->getValues());
}

public function testAddValues(): void
{
$this->parameter->addValues(['value1', 'value2']);
$this->assertSame(['value1', 'value2'], $this->parameter->getValues());

$this->parameter->addValues(['value3', 'value4']);
$this->assertSame(['value1', 'value2', 'value3', 'value4'], $this->parameter->getValues());
}

public function testRemoveValue(): void
{
$this->parameter->setValues(['value1', 'value2']);
$this->assertSame(['value1', 'value2'], $this->parameter->getValues());

$this->parameter->removeValue('value1');
$this->assertSame(['value2'], $this->parameter->getValues());
}

public function testClearValues(): void
{
$this->parameter->setValues(['value1', 'value2']);
$this->assertSame(['value1', 'value2'], $this->parameter->getValues());

$this->parameter->clearValues();
$this->assertEmpty($this->parameter->getValues());
}

public function testToString(): void
{
$this->parameter->clearValues();
$this->assertSame('', (string) $this->parameter);

$this->parameter->setValues(['']);
$this->assertSame('', (string) $this->parameter);

$this->parameter->setValues(['value1']);
$this->assertSame('key=value1', (string) $this->parameter);

$this->parameter->setValues(['value1', 'value2']);
$this->assertSame('key=value1,value2', (string) $this->parameter);
}
}
Loading

0 comments on commit 2493816

Please sign in to comment.