Skip to content

Commit 2e935d4

Browse files
carnagecarnage
authored
Merge pull request #16 from membrane-php/fix-style-conflict-check
Adjust Parameter conflict criteria
2 parents ef539f3 + e5cd7f5 commit 2e935d4

9 files changed

Lines changed: 182 additions & 237 deletions

File tree

‎src/Exception/InvalidOpenAPI.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,16 @@ public static function contentMissingMediaType(Identifier $identifier): self
280280
return new self($message);
281281
}
282282

283+
public static function deepObjectMustBeObject(Identifier $identifier): self
284+
{
285+
$message = <<<TEXT
286+
$identifier
287+
style:deepObject is only applicable to schemas of type:object
288+
Therefore, your schema MUST be valid ONLY as type:object
289+
TEXT;
290+
291+
return new self($message);
292+
}
283293
public static function invalidType(Identifier $identifier, string $type): self
284294
{
285295
$message = <<<TEXT

‎src/ValueObject/Valid/V30/Operation.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ private function validateParameters(
127127
);
128128
}
129129

130-
if ($parameter->canConflict($otherParameter)) {
130+
if ($parameter->canConflictWith($otherParameter)) {
131131
throw CannotSupport::conflictingParameterStyles(
132132
(string) $parameter->getIdentifier(),
133133
(string) $otherParameter->getIdentifier(),

‎src/ValueObject/Valid/V30/Parameter.php‎

Lines changed: 58 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
namespace Membrane\OpenAPIReader\ValueObject\Valid\V30;
66

77
use Membrane\OpenAPIReader\Exception\InvalidOpenAPI;
8-
use Membrane\OpenAPIReader\OpenAPIVersion;
98
use Membrane\OpenAPIReader\ValueObject\Partial;
109
use Membrane\OpenAPIReader\ValueObject\Valid\Enum\In;
1110
use Membrane\OpenAPIReader\ValueObject\Valid\Enum\Style;
@@ -67,17 +66,8 @@ public function __construct(
6766
$parameter->required
6867
);
6968

70-
$this->style = $this->validateStyle(
71-
$identifier,
72-
$this->in,
73-
$parameter->style,
74-
);
75-
76-
$this->explode = $parameter->explode ?? $this->style->defaultExplode();
77-
78-
if (isset($parameter->schema) !== empty($parameter->content)) {
69+
isset($parameter->schema) === empty($parameter->content) ?:
7970
throw InvalidOpenAPI::mustHaveSchemaXorContent($parameter->name);
80-
}
8171

8272
if (isset($parameter->schema)) {
8373
$this->content = [];
@@ -95,7 +85,14 @@ public function __construct(
9585
);
9686
}
9787

98-
$this->checkStyleSuitability();
88+
$this->style = $this->validateStyle(
89+
$identifier,
90+
$this->getSchema(),
91+
$this->in,
92+
$parameter->style,
93+
);
94+
95+
$this->explode = $parameter->explode ?? $this->style->defaultExplode();
9996
}
10097

10198
public function getSchema(): Schema
@@ -129,24 +126,42 @@ public function isSimilar(Parameter $other): bool
129126
mb_strtolower($this->name) === mb_strtolower($other->name);
130127
}
131128

132-
public function canConflict(Parameter $other): bool
129+
public function canConflictWith(Parameter $other): bool
133130
{
134-
if (
135-
$this->in !== $other->in || // parameter can be identified by differing location
136-
$this->style !== $other->style || // parameter can be identified by differing style
137-
$this->in !== In::Query
138-
) {
139-
return false;
140-
}
131+
return ($this->canCauseConflict() && $other->isVulnerableToConflict()) ||
132+
($this->isVulnerableToConflict() && $other->canCauseConflict());
133+
}
134+
135+
private function canCauseConflict(): bool
136+
{
137+
return $this->in === In::Query &&
138+
$this->style === Style::Form &&
139+
$this->explode &&
140+
$this->getSchema()->canBe(Type::Object);
141+
}
141142

142-
return match ($this->style) {
143-
Style::Form =>
144-
$this->explode &&
145-
$other->explode &&
146-
$this->getSchema()->canBe(Type::Object),
147-
Style::PipeDelimited, Style::SpaceDelimited =>
148-
!$this->getSchema()->canOnlyBePrimitive(),
149-
default => false,
143+
private function isVulnerableToConflict(): bool
144+
{
145+
/**
146+
* @todo once schemas account for minItems and minProperties keywords.
147+
* pipeDelimited and spaceDelimited are also vulnerable if:
148+
* type:array and minItems <= 1
149+
* this is because there would be no delimiter to distinguish it from a form parameter
150+
*
151+
* form would not be vulnerable if:
152+
* explode:false
153+
* and...
154+
* type:object and minProperties > 1
155+
* or ...
156+
* type:array and minItems > 1
157+
* this is because there would be a delimiter to distinguish it from an exploding parameter
158+
*/
159+
160+
return $this->in === In::Query && match ($this->style) {
161+
Style::Form => true,
162+
Style::PipeDelimited,
163+
Style::SpaceDelimited => $this->getSchema()->canBePrimitive(),
164+
default => false
150165
};
151166
}
152167

@@ -174,6 +189,7 @@ private function validateRequired(
174189

175190
private function validateStyle(
176191
Identifier $identifier,
192+
Schema $schema,
177193
In $in,
178194
?string $style
179195
): Style {
@@ -187,6 +203,20 @@ private function validateStyle(
187203
$style->isAllowed($in) ?:
188204
throw InvalidOpenAPI::parameterIncompatibleStyle($identifier);
189205

206+
$style !== Style::DeepObject || $schema->canOnlyBe(Type::Object) ?:
207+
throw InvalidOpenAPI::deepObjectMustBeObject($identifier);
208+
209+
if (
210+
in_array($style, [Style::SpaceDelimited, Style::PipeDelimited]) &&
211+
$schema->canBePrimitive()
212+
) {
213+
$this->addWarning(
214+
"style:$style->value, is not allowed to be primitive." .
215+
'In these instances style:form is recommended.',
216+
Warning::UNSUITABLE_STYLE
217+
);
218+
}
219+
190220
return $style;
191221
}
192222

@@ -218,20 +248,4 @@ private function validateContent(
218248
),
219249
];
220250
}
221-
222-
private function checkStyleSuitability(): void
223-
{
224-
foreach (Type::casesForVersion(OpenAPIVersion::Version_3_0) as $type) {
225-
if (
226-
$this->getSchema()->canBe($type) &&
227-
$this->style->isSuitableFor($type)
228-
) {
229-
return;
230-
}
231-
}
232-
$this->addWarning(
233-
'unsuitable style for primitive data types',
234-
Warning::UNSUITABLE_STYLE
235-
);
236-
}
237251
}

‎src/ValueObject/Valid/V30/Schema.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,15 @@ public function canOnlyBe(Type $type): bool
129129
return [$type] === $this->typesItCanBe;
130130
}
131131

132-
public function canOnlyBePrimitive(): bool
132+
public function canBePrimitive(): bool
133133
{
134-
return !$this->canBe(Type::Array) && !$this->canBe(Type::Object);
134+
foreach ($this->typesItCanBe as $typeItCanBe) {
135+
if ($typeItCanBe->isPrimitive()) {
136+
return true;
137+
}
138+
}
139+
140+
return false;
135141
}
136142

137143
/** @return string[] */

‎tests/MembraneReaderTest.php‎

Lines changed: 15 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -602,64 +602,33 @@ public static function provideConflictingParameters(): Generator
602602
'responses' => [200 => ['description' => ' Successful Response']]
603603
];
604604

605-
yield 'operation with two spaceDelimited exploding arrays' => [
606-
json_encode(
607-
$openAPI($path(
608-
[],
609-
$operation([
610-
'operationId' => 'test-op',
611-
'parameters' => [
612-
[
613-
'name' => 'param1',
614-
'in' => 'query',
615-
'explode' => true,
616-
'style' => 'spaceDelimited',
617-
'schema' => ['type' => 'array'],
618-
],
619-
[
620-
'name' => 'param2',
621-
'in' => 'query',
622-
'explode' => true,
623-
'style' => 'spaceDelimited',
624-
'schema' => ['type' => 'array'],
625-
]
626-
],
627-
])
628-
))
629-
),
630-
CannotSupport::conflictingParameterStyles(
631-
'["test-api(1.0.0)"]["/path"]["test-op(get)"]["param1(query)"]',
632-
'["test-api(1.0.0)"]["/path"]["test-op(get)"]["param2(query)"]',
633-
)
634-
];
635-
636-
yield 'spaceDelimited exploding in path, pipeDelimited exploding in query and spaceDelimited exploding in query' => [
605+
yield 'form exploding object in path, pipeDelimited object in path, form exploding in operation' => [
637606
json_encode(
638607
$openAPI($path(
639608
[
640609
[
641610
'name' => 'param1',
642611
'in' => 'query',
643612
'explode' => true,
644-
'style' => 'spaceDelimited',
645-
'schema' => ['type' => 'array'],
613+
'style' => 'form',
614+
'schema' => ['type' => 'object'],
615+
],
616+
[
617+
'name' => 'param2',
618+
'in' => 'query',
619+
'explode' => true,
620+
'style' => 'pipeDelimited',
621+
'schema' => ['type' => 'object'],
646622
],
647623
],
648624
$operation([
649625
'operationId' => 'test-op',
650626
'parameters' => [
651-
[
652-
'name' => 'param2',
653-
'in' => 'query',
654-
'explode' => true,
655-
'style' => 'pipeDelimited',
656-
'schema' => ['type' => 'array'],
657-
],
658627
[
659628
'name' => 'param3',
660629
'in' => 'query',
661630
'explode' => true,
662-
'style' => 'spaceDelimited',
631+
'style' => 'form',
663632
'schema' => ['type' => 'array'],
664633
]
665634
],
@@ -672,7 +641,7 @@ public static function provideConflictingParameters(): Generator
672641
)
673642
];
674643

675-
yield 'form exploding object in path, pipeDelimited object in path, pipeDelimited exploding in query' => [
644+
yield 'form exploding object in path, pipeDelimited primitive in path, form exploding in operation' => [
676645
json_encode(
677646
$openAPI($path(
678647
[
@@ -688,25 +657,14 @@ public static function provideConflictingParameters(): Generator
688657
'in' => 'query',
689658
'explode' => true,
690659
'style' => 'pipeDelimited',
691-
'schema' => ['type' => 'object'],
660+
'schema' => ['type' => 'string'],
692661
],
693662
],
694-
$operation([
695-
'operationId' => 'test-op',
696-
'parameters' => [
697-
[
698-
'name' => 'param3',
699-
'in' => 'query',
700-
'explode' => true,
701-
'style' => 'pipeDelimited',
702-
'schema' => ['type' => 'array'],
703-
]
704-
],
705-
])
663+
$operation(['operationId' => 'test-op'])
706664
))
707665
),
708666
CannotSupport::conflictingParameterStyles(
709-
'["test-api(1.0.0)"]["/path"]["test-op(get)"]["param3(query)"]',
667+
'["test-api(1.0.0)"]["/path"]["param1(query)"]',
710668
'["test-api(1.0.0)"]["/path"]["param2(query)"]',
711669
)
712670
];

0 commit comments

Comments
 (0)