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

Fix Open Api models to be equal with return data #1239

Closed
wants to merge 1 commit into from

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Sep 29, 2020

Subject

Current REST API models are not correct with Open Api Specifitaion (OAS). This PR will fix it for nelmio v3 (swagger v2). This change will be BC-break, but will be hide behind major nelmio upgrade.

Example:

  • people override User by App\Entity\SonataUserUser (add id and own extra fields)
  • add own jms_serializer dictionary for serialize it
  • someone will use OAS to use this API
  • model will be taken from UserInterface which will not provide fields from App\Entity\SonataUserUser

Solution

  • change models to nelmio model alias
  • add aliases to nelmio configuration (parameters like %sonata.user.admin.group.entity% can be used)
  • add upgrade note

I am targeting this branch, because this chane should be done in here. It is potential BC-break.

Changelog

### Fixed
- Fixed Open Api Models to be equal with return data

To do

  • change PagerInterface to correct model

@wbloszyk wbloszyk mentioned this pull request Sep 29, 2020
@wbloszyk wbloszyk requested review from phansys and a team September 29, 2020 15:22
@VincentLanglet
Copy link
Member

Is it related to/does it close #1238 ?

@wbloszyk
Copy link
Member Author

Is it related to/does it close #1238 ?

This PR will integrate REST API with OPEN API and is not related to this issue.

@phansys
Copy link
Member

phansys commented Sep 29, 2020

If we need to ensure a given schema, I suggest to add a simple test based on the generated JSON spec in order to ensure some endpoints and models to have the expected structure.

@wbloszyk
Copy link
Member Author

@phansys What do you think about remove Pager from models and use schema based on object type with returned properies + description for it? IMO we dont need model if we cant deserialize it. Also generate documentation which will return correct description will be hard.

Here you can check how it look like:

