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

feat: swagger v2 #591

Merged
merged 3 commits into from Jul 7, 2016
Merged

feat: swagger v2 #591

merged 3 commits into from Jul 7, 2016

Conversation

@Simperfit
Copy link
Member

@Simperfit Simperfit commented Jul 3, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? yes if we remove nelmio
Deprecations? no
Tests pass? yes
Fixed tickets #538
License MIT
Doc PR todo

Since NelmioApiDocBundle is not going to update to swagger v2, we will do it in api-platform.

This is WIP :

  • Add tests (unit / behat) [ see https://travis-ci.org/api-platform/core/jobs/142226685#L4610]
  • Refactor, delete maximum of dupplication
  • look at scrutinizer

I have been doing that today, tested with SwaggerUI by changing the format of hydra / swagger

*
* @param XmlFileLoader $loader
*/
private function enableJsonLd(XmlFileLoader $loader)
{
$loader->load('jsonld.xml');
$loader->load('hydra.xml');
$loader->load('swagger.xml');

This comment has been minimized.

@@ -21,7 +21,7 @@
*
* @author Kévin Dunglas <dunglas@gmail.com>
*/
final class ValidationExceptionListener
final class HydraValidationExceptionListener

This comment has been minimized.

@dunglas

dunglas Jul 3, 2016
Member

Why this change?

@@ -22,7 +22,7 @@
*/
final class ConstraintViolationListNormalizer implements NormalizerInterface
{
const FORMAT = 'hydra-error';
const FORMAT = ['swagger-error', 'hydra-error'];

This comment has been minimized.

@dunglas

dunglas Jul 3, 2016
Member

IMO Hydra and Swagger support should be 100% independent and the resulting documents shouldn't mix both formats.

if ('' !== $this->description) {
$doc['info']['description'] = $this->description;
}
$doc['host'] = $_SERVER['HTTP_HOST'];

This comment has been minimized.

@dunglas

dunglas Jul 3, 2016
Member

Super-globals should never be accessed directly. This data would better be injected in the constructor.

}

switch ($type->getBuiltinType()) {
case 'string':

This comment has been minimized.

@dunglas

dunglas Jul 3, 2016
Member

Constants from the Type class should be used.

$className = $type->getClassName();

if ($className) {
if ('DateTime' === $className) {

This comment has been minimized.

@dunglas

dunglas Jul 3, 2016
Member

if (is_subclass_of($className, \DateTimeInterface::class)) {

$className = $type->getClassName();
if ($this->resourceClassResolver->isResourceClass($className)) {
return ['$ref' => sprintf('#/definitions/%s', $this->resourceMetadataFactory->create($className)->getShortName())];
}

This comment has been minimized.

@dunglas

dunglas Jul 3, 2016
Member

Is it ok to ignore classes that are not dates nor resources?

This comment has been minimized.

@Simperfit

Simperfit Jul 4, 2016
Author Member

I guess yes because they are not definitions

*/
public function onKernelResponse(FilterResponseEvent $event)
{
if ('application/swagger' === $event->getResponse()->headers->get('Content-Type')) {

This comment has been minimized.

@dunglas

dunglas Jul 3, 2016
Member

Can't find anything about that mime type in the spec. Where do it come from?

This comment has been minimized.

@Simperfit

Simperfit Jul 4, 2016
Author Member

It was just for testing purpose

@dunglas
Copy link
Member

@dunglas dunglas commented Jul 3, 2016

First of all, thanks for working on this @Simperfit. It's very much appreciated.

Unless JSON-LD, HAL or JSON API, Swagger is just an API documentation format. Not all resources exposed by the API will be available in the "Swagger". There will be only one new URL returning the Swagger doc.

Basically, you can keep your Swagger\ApiDocumentationBuilder and add a new Swagger action (mapped to a /swagger path for instance) and you're all done. When someone will want to use Swagger UI, it will query this URL. Then Swagger UI will send JSON-LD requests to the API.

@Simperfit
Copy link
Member Author

@Simperfit Simperfit commented Jul 4, 2016

@dunglas Comments taken in account. I've added a console command that dumps the documentation

@Simperfit Simperfit force-pushed the Simperfit:feature/swagger-v2 branch from b665c20 to 48997eb Jul 4, 2016
*
* @author Amrouche Hamza <hamza.simperfit@gmail.com>
*/
class SwaggerCommand extends Command

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Can you make this class final?

class SwaggerCommand extends Command
{
/**
* @var ApiDocumentationBuilder

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Useless dockblock, should be removed.

parent::__construct();
}

protected function configure()

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

/**
 * {@inheritdoc}
 */

missing

{
$this
->setName('api:swagger:export')
->setDescription('Export a beautiful json of swagger api');

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Dump the Swagger 2.0 (OpenAPI) documentation?

->setDescription('Export a beautiful json of swagger api');
}

protected function execute(InputInterface $input, OutputInterface $output)

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

/**
 * {@inheritdoc}
 */

missing

@@ -52,7 +52,7 @@
<tag name="serializer.normalizer" priority="50" />
</service>

<service id="api_platform.hydra.normalizer.constraint_violation_list" class="ApiPlatform\Core\Bridge\Symfony\Validator\Hydra\Serializer\ConstraintViolationListNormalizer" public="false">
<service id="api_platform.hydra.normalizer.constraint_violation_list" class="ApiPlatform\Core\Bridge\Symfony\Validator\Serializer\ConstraintViolationListNormalizer" public="false">

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Should be reverted.

xsi:schemaLocation="http://symfony.com/schema/routing
http://symfony.com/schema/routing/routing-1.0.xsd">

<route id="api_swagger_vocab" path="/swagger.json">

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

<route id="api_swagger_doc" path="/swagger">?

@@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace ApiPlatform\Core\Bridge\Symfony\Validator\Hydra\Serializer;
namespace ApiPlatform\Core\Bridge\Symfony\Validator\Serializer;

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

All changes in this file should be reverted.

* file that was distributed with this source code.
*/

namespace ApiPlatform\Core\Bridge\Symfony\Validator\Swagger\EventListener;

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Changes in this file must be reverted too.

@@ -11,7 +11,7 @@

namespace ApiPlatform\Core\Hydra\Action;

use ApiPlatform\Core\Hydra\ApiDocumentationBuilderInterface;
use ApiPlatform\Core\Util\ApiDocumentationBuilderInterface;

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Useless, should be reverted.

@@ -21,6 +21,8 @@
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceNameCollectionFactoryInterface;
use ApiPlatform\Core\Metadata\Resource\ResourceMetadata;
use ApiPlatform\Core\Util\ApiDocumentationBuilderInterface;

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Useless, should be reverted.

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Or an implements statement is missing.

@@ -97,8 +97,10 @@ public function normalize($object, $format = null, array $context = [])
return $rawData;
}

$data['@id'] = $this->iriConverter->getIriFromItem($object);
$data['@type'] = ($iri = $resourceMetadata->getIri()) ? $iri : $resourceMetadata->getShortName();
if ($format === self::FORMAT) {

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Should be reverted.

*/
public function __invoke(Request $request)
{
$request->attributes->set('_api_format', 'jsonld');

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Will change when #590 will be merged.

*
* @author Kévin Dunglas <dunglas@gmail.com>
*/
final class ApiDocumentationBuilder implements ApiDocumentationBuilderInterface

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Does it really worth it to introduce a common API for the Hydra and the Swagger doc?

@@ -9,17 +9,17 @@
* file that was distributed with this source code.
*/

namespace ApiPlatform\Core\Hydra;
namespace ApiPlatform\Core\Util;

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

OK why not keeping this command interface, but:

  • Should be move to a Documentation namespace
  • Can be used to have a common controller for both docs (and an optional ?format=swagger|hydra for instance)
@dunglas
Copy link
Member

@dunglas dunglas commented Jul 4, 2016

When you'll write tests, you can use https://hub.docker.com/r/swaggerapi/swagger-validator/ to validate the generated Swagger file of the test app in Travis.

{
/** @var InitializedContextEnvironment $environment */
$environment = $scope->getEnvironment();
$this->restContext = $environment->getContext('Sanpi\Behatch\Context\RestContext');

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

Sanpi\Behatch\Context\RestContext::class?

*
* @return array
*/
private function getOperation($method, $className)

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

as we are in php7 we can force the typehint everywhere and remove useless phpdoc

*/
final class SwaggerCommand extends Command
{
protected $apiDocumentationBuilder;

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

private, same for the rest

This comment has been minimized.

@Simperfit

Simperfit Jul 4, 2016
Author Member

I cannot make the function private since its a command

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Why? It's a Symfony limitation?

This comment has been minimized.

@Simperfit

Simperfit Jul 4, 2016
Author Member

Yes it is

@@ -22,7 +22,7 @@
*/
final class ConstraintViolationListNormalizer implements NormalizerInterface
{
const FORMAT = 'hydra-error';
const FORMAT = ['hydra-error'];

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Should be reverted.

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Same for all changes in this file.

@@ -100,6 +100,7 @@ public function normalize($object, $format = null, array $context = [])
$data['@id'] = $this->iriConverter->getIriFromItem($object);
$data['@type'] = ($iri = $resourceMetadata->getIri()) ? $iri : $resourceMetadata->getShortName();


This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Should be reverted.

/**
* Creates a machine readable Swagger API documentation.
*
* @author Kévin Dunglas <dunglas@gmail.com>

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

You can credit yourself here :)

$required = array_merge($required, [$propertyName]);
}

if (0 !== count($required)) {

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

!empty (micro optimization)

try {
$resourceClassIri = $this->iriConverter->getIriFromResourceClass($resourceClass);
} catch (InvalidArgumentException $e) {
$resourceClassIri = '/nopaths';

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

What is the reason of this?

This comment has been minimized.

@Simperfit

Simperfit Jul 4, 2016
Author Member

Because in the tests NoCollectionDummy does throw an error in here.

$classes[] = $class;
}

$doc['swagger'] = '2.0';

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

What about using a class constant?

This comment has been minimized.

@Simperfit

Simperfit Jul 4, 2016
Author Member

I was thinking of a parameters in the service too ?

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

IMO a constant is better because the version cannot be changed without patching the code.

$doc['host'] = $this->host;
$doc['basePath'] = $this->urlGenerator->generate('api_jsonld_entrypoint');
$doc['definitions'] = $properties;
$doc['externalDocs'] = ['description' => 'Find more about api-platform', 'url' => 'docs'];

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

$doc['externalDocs'] = ['description' => 'Find more about API Platform', 'url' => 'https://api-platform.com'];?

$shortName = $resourceMetadata->getShortName();
$swaggerOperation[$methodSwagger] = [];
$swaggerOperation[$methodSwagger]['tags'] = [$shortName];
$swaggerOperation[$methodSwagger]['produces'] = ['application/json'];

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

application/ld+json?

$swaggerOperation[$methodSwagger]['responses'] = [
'200' => ['description' => 'Valid ID'],
];

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Extra line to remove.

];

break;
case 'DELETE':

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

Add a blank line before.

@@ -22,10 +22,13 @@ before_install:
- if [ "$TRAVIS_PHP_VERSION" != "hhvm" ]; then phpenv config-rm xdebug.ini; fi;
- phpunit --self-update
- if [ "$SYMFONY_VERSION" != "" ]; then composer require "symfony/symfony:${SYMFONY_VERSION}" --no-update; fi;
- npm install -g swagger-cli

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

outch, IMO it would be better to have it in a packages.json and shrinkwrap it. node_modules could then be cached like it is for composer

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

IMO it's better as is. Having a packages.json in the repository (that doesn't use any NPM dependency) has no sense and it's good to test the Swagger support with the latest available version of swagger-cli (we also install the last version of composer for instance).

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

Having a packages.json in the repository (that doesn't use any NPM dependency) has no sense

Well, with this we are. If we don't add it it's a hidden project dependency. Not required unless you are developing on this bit, but hidden still. Having a packages.json does not force you to do a npm install when you want to get started with the project, but at least it avoids people to have to look at Travis to figure out that they need swagger-cli.

it's good to test the Swagger support with the latest available version of swagger-cli

In that case yeah we can drop the npm-shrinkwrap.json thing, which makes it even easier as unlike composer, shrinkwrap depends on the local node version installed

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

They don't need it to develop. It's just an extra check we do in the CI to be sure that the output of the Swagger generator is correct.
If we used a go validator (goswagger is able to validate Swagger files), we would not have versioned a go setup in the repo just to use this program.

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

fair enough

sprintf('The property "%s" for the class "%s" exist.', $propertyName, $className)
);
} catch (\Exception $exception) {
// an exception must be catched

This comment has been minimized.

This comment has been minimized.

@dunglas

dunglas Jul 4, 2016
Member

I guess it's to respect PSR-2 that forbid empty catch without comment. Maybe a better message can be found :-)

}

if (empty($propertyInfos)) {
throw new \Exception(sprintf('Property "%s" of class "%s" does\'nt exist', $propertyName, $className));

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

does not

@dunglas
Copy link
Member

@dunglas dunglas commented Jul 4, 2016

I left some minor comments but it looks good to me. What do you think @api-platform/core-team I want it in the first beta too!

$propertyInfos = null;
foreach ($properties as $classPropertyName => $property) {
if ($classPropertyName === $propertyName) {
$propertyInfos = $property;

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

do we have to iterate over all the properties? Can't we return as soon as we got one?

*
* @throws Exception
*
* @return array | stdClass

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

IMO everything except the Gets an operation by its method name. can be removed. As the context is not meant to be reused, indicating that an exception is thrown is useless

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

you also have a return typehint mismatch

This comment has been minimized.

@Simperfit

Simperfit Jul 5, 2016
Author Member

done

{
$classInfos = $this->getClassInfos($className);

return isset($classInfos) ? $classInfos : [];

This comment has been minimized.

@theofidry

theofidry Jul 4, 2016
Member

don't we have something better than isset here? $classInfo is an array right? so we can use empty instead

@dunglas
Copy link
Member

@dunglas dunglas commented Jul 4, 2016

Can you also add a unit test for the Swagger generator?

@Simperfit
Copy link
Member Author

@Simperfit Simperfit commented Jul 5, 2016

@dunglas I asserted the Json, what do I test ? Like in behat ?

@dunglas
Copy link
Member

@dunglas dunglas commented Jul 6, 2016

@Simperfit a PHPUnit test on the class itself would be the best. Can you resolve conflicts too?

Simperfit added 3 commits Jul 3, 2016
@Simperfit Simperfit force-pushed the Simperfit:feature/swagger-v2 branch from 0febd58 to 420deeb Jul 6, 2016
@Simperfit
Copy link
Member Author

@Simperfit Simperfit commented Jul 6, 2016

@dunglas I've added one on the console,
Do I have to add one on the ApiDocumentationBuilder ?
I have rebased

@dunglas
Copy link
Member

@dunglas dunglas commented Jul 6, 2016

On the builder. We try to have unit tests for every new classes.

@dunglas dunglas merged commit dc227ec into api-platform:master Jul 7, 2016
4 checks passed
4 checks passed
Scrutinizer 18 new issues, 14 updated code elements
Details
StyleCI The StyleCI analysis has passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dunglas
Copy link
Member

@dunglas dunglas commented Jul 7, 2016

Thank you @Simperfit

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.