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 only requested attributes #14734

Merged
merged 2 commits into from
Jul 6, 2017
Merged

Return only requested attributes #14734

merged 2 commits into from
Jul 6, 2017

Conversation

jntullo
Copy link

@jntullo jntullo commented Apr 11, 2017

This PR ensures that only the requested attributes are being returned when:
expand='resources'&attributes='<attributes>' is specified.

First fix: attribute_selection: This was merging attribute selection with any additional attributes. As a result, any @additional_attributes specified within the controller were being returned, no matter what the user specified attributes were.

After I fixed ^, everything else broke 😄 ...

Second fix: expand_subcollection?: With the first fix, this was no longer rendering everything as expected. From the tests that were broken, I saw that there were essentially 3 cases:

  1. Subcollection needed to be expanded if expand=resources was specified, no other attributes were specified, and it was in the collection_config
  2. A resource had an action performed on it, and it was expecting those subcollections to be expanded
  3. It was explicitly requested (ie expand='service_templates')

From the above cases I updated expand_subcollection? to use 3 methods that test those cases.

Also of note: Although the BZ states that fqname should not be returned for the automate collection, it seems that it may be done intentionally - otherwise that will need to be refactored as well

Definitely need some eyes, especially for number 2!

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

@abellotti
Copy link
Member

@jntullo yes, the fqname is returned intentionally. the id cannot be used for fetching automate resources, they pertain to different classes in the automate model. automate resources are fetched by path name (arbitratry_resource_path in our api.yml, like settings), thus the fqname needs to be returned. Thanks.

@abellotti
Copy link
Member

# Expand if: resource is being returned and subcollection is configured
# IE an update to /service_catalogs expects service_templates as part of its resource
def expand_action_resource?(sc)
@req.method != :get && collection_config.show?(sc)
Copy link
Member

Choose a reason for hiding this comment

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

@jntullo why was the @req.method != :get check needed ?

Copy link
Author

Choose a reason for hiding this comment

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

@chessbyte
Copy link
Member

@abellotti @jntullo bump

@abellotti
Copy link
Member

it seems we're losing id, i.e. fetching /api/vms?expand=resources&attributes=name,vendor returns href,name,vendor but no longer returns id. Let's add id back, could be breaking current client scripts. This PR will still be fixing
https://bugzilla.redhat.com/show_bug.cgi?id=1437201. Thanks.

@miq-bot
Copy link
Member

miq-bot commented Jul 6, 2017

Checked commits jntullo/manageiq@1515262~...0e9623d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 0 offenses detected
Everything looks fine. 🏆

@abellotti
Copy link
Member

Thanks @jntullo for fixing this. 😍

@abellotti abellotti merged commit a3e486e into ManageIQ:master Jul 6, 2017
@abellotti abellotti added this to the Sprint 64 Ending Jul 10, 2017 milestone Jul 6, 2017
@jntullo jntullo deleted the bz/expand_attributes 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.

4 participants