Skip to content
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.

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

Update Swagger DocumentationNormalizer for AWS API-Gateway #1697

Merged
merged 9 commits into from
Mar 6, 2018

Conversation

vincentchalamon
Copy link
Contributor

@vincentchalamon vincentchalamon commented Feb 9, 2018

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR api-platform/docs#395

This PR represents the changes required on the /docs.json Swagger doc to be valid to AWS API Gateway.

@@ -609,7 +605,7 @@ private function computeDoc(Documentation $documentation, \ArrayObject $definiti
{
$doc = [
'swagger' => self::SWAGGER_VERSION,
'basePath' => $context['base_url'] ?? '/',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my case, basePath is empty, which is not valid according to Swagger 2.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

($context['base_url'] ?? '/') ?: '/'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty checks if the variable is defined and not empty:
capture d ecran 2018-02-09 a 20 08 01

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(it's the line below this modification. GitHub shows a removal, but it's a line edit)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I know for empty, it's because my version is shorter, but nitpicking, keep it as is)

@@ -155,7 +155,7 @@ public function normalize($object, $format = null, array $context = [])

if ($parameters = $this->getFiltersParameters($resourceClass, $operationName, $subResourceMetadata, $definitions, $serializerContext)) {
foreach ($parameters as $parameter) {
if (!\in_array($parameter['name'], $parametersMemory, true)) {
if (!\in_array($parameter['name'], $parametersMemory, true) && preg_match('/^[a-zA-Z0-9._$-]+$/', $parameter['name'])) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex really sucks, it doesn't allow array filters like ?foo[]=bar, but this pattern is required by API Gateway on Swagger doc import

@@ -511,10 +511,6 @@ private function getPropertySchema(PropertyMetadata $propertyMetadata, \ArrayObj
{
$propertySchema = new \ArrayObject($propertyMetadata->getAttributes()['swagger_context'] ?? []);

if (false === $propertyMetadata->isWritable()) {
$propertySchema['readOnly'] = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be removed, but API Gateway doesn't support it:

The readOnly field is not supported.

@@ -690,6 +686,9 @@ private function getFiltersParameters(string $resourceClass, string $operationNa
}

foreach ($filter->getDescription($resourceClass) as $name => $data) {
if (!preg_match('/^[a-zA-Z0-9._$-]+$/', $name)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same: this regex really sucks, it doesn't allow array filters like ?foo[]=bar, but this pattern is required by API Gateway on Swagger doc import

@dunglas
Copy link
Member

dunglas commented Feb 9, 2018

Other idea: can't we create a decorator that will remove all things that API Gateway doesn't allow, create a command like our swagger:export using it and ship it as an external bundle?

@vincentchalamon
Copy link
Contributor Author

@dunglas I've updated this PR to enabled API Gateway support only if api_gateway query parameter is present.

@@ -66,6 +67,7 @@
private $paginationPageParameterName;
private $clientItemsPerPage;
private $itemsPerPageParameterName;
private $apiGateway = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we do it without introducing a state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to, but final code would have been much more complex

@vincentchalamon vincentchalamon changed the title Update Swagger DocumentationNormalizer for AWS API-Gateway [WIP] Update Swagger DocumentationNormalizer for AWS API-Gateway Feb 12, 2018
@vincentchalamon vincentchalamon changed the title [WIP] Update Swagger DocumentationNormalizer for AWS API-Gateway Update Swagger DocumentationNormalizer for AWS API-Gateway Feb 13, 2018
@@ -42,7 +42,11 @@ public function __construct(ResourceNameCollectionFactoryInterface $resourceName
public function __invoke(Request $request = null): Documentation
{
if (null !== $request) {
$request->attributes->set('_api_normalization_context', $request->attributes->get('_api_normalization_context', []) + ['base_url' => $request->getBaseUrl()]);
$context = ['base_url' => $request->getBaseUrl()];
if ($request->query->has('api_gateway') && true === $request->query->getBoolean('api_gateway')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified in if ($request->query->getBoolean('api_gateway', false))

use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

/**
* Override Swagger documentation for API Gateway.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* Removes features unsupported by Amazon API Gateway.
*
* @see <the-relevant-amazon-docs>


use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you mark this class as @internal, so we'll be able to remove it when Amazon will improve its Docker support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@dunglas dunglas merged commit 274f4f5 into api-platform:2.1 Mar 6, 2018
@dunglas
Copy link
Member

dunglas commented Mar 6, 2018

Thanks @vincentchalamon

@vincentchalamon vincentchalamon deleted the 2.1 branch March 6, 2018 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants