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 definition in order to be compatible with string identifiers #1198

Merged
merged 1 commit into from
Sep 2, 2020

Conversation

phansys
Copy link
Member

@phansys phansys commented Jul 27, 2020

Subject

Update OpenAPI (Swagger) definition in order to be compatible with string identifiers (like UUIDs).

These changes are consistent with the API narrowing made at sonata-project/admin-bundle (see AdminInterface::id()).

I am targeting this branch, because these changes respect BC.

Changelog

### Removed
- Removed requirements that were only allowing integers for model identifiers at Open API definitions.
### Fixed
- Fixed support for string model identifiers at Open API definitions.

@phansys
Copy link
Member Author

phansys commented Jul 27, 2020

@wbloszyk, could you please confirm if the sandbox is working fine without these restrictions?

@phansys phansys requested a review from a team July 27, 2020 17:52
@wbloszyk
Copy link
Member

@wbloszyk, could you please confirm if the sandbox is working fine without these restrictions?

In @ApiDoc this restriction in only like information for people. It will be work but you should keep "dataType"="integer|UUIDs". Restriction will be when you add it in @Route and @ApiDoc will read this and display descriptions for people from it.

@phansys
Copy link
Member Author

phansys commented Jul 27, 2020

In @apidoc this restriction in only like information for people.

AFAIK, the @apidoc annotations are used to build the Open API Specification, which is consumed by the built-in sandbox to display the information which you are referencing and to create the "Try it out" forms. It also can be consumed by 3rd party compatible clients.

@phansys
Copy link
Member Author

phansys commented Jul 27, 2020

If you can, please check the resulting definition at Swagger Editor. You can also play with the changes you're proposing in order to check if the result works as expected.

It will be work but you should keep "dataType"="integer|UUIDs"

I wouldn't use "UUID" as type, since their type is really a string.

@wbloszyk
Copy link
Member

For swagger for path parameter you have to:

@SWG\Parameter(name="id", in="path", type="string")

Type must be "string", "number", "integer", "boolean", "array", "file".
"string" works becouse it is convert to url but probably it is not good idea.
"integer" will not allow use string.

@phansys
Copy link
Member Author

phansys commented Jul 27, 2020

"string" works becouse it is convert to url but probably it is not good idea.

Why do you think is not a good idea? IMO, we could use type="string" in conjunction with pattern attribute in order to keep narrowed the allowed values.

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@phansys phansys force-pushed the model_ids branch 3 times, most recently from 688182d to b2bfba6 Compare July 29, 2020 01:20
@phansys phansys marked this pull request as ready for review July 29, 2020 01:25
src/Controller/Api/GroupController.php Outdated Show resolved Hide resolved
src/Controller/Api/GroupController.php Show resolved Hide resolved
src/Controller/Api/GroupController.php Show resolved Hide resolved
@wbloszyk wbloszyk merged commit 7c869cc into sonata-project:4.x Sep 2, 2020
@wbloszyk
Copy link
Member

wbloszyk commented Sep 2, 2020

Thanks @phansys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants