Skip to content

Commit 5d51ad9

Browse files
committed
fix(metadata): :property dedup drops repeated parameters
Multiple `#[QueryParameter(key: ':property', ...)]` attributes with disjoint properties lists were silently overwritten by the last one in `Parameters::add`, `Parameters::__construct` and `MetadataCollectionFactoryTrait::mergeOperationParameters`. The `getProperties()` cache in `ParameterResourceMetadataCollectionFactory` also collided on shared key. `:property` keys are templates expanded per-property later, so multiple templates with disjoint `properties` must coexist. Templates with identical `properties` keep last-write-wins semantics. Closes #8184
1 parent 286a47e commit 5d51ad9

5 files changed

Lines changed: 123 additions & 6 deletions

File tree

src/Metadata/Parameters.php

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,12 @@ public function __construct(array $parameters = [])
3737
$parameterName = $parameter->getKey();
3838
}
3939

40-
$key = \sprintf('%s.%s', $parameter::class, $parameterName);
40+
// `:property` is a template expanded per-property later; multiple templates with disjoint properties must coexist.
41+
if (str_contains((string) $parameterName, ':property')) {
42+
$key = \sprintf('%s.%s.%s', $parameter::class, $parameterName, self::propertyDiscriminator($parameter));
43+
} else {
44+
$key = \sprintf('%s.%s', $parameter::class, $parameterName);
45+
}
4146

4247
$this->parameters[$key] = [$parameterName, $parameter];
4348
}
@@ -61,19 +66,38 @@ public function getIterator(): \Traversable
6166

