Plugins using elgg/Ajax can't tell if action is successful #9767

Closed
hypeJunction opened this Issue Apr 23, 2016 · 16 comments

Comments

Projects
None yet
2 participants
@hypeJunction
Contributor

hypeJunction commented Apr 23, 2016

Old API used to sniff for errors and set status to -1 if it found registered errors. Current elgg/Ajax implementation strips error messages from the response and expects actions to implement their own status logic hence there is no reliable way of telling if the action has succeeded.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 23, 2016

Member

I removed this on the basis that presence of an error message isn't a strong indicator of failure, but I concede it's better than nothing. My hope was to move away from this heuristic by returning more useful values if an elgg/Ajax request was received.

Member

mrclay commented Apr 23, 2016

I removed this on the basis that presence of an error message isn't a strong indicator of failure, but I concede it's better than nothing. My hope was to move away from this heuristic by returning more useful values if an elgg/Ajax request was received.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 23, 2016

Member

How do you suggest plugins should detect an error? Maybe returning only the value was an API mistake...

Member

mrclay commented Apr 23, 2016

How do you suggest plugins should detect an error? Maybe returning only the value was an API mistake...

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Apr 23, 2016

Contributor

Ideally we would make proper use of http codes. Value indeed seems like a problem, bacause hooks can't attach any more values to it if it's an html string. Let me sleep on it.

Contributor

hypeJunction commented Apr 23, 2016

Ideally we would make proper use of http codes. Value indeed seems like a problem, bacause hooks can't attach any more values to it if it's an html string. Let me sleep on it.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 23, 2016

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Apr 24, 2016

Contributor

I removed this on the basis that presence of an error message isn't a strong indicator of failure, but I concede it's better than nothing. My hope was to move away from this heuristic by returning more useful values if an elgg/Ajax request was received.

I agree with the notion. I don't have any better ideas at the moment though.

Contributor

hypeJunction commented Apr 24, 2016

I removed this on the basis that presence of an error message isn't a strong indicator of failure, but I concede it's better than nothing. My hope was to move away from this heuristic by returning more useful values if an elgg/Ajax request was received.

I agree with the notion. I don't have any better ideas at the moment though.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 24, 2016

Member

I'm not convinced the added complexity of sending correct 4xx HTTP codes is worth the complexity; now that plugins can piggyback on responses (and requests), what code should be sent if the response hook sets an error, but the requested action succeeds, or vise versa?

Strawman 1: elgg/Ajax3 module works a bit differently: it returns a wrapper object with value and error properties, and sends requests with X-Elgg-Ajax-API: 3. This allows us to recognize these requests and handle them however we need to.

Strawman 2: The ajax user passes in a "meta" object with the request. The API populates meta.error before the response is received:

var meta = {};
ajax.action('foo', options, meta).done(function (value) {
    if (meta.error) { ... }
});

