Skip to content

Adjust Parameter conflict criteria #16

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.

Sign up for GitHub

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

Merged
carnage merged 1 commit into from
Apr 25, 2024
Merged

Adjust Parameter conflict criteria #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/Exception/InvalidOpenAPI.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,16 @@ public static function contentMissingMediaType(Identifier $identifier): self
return new self($message);
}

public static function deepObjectMustBeObject(Identifier $identifier): self
{
$message = <<<TEXT
$identifier
style:deepObject is only applicable to schemas of type:object
Therefore, your schema MUST be valid ONLY as type:object
TEXT;

return new self($message);
}
public static function invalidType(Identifier $identifier, string $type): self
{
$message = <<<TEXT
Expand Down
2 changes: 1 addition & 1 deletion src/ValueObject/Valid/V30/Operation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ private function validateParameters(
);
}

if ($parameter->canConflict($otherParameter)) {
if ($parameter->canConflictWith($otherParameter)) {
throw CannotSupport::conflictingParameterStyles(
(string) $parameter->getIdentifier(),
(string) $otherParameter->getIdentifier(),
Expand Down
102 changes: 58 additions & 44 deletions src/ValueObject/Valid/V30/Parameter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
namespace Membrane\OpenAPIReader\ValueObject\Valid\V30;

use Membrane\OpenAPIReader\Exception\InvalidOpenAPI;
use Membrane\OpenAPIReader\OpenAPIVersion;
use Membrane\OpenAPIReader\ValueObject\Partial;
use Membrane\OpenAPIReader\ValueObject\Valid\Enum\In;
use Membrane\OpenAPIReader\ValueObject\Valid\Enum\Style;
Expand Down Expand Up @@ -67,17 +66,8 @@ public function __construct(
$parameter->required
);

$this->style = $this->validateStyle(
$identifier,
$this->in,
$parameter->style,
);

$this->explode = $parameter->explode ?? $this->style->defaultExplode();

if (isset($parameter->schema) !== empty($parameter->content)) {
isset($parameter->schema) === empty($parameter->content) ?:
throw InvalidOpenAPI::mustHaveSchemaXorContent($parameter->name);
}

if (isset($parameter->schema)) {
$this->content = [];
Expand All @@ -95,7 +85,14 @@ public function __construct(
);
}

$this->checkStyleSuitability();
$this->style = $this->validateStyle(
$identifier,
$this->getSchema(),
$this->in,
$parameter->style,
);

$this->explode = $parameter->explode ?? $this->style->defaultExplode();
}

public function getSchema(): Schema
Expand Down Expand Up @@ -129,24 +126,42 @@ public function isSimilar(Parameter $other): bool
mb_strtolower($this->name) === mb_strtolower($other->name);
}

public function canConflict(Parameter $other): bool
public function canConflictWith(Parameter $other): bool
{
if (
$this->in !== $other->in || // parameter can be identified by differing location
$this->style !== $other->style || // parameter can be identified by differing style
$this->in !== In::Query
) {
return false;
}
return ($this->canCauseConflict() && $other->isVulnerableToConflict()) ||
($this->isVulnerableToConflict() && $other->canCauseConflict());
}

private function canCauseConflict(): bool
{
return $this->in === In::Query &&
$this->style === Style::Form &&
$this->explode &&
$this->getSchema()->canBe(Type::Object);
}

return match ($this->style) {
Style::Form =>
$this->explode &&
$other->explode &&
$this->getSchema()->canBe(Type::Object),
Style::PipeDelimited, Style::SpaceDelimited =>
!$this->getSchema()->canOnlyBePrimitive(),
default => false,
private function isVulnerableToConflict(): bool
{
/**
* @todo once schemas account for minItems and minProperties keywords.
* pipeDelimited and spaceDelimited are also vulnerable if:
* type:array and minItems <= 1
* this is because there would be no delimiter to distinguish it from a form parameter
*
* form would not be vulnerable if:
* explode:false
* and...
* type:object and minProperties > 1
* or ...
* type:array and minItems > 1
* this is because there would be a delimiter to distinguish it from an exploding parameter
*/

return $this->in === In::Query && match ($this->style) {
Style::Form => true,
Style::PipeDelimited,
Style::SpaceDelimited => $this->getSchema()->canBePrimitive(),
default => false
};
}

Expand Down Expand Up @@ -174,6 +189,7 @@ private function validateRequired(

private function validateStyle(
Identifier $identifier,
Schema $schema,
In $in,
?string $style
): Style {
Expand All @@ -187,6 +203,20 @@ private function validateStyle(
$style->isAllowed($in) ?:
throw InvalidOpenAPI::parameterIncompatibleStyle($identifier);

$style !== Style::DeepObject || $schema->canOnlyBe(Type::Object) ?:
throw InvalidOpenAPI::deepObjectMustBeObject($identifier);

if (
in_array($style, [Style::SpaceDelimited, Style::PipeDelimited]) &&
$schema->canBePrimitive()
) {
$this->addWarning(
"style:$style->value, is not allowed to be primitive." .
'In these instances style:form is recommended.',
Warning::UNSUITABLE_STYLE
);
}

return $style;
}

Expand Down Expand Up @@ -218,20 +248,4 @@ private function validateContent(
),
];
}

private function checkStyleSuitability(): void
{
foreach (Type::casesForVersion(OpenAPIVersion::Version_3_0) as $type) {
if (
$this->getSchema()->canBe($type) &&
$this->style->isSuitableFor($type)
) {
return;
}
}
$this->addWarning(
'unsuitable style for primitive data types',
Warning::UNSUITABLE_STYLE
);
}
}
10 changes: 8 additions & 2 deletions src/ValueObject/Valid/V30/Schema.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,15 @@ public function canOnlyBe(Type $type): bool
return [$type] === $this->typesItCanBe;
}

public function canOnlyBePrimitive(): bool
public function canBePrimitive(): bool
{
return !$this->canBe(Type::Array) && !$this->canBe(Type::Object);
foreach ($this->typesItCanBe as $typeItCanBe) {
if ($typeItCanBe->isPrimitive()) {
return true;
}
}

return false;
}

/** @return string[] */
Expand Down
72 changes: 15 additions & 57 deletions tests/MembraneReaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,64 +602,33 @@ public static function provideConflictingParameters(): Generator
'responses' => [200 => ['description' => ' Successful Response']]
];

yield 'operation with two spaceDelimited exploding arrays' => [
json_encode(
$openAPI($path(
[],
$operation([
'operationId' => 'test-op',
'parameters' => [
[
'name' => 'param1',
'in' => 'query',
'explode' => true,
'style' => 'spaceDelimited',
'schema' => ['type' => 'array'],
],
[
'name' => 'param2',
'in' => 'query',
'explode' => true,
'style' => 'spaceDelimited',
'schema' => ['type' => 'array'],
]
],
])
))
),
CannotSupport::conflictingParameterStyles(
'["test-api(1.0.0)"]["/path"]["test-op(get)"]["param1(query)"]',
'["test-api(1.0.0)"]["/path"]["test-op(get)"]["param2(query)"]',
)
];

yield 'spaceDelimited exploding in path, pipeDelimited exploding in query and spaceDelimited exploding in query' => [
yield 'form exploding object in path, pipeDelimited object in path, form exploding in operation' => [
json_encode(
$openAPI($path(
[
[
'name' => 'param1',
'in' => 'query',
'explode' => true,
'style' => 'spaceDelimited',
'schema' => ['type' => 'array'],
'style' => 'form',
'schema' => ['type' => 'object'],
],
[
'name' => 'param2',
'in' => 'query',
'explode' => true,
'style' => 'pipeDelimited',
'schema' => ['type' => 'object'],
],
],
$operation([
'operationId' => 'test-op',
'parameters' => [
[
'name' => 'param2',
'in' => 'query',
'explode' => true,
'style' => 'pipeDelimited',
'schema' => ['type' => 'array'],
],
[
'name' => 'param3',
'in' => 'query',
'explode' => true,
'style' => 'spaceDelimited',
'style' => 'form',
'schema' => ['type' => 'array'],
]
],
Expand All @@ -672,7 +641,7 @@ public static function provideConflictingParameters(): Generator
)
];

yield 'form exploding object in path, pipeDelimited object in path, pipeDelimited exploding in query' => [
yield 'form exploding object in path, pipeDelimited primitive in path, form exploding in operation' => [
json_encode(
$openAPI($path(
[
Expand All @@ -688,25 +657,14 @@ public static function provideConflictingParameters(): Generator
'in' => 'query',
'explode' => true,
'style' => 'pipeDelimited',
'schema' => ['type' => 'object'],
'schema' => ['type' => 'string'],
],
],
$operation([
'operationId' => 'test-op',
'parameters' => [
[
'name' => 'param3',
'in' => 'query',
'explode' => true,
'style' => 'pipeDelimited',
'schema' => ['type' => 'array'],
]
],
])
$operation(['operationId' => 'test-op'])
))
),
CannotSupport::conflictingParameterStyles(
'["test-api(1.0.0)"]["/path"]["test-op(get)"]["param3(query)"]',
'["test-api(1.0.0)"]["/path"]["param1(query)"]',
'["test-api(1.0.0)"]["/path"]["param2(query)"]',
)
];
Expand Down
Loading