6267
public function add(string $key, Parameter $value): self
6368
{
69+
// `:property` is a template expanded per-property later; templates with disjoint properties coexist, identical ones override.
70+
$isTemplate = str_contains($key, ':property');
71+
$valueDiscriminator = $isTemplate ? self::propertyDiscriminator($value) : null;
72+
6473
foreach ($this->parameters as $i => [$parameterName, $parameter]) {
65-
if ($parameterName === $key && $value::class === $parameter::class) {
66-
$this->parameters[$i] = [$key, $value];
74+
if ($parameterName !== $key || $value::class !== $parameter::class) {
75+
continue;
76+
}
6777

68-
return $this;
78+
if ($isTemplate && self::propertyDiscriminator($parameter) !== $valueDiscriminator) {
79+
continue;
6980
}
81+
82+
$this->parameters[$i] = [$key, $value];
83+
84+
return $this;
7085
}
7186

7287
$this->parameters[] = [$key, $value];
7388

7489
return $this;
7590
}
7691

92+
private static function propertyDiscriminator(Parameter $parameter): string
93+
{
94+
if ($properties = $parameter->getProperties()) {
95+
return '['.implode(',', $properties).']';
96+
}
97+
98+
return $parameter->getProperty() ?? '';
99+
}
100+
77101
/**
78102
* @template T of Parameter
79103
*

src/Metadata/Resource/Factory/MetadataCollectionFactoryTrait.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,8 @@ private function mergeOperationParameters(Metadata $resource, Parameters $global
288288
$parameterName = $key;
289289
}
290290

291-
if (!$parameters->has($parameterName, $parameter::class)) {
291+
// `:property` is a template expanded per-property later; multiple templates must coexist.
292+
if (str_contains((string) $parameterName, ':property') || !$parameters->has($parameterName, $parameter::class)) {
292293
$parameters->add($parameterName, $parameter);
293294
}
294295
}

src/Metadata/Resource/Factory/ParameterResourceMetadataCollectionFactory.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,9 @@ private function getProperties(string $resourceClass, ?Parameter $parameter = nu
113113
if ($parameter) {
114114
$paramKey = $parameter->getProperties() ? ($parameter->getKey() ?? '') : ($parameter->getProperty() ?? $parameter->getKey() ?? '');
115115
}
116-
$k = $resourceClass.$paramKey.(\is_string($parameter->getFilter()) ? $parameter->getFilter() : '').$filterClass;
116+
// Include properties list so repeated `:property` templates with disjoint properties get distinct cache entries.
117+
$paramProperties = $parameter?->getProperties() ? '['.implode(',', $parameter->getProperties()).']' : '';
118+
$k = $resourceClass.$paramKey.$paramProperties.(\is_string($parameter->getFilter()) ? $parameter->getFilter() : '').$filterClass;
117119
if (isset($this->localPropertyCache[$k])) {
118120
return $this->localPropertyCache[$k];
119121
}

src/Metadata/Tests/ParametersTest.php

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,32 @@ public function testDuplicated(): void
4949
$this->assertSame($r2, $parameters->get('a'));
5050
$this->assertSame($r4, $parameters->get('a', HeaderParameter::class));
5151
}
52+
53+
public function testPropertyPlaceholderKeysAreNotDeduplicated(): void
54+
{
55+
$r1 = new QueryParameter(key: ':property', properties: ['field1', 'field2']);
56+
$r2 = new QueryParameter(key: ':property', properties: ['field3', 'field4']);
57+
$parameters = new Parameters([$r1, $r2]);
58+
59+
$this->assertCount(2, $parameters);
60+
61+
$collected = [];
62+
foreach ($parameters as $key => $parameter) {
63+
$collected[] = [$key, $parameter];
64+
}
65+
66+
$this->assertSame(':property', $collected[0][0]);
67+
$this->assertSame(':property', $collected[1][0]);
68+
$this->assertSame(['field1', 'field2'], $collected[0][1]->getProperties());
69+
$this->assertSame(['field3', 'field4'], $collected[1][1]->getProperties());
70+
}
71+
72+
public function testPropertyPlaceholderKeysAreNotDeduplicatedViaAdd(): void
73+
{
74+
$parameters = new Parameters();
75+
$parameters->add(':property', new QueryParameter(key: ':property', properties: ['field1', 'field2']));
76+
$parameters->add(':property', new QueryParameter(key: ':property', properties: ['field3', 'field4']));
77+
78+
$this->assertCount(2, $parameters);
79+
}
5280
}

src/Metadata/Tests/Resource/Factory/ParameterResourceMetadataCollectionFactoryTest.php

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,40 @@ public function testNestedPropertyWithNameConverter(): void
323323
$this->assertSame('search[related.nested]', $searchNestedParam->getKey());
324324
}
325325

326+
public function testRepeatedPropertyPlaceholderAttributesExpandPerPropertyFilter(): void
327+
{
328+
$nameCollection = $this->createStub(PropertyNameCollectionFactoryInterface::class);
329+
$nameCollection->method('create')->willReturn(new PropertyNameCollection(['field1', 'field2', 'field3', 'field4']));
330+
331+
$propertyMetadata = $this->createStub(PropertyMetadataFactoryInterface::class);
332+
$propertyMetadata->method('create')->willReturn(new ApiProperty(readable: true));
333+
334+
$filterLocator = $this->createStub(ContainerInterface::class);
335+
$filterLocator->method('has')->willReturn(false);
336+
337+
$parameterFactory = new ParameterResourceMetadataCollectionFactory(
338+
$nameCollection,
339+
$propertyMetadata,
340+
new AttributesResourceMetadataCollectionFactory(),
341+
$filterLocator
342+
);
343+
344+
$collection = $parameterFactory->create(HasRepeatedPropertyPlaceholderParameter::class);
345+
$operation = $collection->getOperation(forceCollection: true);
346+
$parameters = $operation->getParameters();
347+
348+
$this->assertInstanceOf(Parameters::class, $parameters);
349+
350+
foreach (['field1', 'field2', 'field3', 'field4'] as $field) {
351+
$this->assertTrue($parameters->has($field), \sprintf('Parameter "%s" should exist after :property expansion', $field));
352+
}
353+
354+
$this->assertInstanceOf(RepeatedPlaceholderExactFilter::class, $parameters->get('field1')->getFilter());
355+
$this->assertInstanceOf(RepeatedPlaceholderExactFilter::class, $parameters->get('field2')->getFilter());
356+
$this->assertInstanceOf(RepeatedPlaceholderBooleanFilter::class, $parameters->get('field3')->getFilter());
357+
$this->assertInstanceOf(RepeatedPlaceholderBooleanFilter::class, $parameters->get('field4')->getFilter());
358+
}
359+
326360
private function createNestedPropertyFactory(): ParameterResourceMetadataCollectionFactory
327361
{
328362
$nameCollection = $this->createStub(PropertyNameCollectionFactoryInterface::class);
@@ -428,6 +462,34 @@ public function testSimplePropertyHasNoNestedPropertyInfo(): void
428462
}
429463
}
430464

465+
class RepeatedPlaceholderExactFilter implements FilterInterface
466+
{
467+
public function getDescription(string $resourceClass): array
468+
{
469+
return [];
470+
}
471+
}
472+
473+
class RepeatedPlaceholderBooleanFilter implements FilterInterface
474+
{
475+
public function getDescription(string $resourceClass): array
476+
{
477+
return [];
478+
}
479+
}
480+
481+
#[ApiResource]
482+
#[QueryParameter(key: ':property', filter: new RepeatedPlaceholderExactFilter(), properties: ['field1', 'field2'])]
483+
#[QueryParameter(key: ':property', filter: new RepeatedPlaceholderBooleanFilter(), properties: ['field3', 'field4'])]
484+
class HasRepeatedPropertyPlaceholderParameter
485+
{
486+
public $id;
487+
public $field1;
488+
public $field2;
489+
public $field3;
490+
public $field4;
491+
}
492+
431493
#[ApiResource(
432494
operations: [
433495
new GetCollection(

0 commit comments

Comments
 (0)