[WIP] Raise a BadRequestError if attribute isn't on resource

[d-m-u]

I would expect GET /api/services/:id?attributes=reconfigure_dialog to return a bad request error rather than an internal_server_error from undefined method `dialog_tabs' for nil:NilClass. The issue is that we come into https://github.com/ManageIQ/manageiq/blob/master/app/models/service.rb#L338 and don't have a Reconfigure action, and then call try to find the tabs from https://github.com/ManageIQ/manageiq/blob/master/app/models/service.rb#L347.

Fix for https://bugzilla.redhat.com/show_bug.cgi?id=1663972

Depends on:
ManageIQ/manageiq#18636

[d-m-u]

@miq-bot assign @jrafanie
@miq-bot add_label bug

[abellotti]

I believe this fix masks model issues in general and should not return the 400 but the 500 it does today. The request is indeed valid as reconfigure_dialog is a valid relationship for services (OPTIONS /api/services would show that) and thus GET /api/services/:id?attributes=reconfigure_dialog is a valid API request. The fix should be moved down to the model and in this particular case, the reconfigure_dialog would need to return a nil instead of stack tracing.

[d-m-u]

So I suggested in the description changing 211 to Rbac.filtered_object(resource).try(:attribute) so that it would, in fact, return nil.

[abellotti]

AFAIR the try will fail if the attribute/method exists and raises an exception, it will only return nil if it does not exist. The issue here is the begin/rescue masks the error and returns the 400/bad request when it is indeed a good request.

[jrafanie]

I thought reconfigure_dialog was not defined on services...

[abellotti]

it is as a relationship. attributes= in the API allows one to not only query attributes, virtual attributes, but also relationships, which reconfigure_dialog is seen as such, do an OPTIONS /api/services to see the whole lot.

[miq-bot]

Checked commit d-m-u@ebcf6d8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

app/controllers/api/base_controller/renderer.rb

• ❗ - Line 212, Col 13 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

[d-m-u]

@abellotti with the main pr it returns this:
{
"error": {
"kind": "bad_request",
"message": "Invalid attributes specified: Reconfigure unsupported",
"klass": "Api::BadRequestError"
}
}

I was under the impression that this was the fix you wanted from our conversation last week but your comments here seem to indicate otherwise. Could you please clarify?

[abellotti]

Because this is a valid relationship, the expectation is that when the API fetches it, it would just get nil in this particular service, the model needs to be updated to just return nil for reconfigure_dialog if not applicable for that service. no changes needed in the API here.

Note that this is important for bulk queries too, doing something like GET /api/services?attributes=reconfigure_dialog,id,name&expand=resources would fail with the bad_request when indeed it is a valid request. Expectation here is getting the expanded services, with id, name and reconfigure_dialog returned when valid (otherwise, nil and not returned in response).

Hope this helps/clarifies it.

[d-m-u]

@abellotti okay, then will you please go review ManageIQ/manageiq#18636 for me?