{"swagger":"2.0","info":{"title":"Sonata Sandbox","description":"This is demo RestFul API!","version":"1.0.0"},"paths":{"\/api\/user\/groups":{"get":{"summary":"Returns a paginated list of groups.","parameters":[{"name":"page","in":"query","allowEmptyValue":false,"required":false,"description":"Page for groups list pagination (1-indexed)","type":"string","default":"1"},{"name":"count","in":"query","allowEmptyValue":false,"required":false,"description":"Number of groups per page","type":"string","default":"10"},{"name":"orderBy","in":"query","required":false,"description":"Query groups order by clause (key is field, value is direction)","type":"string"},{"name":"enabled","in":"query","allowEmptyValue":true,"required":false,"description":"Enables or disables the groups only?","type":"string"},{"name":"orderBy[]","in":"query","allowEmptyValue":true,"required":false,"description":"Query groups order by clause (key is field, value is direction)","items":{"type":"string","pattern":"ASC|DESC"},"type":"array","collectionFormat":"multi"}],"responses":{"200":{"description":"Returned when successful","schema":{"properties":{"page":{"type":"integer"},"per_page":{"type":"integer"},"last_page":{"type":"integer"},"total":{"type":"integer"},"entries":{"items":{"$ref":"#\/definitions\/SonataUserUser"},"type":"array"}},"type":"object"}}},"tags":["\/api\/user\/groups"]},"post":{"summary":"Adds a group.","responses":{"200":{"description":"Returned when successful","schema":{"$ref":"#\/definitions\/SonataUserGroup"}},"400":{"description":"Returned when an error has occurred during the group creation"}},"tags":["\/api\/user\/groups"]}},"\/api\/user\/groups\/{id}":{"get":{"summary":"Retrieves a specific group.","parameters":[{"name":"id","in":"path","required":true,"type":"string"}],"responses":{"200":{"description":"Returned when successful","schema":{"$ref":"#\/definitions\/SonataUserGroup"}},"404":{"description":"Returned when group is not found"}},"tags":["\/api\/user\/groups"]},"put":{"summary":"Updates a group.","parameters":[{"name":"id","in":"path","required":true,"type":"string"}],"responses":{"200":{"description":"Returned when successful","schema":{"$ref":"#\/definitions\/SonataUserGroup"}},"400":{"description":"Returned when an error has occurred during the group creation"},"404":{"description":"Returned when unable to find group"}},"tags":["\/api\/user\/groups"]},"delete":{"summary":"Deletes a group.","parameters":[{"name":"id","in":"path","required":true,"type":"string"}],"responses":{"200":{"description":"Returned when group is successfully deleted"},"400":{"description":"Returned when an error has occurred during the group deletion"},"404":{"description":"Returned when unable to find group"}},"tags":["\/api\/user\/groups"]}},"\/api\/user\/users":{"get":{"summary":"Returns a paginated list of users.","parameters":[{"name":"page","in":"query","allowEmptyValue":false,"required":false,"description":"Page for users list pagination (1-indexed)","type":"string","default":"1"},{"name":"count","in":"query","allowEmptyValue":false,"required":false,"description":"Number of users per page","type":"string","default":"10"},{"name":"orderBy","in":"query","required":false,"description":"Query users order by clause (key is field, value is direction)","type":"string"},{"name":"enabled","in":"query","allowEmptyValue":true,"required":false,"description":"Enables or disables the users only?","type":"string"},{"name":"orderBy[]","in":"query","allowEmptyValue":true,"required":false,"description":"Query users order by clause (key is field, value is direction)","items":{"type":"string","pattern":"ASC|DESC"},"type":"array","collectionFormat":"multi"}],"responses":{"200":{"description":"Returned when successful","schema":{"properties":{"page":{"type":"integer"},"per_page":{"type":"integer"},"last_page":{"type":"integer"},"total":{"type":"integer"},"entries":{"items":{"$ref":"#\/definitions\/SonataUserUser"},"type":"array"}},"type":"object"}}},"tags":["\/api\/user\/users"]},"post":{"summary":"Updates a user.","responses":{"200":{"description":"Returned when successful","schema":{"$ref":"#\/definitions\/SonataUserUser"}},"400":{"description":"Returned when an error has occurred during the user creation"},"404":{"description":"Returned when unable to find user"}},"tags":["\/api\/user\/users"]}},"\/api\/user\/users\/{id}":{"get":{"summary":"Retrieves a specific user.","parameters":[{"name":"id","in":"path","required":true,"type":"string"}],"responses":{"200":{"description":"Returned when successful","schema":{"$ref":"#\/definitions\/SonataUserUser"}},"404":{"description":"Returned when user is not found"}},"tags":["\/api\/user\/users"]},"put":{"summary":"Updates a user.","parameters":[{"name":"id","in":"path","required":true,"type":"string"}],"responses":{"200":{"description":"Returned when successful","schema":{"$ref":"#\/definitions\/SonataUserUser"}},"400":{"description":"Returned when an error has occurred during the user creation"},"404":{"description":"Returned when unable to find user"}},"tags":["\/api\/user\/users"]},"delete":{"summary":"Deletes an user.","parameters":[{"name":"id","in":"path","required":true,"type":"string"}],"responses":{"200":{"description":"Returned when user is successfully deleted"},"400":{"description":"Returned when an error has occurred during the user deletion"},"404":{"description":"Returned when unable to find user"}},"tags":["\/api\/user\/users"]}},"\/api\/user\/users\/{userId}\/groups\/{groupId}":{"post":{"summary":"Attach a group to a user.","parameters":[{"name":"userId","in":"path","required":true,"type":"string"},{"name":"groupId","in":"path","required":true,"type":"string"}],"responses":{"200":{"description":"Returned when successful","schema":{"$ref":"#\/definitions\/SonataUserUser"}},"400":{"description":"Returned when an error has occurred while user\/group attachment"},"404":{"description":"Returned when unable to find user or group"}},"tags":["\/api\/user\/users"]},"delete":{"summary":"Detach a group to a user.","parameters":[{"name":"userId","in":"path","required":true,"type":"string"},{"name":"groupId","in":"path","required":true,"type":"string"}],"responses":{"200":{"description":"Returned when successful","schema":{"$ref":"#\/definitions\/SonataUserUser"}},"400":{"description":"Returned when an error occurred while detaching the user from the group"},"404":{"description":"Returned when unable to find user or group"}},"tags":["\/api\/user\/users"]}},"\/api\/doc.json":{"get":{"responses":{"default":{"description":""}}}}},"definitions":{"SonataUserUser":{"properties":{"username":{"type":"string"},"username_canonical":{"type":"string"},"email":{"type":"string"},"email_canonical":{"type":"string"},"enabled":{"type":"boolean"},"last_login":{"type":"string","format":"date-time"},"confirmation_token":{"title":"Random string sent to the user email address in order to verify it.","type":"string"},"password_requested_at":{"type":"string","format":"date-time"},"roles":{"additionalProperties":true,"type":"object"},"facebook_data":{"title":"{@inheritdoc}","additionalProperties":true,"type":"object"},"twitter_data":{"title":"{@inheritdoc}","additionalProperties":true,"type":"object"},"gplus_data":{"title":"{@inheritdoc}","additionalProperties":true,"type":"object"},"created_at":{"type":"string","format":"date-time"},"updated_at":{"type":"string","format":"date-time"},"two_step_verification_code":{"type":"string"},"date_of_birth":{"type":"string","format":"date-time"},"firstname":{"type":"string"},"lastname":{"type":"string"},"website":{"type":"string"},"biography":{"type":"string"},"gender":{"type":"string"},"locale":{"type":"string"},"timezone":{"type":"string"},"phone":{"type":"string"},"facebook_uid":{"type":"string"},"facebook_name":{"type":"string"},"twitter_uid":{"type":"string"},"twitter_name":{"type":"string"},"gplus_uid":{"type":"string"},"gplus_name":{"type":"string"},"token":{"type":"string"},"id":{"type":"integer"}},"type":"object"},"SonataUserGroup":{"properties":{"name":{"type":"string"},"id":{"type":"integer"}},"type":"object"}},"host":"127.0.0.1"}

@phansys
Copy link
Member

phansys commented Sep 30, 2020

I don't know what is the issue with the serialization for Sonata\DatagridBundle\Pager\PagerInterface. If the serializer does not know how to extract information from it, aren't we able to override the definition?

@wbloszyk
Copy link
Member Author

I don't know what is the issue with the serialization for Sonata\DatagridBundle\Pager\PagerInterface. If the serializer does not know how to extract information from it, aren't we able to override the definition?

Sonata\DatagridBundle\Pager\BasePager (becouse this class has serialize dictionary) has field results (serialized-name="entries") which type is array. If we want use it we should:

  • allow people to override BasePager like User or Group in this PR
  • override people Pagermodel to specify items from entiries
  • add query, otherwise create Pager model unhelpul,

We should not create model for Pager if we cant reproduce Pager functionality. Override model will create new one. It mean we will create n+1 pager models where n is number of sonata models.

That is why I think we should not create pager models and use object instead.

@jordisala1991
Copy link
Member

What is the status of this PR? Can we release user bundle without it?

@wbloszyk
Copy link
Member Author

OpenAPI modele are broken in old version (swagger V1). IMO we can add swagger V2 support with broken API modele becouse it will be BC (API will be work correct).

WDYT? @phansys

@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Apr 20, 2021
@github-actions github-actions bot closed this Apr 27, 2021
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.

4 participants