UI: XHR keys need to include the method as well #4319
Conversation
ui/app/adapters/job.js
Outdated
| @@ -49,9 +49,9 @@ export default Watchable.extend({ | |||
| xhrKey(url, method, options = {}) { | |||
| const namespace = options.data && options.data.namespace; | |||
| if (namespace) { | |||
| return `${url}?namespace=${namespace}`; | |||
| return `${method} ${url}?namespace=${namespace}`; | |||
There was a problem hiding this comment.
Dunno why but I'm always wary of using%20spaces in keys.
There was a problem hiding this comment.
URL encoding haunts all our dreams.
To be honest, I want to rip all this stuff out and replace it with some form of cancellation token like axios uses.
| @@ -51,8 +51,8 @@ export default ApplicationAdapter.extend({ | |||
| return ajaxOptions; | |||
| }, | |||
|
|
|||
| xhrKey(url /* method, options */) { | |||
| return url; | |||
| xhrKey(url, method /* options */) { | |||
There was a problem hiding this comment.
Nice that these were already being passed 👌
ui/app/adapters/job.js
Outdated
| @@ -49,9 +49,9 @@ export default Watchable.extend({ | |||
| xhrKey(url, method, options = {}) { | |||
| const namespace = options.data && options.data.namespace; | |||
| if (namespace) { | |||
| return `${url}?namespace=${namespace}`; | |||
| return `${method} ${url}?namespace=${namespace}`; | |||
There was a problem hiding this comment.
Would appending to this._super(...arguments) work here?
There was a problem hiding this comment.
Definitely would.
The URL alone doesn't guarantee uniqueness
DingoEatingFuzz
force-pushed
the
b-ui-errant-acl-error
branch
from
ee4aa99 to
e8bbd0f
Compare
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
The URL alone doesn't guarantee uniqueness. This caused a bug where stopping a job (
DELETE /v1/job/:id) and reading a job (GET /v1/job/:id) could abort one another.