diff --git a/src/Exception/InvalidOpenAPI.php b/src/Exception/InvalidOpenAPI.php index 9e6a936..b93d692 100644 --- a/src/Exception/InvalidOpenAPI.php +++ b/src/Exception/InvalidOpenAPI.php @@ -280,6 +280,16 @@ public static function contentMissingMediaType(Identifier $identifier): self return new self($message); } + public static function deepObjectMustBeObject(Identifier $identifier): self + { + $message = <<canConflict($otherParameter)) { + if ($parameter->canConflictWith($otherParameter)) { throw CannotSupport::conflictingParameterStyles( (string) $parameter->getIdentifier(), (string) $otherParameter->getIdentifier(), diff --git a/src/ValueObject/Valid/V30/Parameter.php b/src/ValueObject/Valid/V30/Parameter.php index 6e1525b..c4ec69f 100644 --- a/src/ValueObject/Valid/V30/Parameter.php +++ b/src/ValueObject/Valid/V30/Parameter.php @@ -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; @@ -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 = []; @@ -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 @@ -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 }; } @@ -174,6 +189,7 @@ private function validateRequired( private function validateStyle( Identifier $identifier, + Schema $schema, In $in, ?string $style ): Style { @@ -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; } @@ -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 - ); - } } diff --git a/src/ValueObject/Valid/V30/Schema.php b/src/ValueObject/Valid/V30/Schema.php index af09b48..73e5d29 100644 --- a/src/ValueObject/Valid/V30/Schema.php +++ b/src/ValueObject/Valid/V30/Schema.php @@ -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[] */ diff --git a/tests/MembraneReaderTest.php b/tests/MembraneReaderTest.php index 6a7cf5b..98eb6d7 100644 --- a/tests/MembraneReaderTest.php +++ b/tests/MembraneReaderTest.php @@ -602,38 +602,7 @@ 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( [ @@ -641,25 +610,25 @@ public static function provideConflictingParameters(): Generator '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'], ] ], @@ -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( [ @@ -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)"]', ) ]; diff --git a/tests/ReaderTest.php b/tests/ReaderTest.php index b7adae6..4b3e061 100644 --- a/tests/ReaderTest.php +++ b/tests/ReaderTest.php @@ -524,38 +524,7 @@ 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( [ @@ -563,25 +532,25 @@ public static function provideConflictingParameters(): Generator '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'], ] ], @@ -594,7 +563,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( [ @@ -610,25 +579,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)"]', ) ]; diff --git a/tests/ValueObject/Valid/V30/MediaTypeTest.php b/tests/ValueObject/Valid/V30/MediaTypeTest.php index 2016fa5..2758365 100644 --- a/tests/ValueObject/Valid/V30/MediaTypeTest.php +++ b/tests/ValueObject/Valid/V30/MediaTypeTest.php @@ -12,6 +12,7 @@ use Membrane\OpenAPIReader\ValueObject\Valid\V30\MediaType; use Membrane\OpenAPIReader\ValueObject\Valid\V30\Schema; use Membrane\OpenAPIReader\ValueObject\Valid\Validated; +use Membrane\OpenAPIReader\ValueObject\Valid\Warnings; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\UsesClass; @@ -25,6 +26,7 @@ #[UsesClass(Partial\Schema::class)] #[UsesClass(Schema::class)] #[UsesClass(Type::class)] +#[UsesClass(Warnings::class)] class MediaTypeTest extends TestCase { #[Test] diff --git a/tests/ValueObject/Valid/V30/ParameterTest.php b/tests/ValueObject/Valid/V30/ParameterTest.php index 988810d..130bc01 100644 --- a/tests/ValueObject/Valid/V30/ParameterTest.php +++ b/tests/ValueObject/Valid/V30/ParameterTest.php @@ -85,7 +85,11 @@ public function itCanHaveAnyValidStylePerLocation( ): void { $sut = new Parameter( new Identifier('test'), - PartialHelper::createParameter(in: $in->value, style: $style->value) + PartialHelper::createParameter( + in: $in->value, + style: $style->value, + schema: PartialHelper::createSchema(type: Type::Object->value), + ) ); self::assertSame($style, $sut->style); @@ -114,7 +118,8 @@ public function itCanDefaultExplodePerStyle(In $in, Style $style): void PartialHelper::createParameter( in: $in->value, style: $style->value, - explode: null + explode: null, + schema: PartialHelper::createSchema(type: Type::Object->value), ) ); @@ -175,26 +180,44 @@ public function itChecksIfNameIsSimilar( self::assertSame($expected, $sut->isSimilar($otherSUT)); } + #[Test] + public function itInvalidesDeepObjectThatIsNotStrictlyObject(): void { + $identifier = new Identifier(''); + $name = 'test-param'; + $in = In::Query->value; + $partialParameter = PartialHelper::createParameter( + name: $name, + in: $in, + style: Style::DeepObject->value, + schema: PartialHelper::createSchema(), + ); + + self::expectExceptionObject( + InvalidOpenAPI::deepObjectMustBeObject($identifier->append($name, $in)) + ); + + $sut = new Parameter($identifier, $partialParameter); + + } + #[Test, DataProvider('provideStylesThatMayBeSuitable')] public function itWarnsAgainstUnsuitableStyles( - bool $expected, + bool $isUnsuitable, Partial\Parameter $parameter, ): void { $sut = new Parameter(new Identifier(''), $parameter); - self::assertSame( - $expected, - $sut->getWarnings()->hasWarningCode(Warning::UNSUITABLE_STYLE) - ); + self::assertSame($isUnsuitable, $sut + ->getWarnings() + ->hasWarningCode(Warning::UNSUITABLE_STYLE)); } #[Test] #[DataProvider('provideParametersThatAreNotInQuery')] #[DataProvider('provideParametersWithDifferentStyles')] #[DataProvider('provideParametersThatMustBePrimitiveType')] - #[DataProvider('provideParametersWithDifferentExplodes')] #[DataProvider('provideParametersInQueryThatDoNotConflict')] - #[DataProvider('provideParametersThatConflict')] + #[DataProvider('provideVulnerableParametersExplodingFormObjects')] public function itChecksIfItCanConflict( bool $expected, Partial\Parameter $parameter, @@ -203,7 +226,7 @@ public function itChecksIfItCanConflict( $sut = new Parameter(new Identifier(''), $parameter); $otherSUT = new Parameter(new Identifier(''), $other); - self::assertSame($expected, $sut->canConflict($otherSUT)); + self::assertSame($expected, $sut->canConflictWith($otherSUT)); } public static function provideInvalidPartialParameters(): Generator @@ -496,8 +519,11 @@ public static function provideStylesThatMayBeSuitable(): Generator { foreach (Type::casesForVersion(OpenAPIVersion::Version_3_0) as $type) { foreach (Style::cases() as $style) { + if ($style === Style::DeepObject && $type !== Type::Object) { + continue; // this unsuitability invalidates the OpenAPI + } yield "style:$style->value, type:$type->value" => [ - !$style->isSuitableFor($type), + in_array($style, [Style::SpaceDelimited, Style::PipeDelimited]) && $type->isPrimitive(), PartialHelper::createParameter( in: array_values(array_filter( In::cases(), @@ -566,50 +592,28 @@ public static function provideParametersWithDifferentStyles(): Generator $style = array_pop($availableStyles); foreach ($availableStyles as $other) { yield "style:$style->value and other style:$other->value" => [ - false, + in_array(Style::Form, [$style, $other]) && !in_array(Style::DeepObject, [$style, $other]), PartialHelper::createParameter( in: In::Query->value, style: $style->value, explode: true, + schema: PartialHelper::createSchema( + type: $style === Style::DeepObject ? 'object' : null, + ) ), PartialHelper::createParameter( in: In::Query->value, style: $other->value, explode: true, + schema: PartialHelper::createSchema( + type: $other === Style::DeepObject ? 'object' : null, + ) ), ]; } } } - /** - * @return Generator - */ - public static function provideParametersWithDifferentExplodes(): Generator - { - $stylesWhereDifferentExplodesMatter = [Style::Form]; - - foreach ($stylesWhereDifferentExplodesMatter as $style) { - yield "style:$style->value" => [ - false, - PartialHelper::createParameter( - in: In::Query->value, - style: $style->value, - explode: true, - ), - PartialHelper::createParameter( - in: In::Query->value, - style: $style->value, - explode: false, - ), - ]; - } - } - /** * @return Generatorvalue" => [ - false, - PartialHelper::createParameter( - in: In::Query->value, - style: $style->value, - explode: true, - ), - PartialHelper::createParameter( - in: In::Query->value, - style: $style->value, - explode: true, - ), - ]; - } + yield 'deepObject' => [ + false, + PartialHelper::createParameter( + in: In::Query->value, + style: Style::DeepObject->value, + explode: true, + schema: PartialHelper::createSchema(type: Type::Object->value) + ), + PartialHelper::createParameter( + in: In::Query->value, + style: Style::DeepObject->value, + explode: true, + schema: PartialHelper::createSchema(type: Type::Object->value) + ), + ]; } /** @@ -645,15 +647,15 @@ public static function provideParametersInQueryThatDoNotConflict(): Generator * 2: Partial\Parameter, * }> */ - public static function provideParametersThatConflict(): Generator + public static function provideVulnerableParametersExplodingFormObjects(): Generator { $dataSet = fn(Style $style, bool $explode, Type $type) => [ true, PartialHelper::createParameter( in: In::Query->value, - style: $style->value, - explode: $explode, - schema: PartialHelper::createSchema(type: $type->value) + style: Style::Form->value, + explode: true, + schema: PartialHelper::createSchema(type: Type::Object->value) ), PartialHelper::createParameter( in: In::Query->value, @@ -666,18 +668,11 @@ public static function provideParametersThatConflict(): Generator yield 'style:form and explode:true for object data types' => $dataSet(Style::Form, true, Type::Object); + yield 'style:pipeDelimited with primitive data type' => + $dataSet(Style::PipeDelimited, false, Type::String); - yield 'style:pipeDelimited for array data types' => - $dataSet(Style::PipeDelimited, false, Type::Array); - - yield 'style:pipeDelimited for object data types' => - $dataSet(Style::PipeDelimited, false, Type::Object); - - yield 'style:spaceDelimited for array data types' => - $dataSet(Style::SpaceDelimited, false, Type::Array); - - yield 'style:spaceDelimited for object data types' => - $dataSet(Style::SpaceDelimited, false, Type::Object); + yield 'style:spaceDelimited with primitive data type' => + $dataSet(Style::SpaceDelimited, false, Type::String); } /** @@ -689,7 +684,7 @@ public static function provideParametersThatConflict(): Generator */ public static function provideParametersThatMustBePrimitiveType(): Generator { - foreach (self::provideParametersThatConflict() as $case => $dataSet) { + foreach (self::provideVulnerableParametersExplodingFormObjects() as $case => $dataSet) { $dataSet[1]->schema = PartialHelper::createSchema(type: Type::Integer->value); $dataSet[2]->schema = PartialHelper::createSchema(type: Type::Integer->value); diff --git a/tests/ValueObject/Valid/V30/SchemaTest.php b/tests/ValueObject/Valid/V30/SchemaTest.php index af5eda9..5568c12 100644 --- a/tests/ValueObject/Valid/V30/SchemaTest.php +++ b/tests/ValueObject/Valid/V30/SchemaTest.php @@ -95,9 +95,11 @@ public function itKnowsIfItCanOnlyBePrimitive( $sut = new Schema(new Identifier(''), $partialSchema); self::assertSame( - !in_array(Type::Object, $typesItCanBe) && - !in_array(Type::Array, $typesItCanBe), - $sut->canOnlyBePrimitive() + !empty(array_filter($typesItCanBe, fn($t) => !in_array( + $t, + [Type::Array, Type::Object] + ))), + $sut->canBePrimitive() ); }