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

Fix cid constant namespacing in the API #13671

Merged
merged 2 commits into from
Jan 27, 2017

Conversation

imtayadeway
Copy link
Contributor

@imtayadeway imtayadeway commented Jan 26, 2017

Does a few things:

  1. Qualify CID_OR_ID_MATCHER constant

Modules can use other modules' methods if included together in the same
class, but they can't reference another module's constants
unqualified. Thus, when referencing the CID_OR_ID_MATCHER constant,
which is included into Api::BaseController via CompressedIds, when
used in a module such as Api::BaseController::Parser it must be
qualified as BaseController::CID_OR_ID_MATCHER.

  1. Since the namespacing is fixed, we no longer need to re-include CompressedIds in Parser

3. One of the references to CID_OR_ID_MATCHER could be remove, because it is dead code, AFAICT. The intention was to parser a policy href if provided, but if so it will already have been parsed and converted to an id.

EDIT: I'll tackle (3) in an follow up

@abellotti can you double check my assumptions here? Also, I'd love to add a test for assigning/unassigning tags by href (as opposed to name/category) but I couldn't get anything to work. Could we look at this together if you want a test? Alternatively I can add one in a follow up.

@imtayadeway
Copy link
Contributor Author

@miq-bot add-label api, bug
@miq-bot assign @abellotti

@jntullo
Copy link

jntullo commented Jan 27, 2017

@imtayadeway this looks great! I also spent a bit of time today trying to get a test for assigning/unassigning to work based off of this bug/my PR but couldn't - AFAICT parse_tag_from_href isn't getting called anywhere (which is probably how we missed this bug in the first place) because the initial request parses the ID from the href before it ever gets to that point in the code. The bulk tag assign action (in #13581) has a different request format, which is why it shed light on this.

@imtayadeway
Copy link
Contributor Author

@jntullo I had a strong suspicion this was so....I came to the same conclusion with the policy stuff. I went conservative with that change in an effort to unblock @abellotti today, but I no longer think this change is necessary, so perhaps we can look at this closer tomorrow?

@abellotti
Copy link
Member

Just tried this on master and it works:

POST /api/vms/166/tags
{
  "action" : "assign",
  "resources" : [
     { "href" : "http://localhost:3000/api/tags/5" }
  ]
}

@@ -41,9 +41,6 @@ def parse_policy(data, collection, klass)

guid = data["guid"]
return klass.find_by(:guid => guid) if guid.present?

href = data["href"]
href =~ %r{^.*/#{collection}/#{CID_OR_ID_MATCHER}$} ? klass.find(from_cid(href.split('/').last)) : {}
Copy link
Member

Choose a reason for hiding this comment

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

why is this removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

☝️ #13671 (comment) item number 3. Happy to be proved wrong, but I did some testing on this endpoint and couldn't get at it

Copy link
Member

Choose a reason for hiding this comment

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

yep, you're right. handled already earlier via parse_id. Thanks.

@abellotti
Copy link
Member

it's still included by base_controller/parser/request_adapter.rb, we should only need it in base_controller.rb

@imtayadeway
Copy link
Contributor Author

it's still included by base_controller/parser/request_adapter.rb, we should only need it in base_controller.rb

RequestAdapter is a class with its own scope, so it needs to have that module included there. Everything else is a module which gets included into BaseController 😄

Modules can use other modules' methods if included together in the same
class, but they can't reference another module's constants
unqualified. Thus, when referencing the `CID_OR_ID_MATCHER` constant,
which is included into `Api::BaseController` via `CompressedIds`, when
used in a module such as `Api::BaseController::Parser` it must be
qualified as `BaseController::CID_OR_ID_MATCHER`.
@abellotti
Copy link
Member

LGTM!! will merge when 🍏

@abellotti abellotti added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 27, 2017
@miq-bot
Copy link
Member

miq-bot commented Jan 27, 2017

Checked commits imtayadeway/manageiq@5d9f8d7~...522366b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

@abellotti
Copy link
Member

🎶

@abellotti abellotti merged commit 22bf342 into ManageIQ:master Jan 27, 2017
imtayadeway added a commit to imtayadeway/manageiq that referenced this pull request Jan 27, 2017
If hrefs were supplied in the request body, they should have already
been parsed by `BaseController#update_multiple_collections` and given to
these methods as the id for the resource they represent. So these parts
of the code should been unreachable, and can be replaced by an empty
hash in the event that all other attempts to parse the resource at hand
have failed.

Follow up to ManageIQ#13671
@imtayadeway imtayadeway deleted the api/fix-cid-namespacing branch February 13, 2017 18:29
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