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 correct resource hrefs #14549

Merged
merged 1 commit into from
Apr 6, 2017
Merged

Return correct resource hrefs #14549

merged 1 commit into from
Apr 6, 2017

Conversation

jntullo
Copy link

@jntullo jntullo commented Mar 28, 2017

This PR fixes a bug where the hrefs being returned do not match those of the collection they were requested from.

For a service request retrieved via /api/requests/:id, the resource is being returned with:

{
   "href" : "/api/service_requests/:id"
  ...
}

On the collection, GET /api/requests?expand=resources returns inconsistent hrefs, ie:

{
   "resources":[
      {"href": "/api/provision_requests/:id" ...},
      {"href": "/api/service_requests/:id"...},
      {"href": "/api/automation_requests/:id"...},
      {"href": "/api/requests/:id"...}
   ]
}

This change ensures that all hrefs from GET /api/requests?expand=resources will be consistent:

{
   "resources":[
      {"href": "/api/requests/:id" ...},
      {"href": "/api/requests/:id"...},
      {"href": "/api/equests/:id"...},
      {"href": "/api/requests/:id"...}
   ]
}

and that the call to GET /api/requests/:id will also return a matching href:

{
   "href" : "/api/requests/:id"
  ...
}

for any subclass of MiqRequest

Additional tests for service catalogs and tenant quotas were added to ensure that the correct href is being returned when it is a different object than the requested resource

@miq-bot add_label bug, api
@miq-bot assign @abellotti

# Ensures hrefs are consistent with those of the collection they were requested from
return reftype if collection_class.descendants.include?(rclass) || collection_class == rclass

collection_config.name_for_klass(rclass)
Copy link
Member

Choose a reason for hiding this comment

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

collection_config.name_for_klass(rclass) || reftype

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if above too can't be combined as:

return reftype if collection_class == rclass
collection_config.name_for_subclass(rclass) || reftype

Copy link
Member

Choose a reason for hiding this comment

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

would this work with the provider_class parameter ?

GET /api/vms?collection_class=ManageIQ::Providers::CloudManager::Vm

Copy link
Author

Choose a reason for hiding this comment

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

@abellotti it works with the collection_class parameter

collection_class = collection_class(type)

# Ensures hrefs are consistent with those of the collection they were requested from
return reftype if collection_class.descendants.include?(rclass) || collection_class == rclass
Copy link
Member

Choose a reason for hiding this comment

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

Hi @jntullo can you switch those 2 tests, collection_class == rclass would be more common, so we'd improve the performance here.

@@ -390,6 +390,7 @@ def init_st(service_template, resource_action)

it "supports single order request" do
api_basic_authorize subcollection_action_identifier(:service_catalogs, :service_templates, :order)
order_request["href"] = /service_requests/
Copy link
Member

Choose a reason for hiding this comment

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

can this be replaced by line 400 below with something like:

  expect_single_resource_query(order_request.merge("href" => a_string_matching(service_requests_url))) 

though I'm not sure if a_string_matching works with that method. maybe just.

  expect_single_resource_query(order_request.merge("href" => /service_requests/) )

Thanks

return retype is no name for klass
@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2017

Checked commit jntullo@760c5e2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 🍪

@abellotti
Copy link
Member

Thanks @jntullo for fixing this !! 😍

@abellotti abellotti merged commit fbb84f2 into ManageIQ:master Apr 6, 2017
@abellotti abellotti added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 6, 2017
simaishi pushed a commit that referenced this pull request Apr 6, 2017
Return correct resource hrefs
(cherry picked from commit fbb84f2)
@simaishi
Copy link
Contributor

simaishi commented Apr 6, 2017

Fine backport details:

$ git log -1
commit 6ebc02003489b791f94235de65c87f0c08a2038e
Author: Alberto Bellotti <[email protected]>
Date:   Thu Apr 6 13:45:17 2017 -0400

    Merge pull request #14549 from jntullo/bug/incorrect_href_specified
    
    Return correct resource hrefs
    (cherry picked from commit fbb84f2cc3463d1808fd4e86a0303bc26d2a9eba)

@simaishi
Copy link
Contributor

@jntullo @abellotti will it be a problem backporting this to Euwe?

@jntullo
Copy link
Author

jntullo commented May 26, 2017

@simaishi there are some conflicts, I will create a separate PR for Euwe

@simaishi
Copy link
Contributor

@jntullo thank you - please remove the conflict label when PR is ready!

@jntullo
Copy link
Author

jntullo commented May 26, 2017

@miq_bot remove_label conflict

PR: #15238

@simaishi
Copy link
Contributor

simaishi commented Jun 1, 2017

Backported to Euwe via #15238

@jntullo jntullo deleted the bug/incorrect_href_specified branch November 28, 2017 19:41
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