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

Reset operations for each resource class and remove api prefix to mak… #613

Merged
merged 1 commit into from
Jul 14, 2016
Merged

Reset operations for each resource class and remove api prefix to mak… #613

merged 1 commit into from
Jul 14, 2016

Conversation

samvdb
Copy link
Contributor

@samvdb samvdb commented Jul 13, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

This PR fixes a problem where the generated swagger output was wrong because some resources got tagged incorrectly.

Example:

$operation['item'] and $operation['collection'] are not empty and the next resource has no collection operations defined in it's annotation. Only $operation['item'] will be overriden and thus the swagger documentation would contain output from the previous resource in its collection array.

When prefixing the annotation platform resources like this:

<import resource="." type="api_platform" prefix="/api"/>

Generated paths in swagger will look like /api/foo and the basepath will be /api for the hydra doc route.
Swagger javascript will construct the url to call like this: /api/api/foo which is invalid.

@samvdb
Copy link
Contributor Author

samvdb commented Jul 13, 2016

Not sure what changed in the swagger fixtures but my swagger file validates just fine with this code.

@dunglas
Copy link
Member

dunglas commented Jul 13, 2016

Thanks for testing and fixing this!

It's the Swagger file from the test app. You can generate it to debug by running: tests/Fixtures/app/console api:swagger:export > swagger.json

Ping @Simperfit


$itemOperationsDocs = [];
$definitions = [];
$basePath = $this->urlGenerator->generate('api_hydra_entrypoint');
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 you pass the ApiPlatform\Core\Api\UrlGeneratorInterface::REL_PATH); as second parameter and remove the addition at line 167 instead?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, it will not work.

@samvdb
Copy link
Contributor Author

samvdb commented Jul 14, 2016

@Simperfit how do i fix the swagger json in travis?

@dunglas
Copy link
Member

dunglas commented Jul 14, 2016

@samvdb The Swagger file generated after your patch isn't valid anymore.

  1. Install the Swagger validator: npm install -g swagger-cli
  2. Then generate the Swagger file of the test app by running:tests/Fixtures/app/console api:swagger:export > swagger.json
  3. Finally run swagger validate swagger.json to validate the generated file and debug your patch

@Simperfit
Copy link
Contributor

I'm currently looking at the swagger file generated after the patch you made

@samvdb
Copy link
Contributor Author

samvdb commented Jul 14, 2016

Yeah i see it now, doesn't seem to be a problem i introduced though :)

@Simperfit
Copy link
Contributor

@samvdb It is :
You change the generation of the path :
\/users/\{id} became "users\/{id}": which is wrong according to the specification.

@dunglas
Copy link
Member

dunglas commented Jul 14, 2016

I propose an easier fix: remove the basePath key totally. It is optional according to the Swagger spec, and as we generate URLs starting with a /, it will fix the problem.

@samvdb
Copy link
Contributor Author

samvdb commented Jul 14, 2016

Still doesnt validates my swagger file.

➜ DunglasApiBundle git:(patch-1-swagger) ✗ tests/Fixtures/app/console api:swagger:export > swagger.json
PHP Fatal error: A function with return type must return a value in Swagger/ApiDocumentationBuilder.php on line 343

(php 7 ofc)

@dunglas
Copy link
Member

dunglas commented Jul 14, 2016

@samvdb can you provide the full stack trace, it's probably another bug.

@dunglas
Copy link
Member

dunglas commented Jul 14, 2016

@samvdb, indeed, line 343 must return an empty array, not void like now. Can you change that too?

@samvdb
Copy link
Contributor Author

samvdb commented Jul 14, 2016

Already changed it, still waiting for tests to pass.

@@ -128,7 +125,7 @@ public function getApiDocumentation() : array
}

$range = $this->getRange($propertyMetadata);
if (null === $range) {
if (count($range) === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

empty($range) is more performant.

@dunglas
Copy link
Member

dunglas commented Jul 14, 2016

Can you rebase?

@samvdb
Copy link
Contributor Author

samvdb commented Jul 14, 2016

@dunglas all good now

@Simperfit
Copy link
Contributor

LGTM 👍

@dunglas dunglas merged commit d2b115e into api-platform:master Jul 14, 2016
@dunglas
Copy link
Member

dunglas commented Jul 14, 2016

Thank you very much @samvdb!

@samvdb samvdb deleted the patch-1-swagger branch July 14, 2016 10:39
magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
Reset operations for each resource class and remove api prefix to mak…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants