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

CMS API: Drop support for XML requests #3255

Conversation

thalesmiguel
Copy link
Contributor

What this PR does / why we need it:

This PR removes support for XML format requests from all CMS endpoints. It also makes JSON the standard request format for the CMS API.

Which issue(s) this PR fixes

THREESCALE-9433

Verification steps

The following endpoints should only respond to JSON requests. Any other format should return a 406 status.

Verb Endpoint Controller#Action
GET /admin/api/cms/sections/:section_id/files admin/api/cms/files#index
GET /admin/api/cms/sections admin/api/cms/sections#index
POST /admin/api/cms/sections admin/api/cms/sections#create
GET /admin/api/cms/sections/new admin/api/cms/sections#new
GET /admin/api/cms/sections/:id/edit admin/api/cms/sections#edit
GET /admin/api/cms/sections/:id admin/api/cms/sections#show
PATCH /admin/api/cms/sections/:id admin/api/cms/sections#update
PUT /admin/api/cms/sections/:id admin/api/cms/sections#update
DELETE /admin/api/cms/sections/:id admin/api/cms/sections#destroy
GET /admin/api/cms/files admin/api/cms/files#index
POST /admin/api/cms/files admin/api/cms/files#create
GET /admin/api/cms/files/new admin/api/cms/files#new
GET /admin/api/cms/files/:id/edit admin/api/cms/files#edit
GET /admin/api/cms/files/:id admin/api/cms/files#show
PATCH /admin/api/cms/files/:id admin/api/cms/files#update
PUT /admin/api/cms/files/:id admin/api/cms/files#update
DELETE /admin/api/cms/files/:id admin/api/cms/files#destroy
PUT /admin/api/cms/templates/:id/publish admin/api/cms/templates#publish
GET /admin/api/cms/templates admin/api/cms/templates#index
POST /admin/api/cms/templates admin/api/cms/templates#create
GET /admin/api/cms/templates/:id admin/api/cms/templates#show
PATCH /admin/api/cms/templates/:id admin/api/cms/templates#update
PUT /admin/api/cms/templates/:id admin/api/cms/templates#update
DELETE /admin/api/cms/templates/:id admin/api/cms/templates#destroy

Special notes for your reviewer:

@thalesmiguel thalesmiguel self-assigned this Mar 16, 2023
Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

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

There are some endpoints in your list that don't exist AFAIK:

Verb Path Action
GET /admin/api/cms/sections/new admin/api/cms/sections#new
GET /admin/api/cms/sections/:id/edit admin/api/cms/sections#edit
GET /admin/api/cms/files/new admin/api/cms/files#new
GET /admin/api/cms/files/:id/edit admin/api/cms/files#edit

Are those routes valid and working? because there's no action in the controllers for them.

Also, I didn't even know this one existed:

Verb Path Action
GET /admin/api/cms/sections/:section_id/files admin/api/cms/files#index

I think we should remove it, if we provide sections/:section_id/files we should also provide sections/:section_id/sections and sections/:section_id/templates. And we have an issue (THREESCALE-9191) to filter results by any parameter, so the client will be able to retrieve the same results by filtering by section_id.

test/integration/cms/api/templates_test.rb Show resolved Hide resolved
test/integration/cms/api/templates_test.rb Outdated Show resolved Hide resolved
app/models/cms/builtin.rb Show resolved Hide resolved
@thalesmiguel
Copy link
Contributor Author

There are some endpoints in your list that don't exist AFAIK:

Yep... Those are "new" and "edit" endpoints that come from the "resources" Rails routes helper. Those don't really make sense for an API.

Also, I didn't even know this one existed:
I think we should remove it, if we provide sections/:section_id/files we should also provide sections/:section_id/sections and sections/:section_id/templates. And we have an issue (THREESCALE-9191) to filter results by any parameter, so the client will be able to retrieve the same results by filtering by section_id.

Agreed!

Since those changes go out of the scope of the PR, I opened #3266 to remove the endpoints you mentioned @jlledom ✌️

@thalesmiguel thalesmiguel changed the title CMS: Drop support for XML requests CMS API: Drop support for XML requests Mar 21, 2023
@thalesmiguel thalesmiguel merged commit cb07a5a into 3scale:master Mar 22, 2023
@thalesmiguel thalesmiguel deleted the THREESCALE-9433-remove-xml-support-from-cms-api branch March 22, 2023 11:43
mayorova added a commit that referenced this pull request Mar 27, 2023
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.

2 participants