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

Return BadRequestError when invalid attributes are specified #15040

Merged
merged 2 commits into from
Jun 14, 2017
Merged

Return BadRequestError when invalid attributes are specified #15040

merged 2 commits into from
Jun 14, 2017

Conversation

jntullo
Copy link

@jntullo jntullo commented May 9, 2017

This PR consolidates attribute selection into one method, validate_attr_selection, which is able to then validate the selected attributes, and adds some other minor/related refactoring.

By consolidating and returning the physical_attrs and virtual_attrs, it also removes the need to retrieve physical attributes via physical_attribute_selection more than once.

One issue encountered is when @additional_attributes is set for a subcollection, and therefore it does not respond to attr_virtual? or attr_physical?, considering it as a virtual_attribute removes any issues.

render_resource_attr is unnecessary as normalize_hash already ensures only the selected attributes are chosen and that they are not nil.

@miq-bot add_label wip, api, enhancement
@miq-bot assign @abellotti
cc: @imtayadeway

@jntullo jntullo changed the title [WIP] Return BadRequestError when invalid attributes are specified Return BadRequestError when invalid attributes are specified May 9, 2017
@jntullo
Copy link
Author

jntullo commented May 9, 2017

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label May 9, 2017
@@ -599,7 +598,7 @@ def create_vms_by_name(names)
run_get vms_url, :expand => "resources,software"

expect_query_result(:vms, 1, 1)
expect_result_resources_to_include_keys("resources", %w(id href guid name vendor software))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow why these keys disappeared as a result of this change and whether that was intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see they were added back in the next commit. Is there a way of removing these changes from the history? Is this commit only 🍏 with this change?

@@ -739,7 +738,7 @@ def create_vms_by_name(names)
run_get vms_url(vm1.id), :attributes => "disconnected"

expect(response).to have_http_status(:ok)
expect_result_to_have_keys(%w(id href name vendor disconnected actions))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as with 90d453f#r115862423

@chessbyte
Copy link
Member

@abellotti bump

@abellotti
Copy link
Member

@jntullo can you resolve conflict and update ? Thanks.

@miq-bot
Copy link
Member

miq-bot commented Jun 9, 2017

This pull request is not mergeable. Please rebase and repush.

Jillian Tullo added 2 commits June 12, 2017 09:41
return empty array to select all attributes if only ID attributes are specified
…er; pass physical_attrs to functions instead of calling for them again
@miq-bot
Copy link
Member

miq-bot commented Jun 12, 2017

Checked commits jntullo/manageiq@f6c0ba5~...0780e5f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@abellotti
Copy link
Member

Thanks @jntullo for making this enhancement. 😍

@abellotti abellotti merged commit fdf3d0d into ManageIQ:master Jun 14, 2017
@abellotti abellotti added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 14, 2017
@jntullo jntullo deleted the enhancement/bad_request_no_attr branch November 28, 2017 19:42
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.

5 participants