Strawman 3: if any errors were sent back, the error/reject handler/promise method is called. If one is not set, we just show the error (we don't also show default ajax error message).

Member

mrclay commented Apr 24, 2016

I'm not convinced the added complexity of sending correct 4xx HTTP codes is worth the complexity; now that plugins can piggyback on responses (and requests), what code should be sent if the response hook sets an error, but the requested action succeeds, or vise versa?

Strawman 1: elgg/Ajax3 module works a bit differently: it returns a wrapper object with value and error properties, and sends requests with X-Elgg-Ajax-API: 3. This allows us to recognize these requests and handle them however we need to.

Strawman 2: The ajax user passes in a "meta" object with the request. The API populates meta.error before the response is received:

var meta = {};
ajax.action('foo', options, meta).done(function (value) {
    if (meta.error) { ... }
});

Strawman 3: if any errors were sent back, the error/reject handler/promise method is called. If one is not set, we just show the error (we don't also show default ajax error message).

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Apr 24, 2016

Contributor

I think proper handling 4xx is worth the effort long term. Once we have a unified approach to sending responses, we should be able to read Accept headers and send proper data back, without all the magic in forward hooks and varying reaponses based on context (ajax or not). That will also be necessary if we ever dream of having RESTful API by default.
Hooks should be able to set their own status code.
I like strawman 3, but provided we can unwrap response data and let plugins use it if they want.

Contributor

hypeJunction commented Apr 24, 2016

I think proper handling 4xx is worth the effort long term. Once we have a unified approach to sending responses, we should be able to read Accept headers and send proper data back, without all the magic in forward hooks and varying reaponses based on context (ajax or not). That will also be necessary if we ever dream of having RESTful API by default.
Hooks should be able to set their own status code.
I like strawman 3, but provided we can unwrap response data and let plugins use it if they want.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 24, 2016

Member

Look at the "statusCode" setting. A general problem we have is we have 1 default error handler, and if end users want to specify it, they have to handle the whole variety of outcomes like 404, 500, timeouts, etc. Maybe our API should require specifying behavior for specific codes (plus "timeout"); we can then provide defaults for each one.

What is REST going to give us? I think generally our system (maybe similar in spirit to JSON-RPC) is fine, mostly suffering from legacy action code.

We could easily add response codes in a BC way by sniffing for the X-Elgg-Ajax-API header. I.e. We could add a $response_code argument to register_error(), or just have a separate elgg_set_ajax_response_code(). Both would not do anything unless the X-Elgg-Ajax-API header indicated a request to support it. Or maybe we just add new ajax-only actions.

Member

mrclay commented Apr 24, 2016

Look at the "statusCode" setting. A general problem we have is we have 1 default error handler, and if end users want to specify it, they have to handle the whole variety of outcomes like 404, 500, timeouts, etc. Maybe our API should require specifying behavior for specific codes (plus "timeout"); we can then provide defaults for each one.

What is REST going to give us? I think generally our system (maybe similar in spirit to JSON-RPC) is fine, mostly suffering from legacy action code.

We could easily add response codes in a BC way by sniffing for the X-Elgg-Ajax-API header. I.e. We could add a $response_code argument to register_error(), or just have a separate elgg_set_ajax_response_code(). Both would not do anything unless the X-Elgg-Ajax-API header indicated a request to support it. Or maybe we just add new ajax-only actions.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Apr 25, 2016

Contributor

We could introduce statusCode handling in elgg/Ajax and let plugins provide custom handlers if they need (we would just extend the default statusCode object with whatever is provided with options).

REST gives us a different way of thinking. We are stuck in thinking about responses in the Elgg way and we fail to see how we could recycle handlers for various purposes without duplicating code. For example, our handlers are dumb to request methods, you can request the same page with GET, POST and DELETE and all you get is a rendered page in return.

I would really like us to eliminate the need for using elgg_is_xhr(). The handlers should return data regardless of the request type - this would allow us to filter data with hooks, i.e. an action should always return data that can be piggybacked, and we should then prepare the response behind the scenes. Currently if you just echo something from an action, you end up with a security exception on non-ajax calls.

Imagine we drop forward() in favor of some more transparent API.

function elgg_send_response($forward_url, $data, $status_code = 200, array $headers = array()) {
   // trigger a hook to filter response params
   if (elgg_is_xhr()) {
      $response = _elgg_prepare_json_response($data, $status_code, $headers);
   } else if ($forward_url) {
      if ($status_code === 200) {
         $response = _elgg_prepare_redirect_response($forward_url, $status_code, $headers);
      } else {
          $response = _egg_prepare_response(elgg_view_resource('error'), $status_code, $headers);
      }
   } else {
      // check Accept headers and send html/json response
      $response = _elgg_prepare_response($data, $status_code, $headers);
   }
   return $response->send();
}

// handle a GET request (page handler)
if ($error) {
   elgg_send_response('all', '', 400); 
} else {
   elgg_send_response('', elgg_view_resource('my/resource')); 
}

// handle a POST request (action)
$data = ['guid' => $guid];
if ($error) {
   register_error('Bad request');
   elgg_send_response(REFERRER, $data, 400); 
} else {
   system_message('All good');
   elgg_send_response('success/url', $data);
}
Contributor

hypeJunction commented Apr 25, 2016

We could introduce statusCode handling in elgg/Ajax and let plugins provide custom handlers if they need (we would just extend the default statusCode object with whatever is provided with options).

REST gives us a different way of thinking. We are stuck in thinking about responses in the Elgg way and we fail to see how we could recycle handlers for various purposes without duplicating code. For example, our handlers are dumb to request methods, you can request the same page with GET, POST and DELETE and all you get is a rendered page in return.

I would really like us to eliminate the need for using elgg_is_xhr(). The handlers should return data regardless of the request type - this would allow us to filter data with hooks, i.e. an action should always return data that can be piggybacked, and we should then prepare the response behind the scenes. Currently if you just echo something from an action, you end up with a security exception on non-ajax calls.

Imagine we drop forward() in favor of some more transparent API.

function elgg_send_response($forward_url, $data, $status_code = 200, array $headers = array()) {
   // trigger a hook to filter response params
   if (elgg_is_xhr()) {
      $response = _elgg_prepare_json_response($data, $status_code, $headers);
   } else if ($forward_url) {
      if ($status_code === 200) {
         $response = _elgg_prepare_redirect_response($forward_url, $status_code, $headers);
      } else {
          $response = _egg_prepare_response(elgg_view_resource('error'), $status_code, $headers);
      }
   } else {
      // check Accept headers and send html/json response
      $response = _elgg_prepare_response($data, $status_code, $headers);
   }
   return $response->send();
}

// handle a GET request (page handler)
if ($error) {
   elgg_send_response('all', '', 400); 
} else {
   elgg_send_response('', elgg_view_resource('my/resource')); 
}

// handle a POST request (action)
$data = ['guid' => $guid];
if ($error) {
   register_error('Bad request');
   elgg_send_response(REFERRER, $data, 400); 
} else {
   system_message('All good');
   elgg_send_response('success/url', $data);
}
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 25, 2016

Member

In effect, the likes/add action would always (by some means) return its data {text:, selector:, num_likes:} and the forward location, and Elgg would decide how to prepare the appropriate response, either 307 redirect or 200 JSON. SGTM so far.

Allowing return arrays (like https://github.com/Elgg/Elgg/pull/9768/files#diff-1073861892e18f0e7c19af6f7e7cf62eR178) seems fine, but another simpler way to upgrade our many existing actions: Add a function to set the response data.:

// discarded if non-ajax request
elgg_set_response_data([
  'foo' => 'bar'
]);
forward(REFERER);

...or a failure:

register_error(...);
forward(REFERER, 422);

REST seems like a separate issue I'm not sold on. Testability of actions and less branching inside them are both big wins.

Member

mrclay commented Apr 25, 2016

In effect, the likes/add action would always (by some means) return its data {text:, selector:, num_likes:} and the forward location, and Elgg would decide how to prepare the appropriate response, either 307 redirect or 200 JSON. SGTM so far.

Allowing return arrays (like https://github.com/Elgg/Elgg/pull/9768/files#diff-1073861892e18f0e7c19af6f7e7cf62eR178) seems fine, but another simpler way to upgrade our many existing actions: Add a function to set the response data.:

// discarded if non-ajax request
elgg_set_response_data([
  'foo' => 'bar'
]);
forward(REFERER);

...or a failure:

register_error(...);
forward(REFERER, 422);

REST seems like a separate issue I'm not sold on. Testability of actions and less branching inside them are both big wins.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 25, 2016

Member

I guess forward(REFERER, 422) won't work due to BC, but we could allow adding status code to register_error().

In any case we should have constants like ELGG_HTTP_BAD_REQUEST and ELGG_HTTP_UNPROCESSABLE_ENTITY.

Member

mrclay commented Apr 25, 2016

I guess forward(REFERER, 422) won't work due to BC, but we could allow adding status code to register_error().

In any case we should have constants like ELGG_HTTP_BAD_REQUEST and ELGG_HTTP_UNPROCESSABLE_ENTITY.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Apr 25, 2016

Contributor

Another issue to consider: #8000
Perhaps instead of registering an error with an http code, we could throw ActionException or similar with the error message and http code.

I am fine with adding elgg_set_response_data() and elgg_set_response_code(). That would mitigate some of the issues.

Contributor

hypeJunction commented Apr 25, 2016

Another issue to consider: #8000
Perhaps instead of registering an error with an http code, we could throw ActionException or similar with the error message and http code.

I am fine with adding elgg_set_response_data() and elgg_set_response_code(). That would mitigate some of the issues.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Apr 25, 2016

Contributor

So, what's out plan. We either add new API to set data or we allow array returns. I don't think pursuing both will lead to anything good.

Contributor

hypeJunction commented Apr 25, 2016

So, what's out plan. We either add new API to set data or we allow array returns. I don't think pursuing both will lead to anything good.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 25, 2016

Member

...working up a ticket now.

Member

mrclay commented Apr 25, 2016

...working up a ticket now.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 26, 2016

Member

I think the error-handling weakness in elgg/Ajax is my primary concern and we should just replace it in 2.2 with elgg/Ajax3, ideally with minimal changes needed to actions.

  • Ajax3 sends a custom header, X-Elgg-Ajax-API: 3
  • Ajax3 allows callers to set handlers for specific error response codes.
  • Do non-200 responses fire client hook? Simpler if not.
  • Ajax3 normalizes all promise and callbacks so they receive a single object with keys: value, xhr (the jqXHR), textStatus (from $.ajax), errorThrown (null or the HTTP status text if error).
  • Any key set by a server/client hook that doesn't start with an underscore is copied to the object as well. ("private" hooks should use keys with leading underscores).
  • We add elgg_set_response_status() to set status code, and in 2.x this only affects Ajax3 responses. this allows us to update core actions in 2.x.
  • For Ajax3 requests, a server register_error() call sets status code to 500 (if not already set).
Member

mrclay commented Apr 26, 2016

I think the error-handling weakness in elgg/Ajax is my primary concern and we should just replace it in 2.2 with elgg/Ajax3, ideally with minimal changes needed to actions.

  • Ajax3 sends a custom header, X-Elgg-Ajax-API: 3
  • Ajax3 allows callers to set handlers for specific error response codes.
  • Do non-200 responses fire client hook? Simpler if not.
  • Ajax3 normalizes all promise and callbacks so they receive a single object with keys: value, xhr (the jqXHR), textStatus (from $.ajax), errorThrown (null or the HTTP status text if error).
  • Any key set by a server/client hook that doesn't start with an underscore is copied to the object as well. ("private" hooks should use keys with leading underscores).
  • We add elgg_set_response_status() to set status code, and in 2.x this only affects Ajax3 responses. this allows us to update core actions in 2.x.
  • For Ajax3 requests, a server register_error() call sets status code to 500 (if not already set).
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 26, 2016

Member

Actually if we used $.ajax's dataFilter option, we could perform the hook there and eliminate the need to filter all these callbacks and promise methods; we wouldn't need custom objects, just the standard $.ajax argument sets.

Member

mrclay commented Apr 26, 2016

Actually if we used $.ajax's dataFilter option, we could perform the hook there and eliminate the need to filter all these callbacks and promise methods; we wouldn't need custom objects, just the standard $.ajax argument sets.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 26, 2016

Member

I don't like the idea of setting a status code when an error is registered.

This would be just for Ajax3 requests. Code like this is very common in actions and pages:

if (!$expected) {
    register_error('Something sad happened');
    forward(REFERER);
}

If our goal is to have Ajax3 use HTTP code-based error handling, then 1 of 3 things has to happen:

  1. core has to add status codes most everywhere in 2.x
  2. dev has to fork the action/resource view to add an explicit status code
  3. (worst IMO) Ajax3 and the dev has to look out for HTTP 200 errors, too.

More generally I'm still trying to decide whether data is returned for errors. jQuery (for whatever reason) seems to say "no" since it only gives you the jqXHR and forces you to manually decode the response if you want it. I think this is probably the way to go:

  • Do send HTTP error code
  • Don't trigger server response hook
  • Only return JSON wrapper for display messages (no "response data")
  • Don't call client-side response hook
  • If no server error message, and no client-set error handler, display the generic ajax message

I think this would simplify error handling server and client-side.

Member

mrclay commented Apr 26, 2016

I don't like the idea of setting a status code when an error is registered.

This would be just for Ajax3 requests. Code like this is very common in actions and pages:

if (!$expected) {
    register_error('Something sad happened');
    forward(REFERER);
}

If our goal is to have Ajax3 use HTTP code-based error handling, then 1 of 3 things has to happen:

  1. core has to add status codes most everywhere in 2.x
  2. dev has to fork the action/resource view to add an explicit status code
  3. (worst IMO) Ajax3 and the dev has to look out for HTTP 200 errors, too.

More generally I'm still trying to decide whether data is returned for errors. jQuery (for whatever reason) seems to say "no" since it only gives you the jqXHR and forces you to manually decode the response if you want it. I think this is probably the way to go:

  • Do send HTTP error code
  • Don't trigger server response hook
  • Only return JSON wrapper for display messages (no "response data")
  • Don't call client-side response hook
  • If no server error message, and no client-set error handler, display the generic ajax message

I think this would simplify error handling server and client-side.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 26, 2016

feature(js): adds improved Ajax3 module
Use of `register_error` automatically causes error client side.
Can set HTTP error code via `register_error` or `elgg_set_response_code`.
Hooks can pass metadata to caller via object.
Uses standard jQuery arguments for all callbacks/promise handlers.

Fixes #9767

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 26, 2016

feature(js): adds improved elgg/Ajax3 module
Use of `register_error` automatically causes error client side (Ajax3 responses only).
Can set HTTP error code (Ajax3 responses only) via `register_error` or `elgg_set_response_code`.
Hooks can pass metadata to caller via object.
Uses standard jQuery arguments for all callbacks/promise handlers.

Fixes #9767

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 26, 2016

feature(js): adds improved elgg/Ajax3 module
Use of `register_error` automatically causes error client side (Ajax3 responses only).
Can set HTTP error code (Ajax3 responses only) via `register_error` or `elgg_set_response_code`.

Uses standard jQuery arguments for all callbacks/promise handlers.

Caller can access full data wrapper via `jqXHR.ajaxData` (3rd argument in success callbacks).

Fixes #9767

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 26, 2016

feature(js): improves error handling in elgg/Ajax system
Adds API version option to elgg/Ajax. If set to 3, the server behaves a bit differently:
HTTP error codes can be sent back with `elgg_set_response_code`, and `register_error()`
automatically sets a 500 status code if none has been set.

All success/fail/complete callbacks and Promise handlers now receive the standard
jQuery arguments, including access to the `jqXHR` object.

The caller can access full data wrapper (as hooks did) via `jqXHR.ajaxData`.

Fixes #9767

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 27, 2016

feature(js): elgg/Ajax users get more access to underlying resources
All success/fail/complete callbacks and Promise handlers now receive the standard
jQuery arguments, including `jqXHR`, `textStatus`, and `errorThrown`.

In successful responses, `jqXHR.AjaxData` contains the wrapper object passed
through the `ajax_response` hooks, and `jqXHR.AjaxData.status` is set to
0 or -1 (indicating `register_error()` was called on the server).

The default error handler is improved so that server-encoded messages (sent
from `register_error`/`system_message`) are displayed even on HTTP error
responses. Note the API to send this type of responses has not yet been
implemented, but is planned.

Updates ElggAjaxTest to use jquery-mockjax for better coverage and tests new
features.

Fixes #9767

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 28, 2016

feature(js): elgg/Ajax users get more access to underlying resources
All success/fail/complete callbacks and Promise handlers now receive the standard
jQuery arguments, including `jqXHR`, `textStatus`, and `errorThrown`.

In successful responses, `jqXHR.AjaxData` contains the wrapper object passed
through the `ajax_response` hooks, and `jqXHR.AjaxData.status` is set to
0 or -1 (indicating `register_error()` was called on the server).

The default error handler is improved so that server-encoded messages (sent
from `register_error`/`system_message`) are displayed even on HTTP error
responses. Note the API to send this type of responses has not yet been
implemented, but is planned.

Updates ElggAjaxTest to use jquery-mockjax for better coverage and tests new
features.

Fixes #9767

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 29, 2016

feature(js): elgg/Ajax users get more access to underlying resources
All success/fail/complete callbacks and Promise handlers now receive the standard
jQuery arguments, including `jqXHR`, `textStatus`, and `errorThrown`.

In successful responses, `jqXHR.AjaxData` contains the wrapper object passed
through the `ajax_response` hooks, and `jqXHR.AjaxData.status` is set to
0 or -1 (indicating `register_error()` was called on the server).

The default error handler is improved so that server-encoded messages (sent
from `register_error`/`system_message`) are displayed even on HTTP error
responses. Note the API to send this type of responses has not yet been
implemented, but is planned.

Updates ElggAjaxTest to use jquery-mockjax for better coverage and tests new
features.

Makes developer/ajax_demo a little more like an integration test.

Fixes #9767

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 30, 2016

feature(js): elgg/Ajax users get more access to underlying resources
All success/fail/complete callbacks and Promise handlers now receive the standard
jQuery arguments, including `jqXHR`, `textStatus`, and `errorThrown`.

In successful responses, `jqXHR.AjaxData` contains the wrapper object passed
through the `ajax_response` hooks, and `jqXHR.AjaxData.status` is set to
0 or -1 (indicating `register_error()` was called on the server).

The default error handler is improved so that server-encoded messages (sent
from `register_error`/`system_message`) are displayed even on HTTP error
responses. Note the API to send this type of responses has not yet been
implemented, but is planned.

Updates ElggAjaxTest to use jquery-mockjax for better coverage and tests new
features.

Makes developer/ajax_demo a little more like an integration test.

Fixes #9767

@mrclay mrclay closed this in #9777 May 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment