Skip to content

[change] api - Updated unsafe requests to use POST instead of GET#1861

Merged
liiight merged 14 commits intodevelopfrom
change_request_methods
Jul 3, 2017
Merged

[change] api - Updated unsafe requests to use POST instead of GET#1861
liiight merged 14 commits intodevelopfrom
change_request_methods

Conversation

@kristenmills
Copy link
Copy Markdown
Member

Motivation for changes:

So currently a bunch of unsafe requests are using GET instead of POST. Because all modern browsers support prefetching, it's not wise to use GET requests for things that do any sort of modification server side.

Detailed changes:

  • Update /logout, /server/reload, /server/shutdown, /database/vacuum, /database/cleanup, and /database/reset_plugin to use POST instead of GET
  • Update Web UI to account for these changed request methods.

…acuum, /database/cleanup, and /database/reset_plugin use POST instead of GET
@kristenmills kristenmills changed the title [change] api - Updated usafe requests to use POST instead of GET [change] api - Updated unsafe requests to use POST instead of GET Jun 2, 2017
@liiight
Copy link
Copy Markdown
Member

liiight commented Jun 3, 2017

This is a good idea. I wonder though if we should move the action to the payload of the request instead of being in the url itself. Something like {"operation": "vacuum"}. What do you think?
I know POST without body is valid but I feel like using a body fits better.

@kristenmills
Copy link
Copy Markdown
Member Author

I kind of like that too. We could combine the endpoints that way. Do you have a suggestion for the combined endpoint name? We could just do POST /server and /database? Maybe /server/action and /database/action?

@liiight
Copy link
Copy Markdown
Member

liiight commented Jun 3, 2017

I think i like /server and /database best

session.execute('VACUUM')
session.commit()
msg = 'DB VACUUM finished'
elif operation == 'list_plugins':
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think list_plugins should still be a GET request. It's idempotent and semantically that makes more sense.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

True, i'll change it

@kristenmills
Copy link
Copy Markdown
Member Author

Sorry I haven't gotten around to doing the other endpoint yet...I'll try to get to it soon.

@liiight
Copy link
Copy Markdown
Member

liiight commented Jun 18, 2017

You mean the server endpoint? I've been meaning to do that, been super busy...

@liiight
Copy link
Copy Markdown
Member

liiight commented Jun 20, 2017

I went with /server/manage

@kristenmills
Copy link
Copy Markdown
Member Author

Alright I think this is good to go now. 👍

@liiight
Copy link
Copy Markdown
Member

liiight commented Jun 30, 2017

Actually I think we missed one, /user/token better be post as well

@kristenmills
Copy link
Copy Markdown
Member Author

That one might more sense as a put but in general I agree.

@liiight liiight merged commit 1c9741a into develop Jul 3, 2017
@liiight
Copy link
Copy Markdown
Member

liiight commented Jul 3, 2017

Awesome, thank for this change!

@liiight liiight deleted the change_request_methods branch July 17, 2017 14:45
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.

2 participants