Return 404 error if perform an action against a non existent Physical Server #202

douglasgabriel · 2017-11-13T12:22:22Z

This PR is able to:

Fix the error that returns a 200 code if try to perform an action against a non existent PH;
Return 404 if the ID of the Physical Server doesn't exist.

douglasgabriel · 2017-11-13T12:52:53Z

Hi @imtayadeway @agrare, could you review this PR?

douglasgabriel · 2017-11-13T12:53:10Z

@miq-bot add_label gaprindashvili/yes

abellotti · 2017-11-13T14:09:46Z

while this might be ok for targeting a single resource, this will be incompatible with and break the behavior on bulk requests. Note that for bulk requests, we don't want to interrupt the request mid-flight and return the 404, callers won't know what was, wasn't done. The action result signature while is a 200 has the success attribute which can be queried. This allows us to complete bulk requests even if a couple of resources in the middle of the request do not exist.

jntullo · 2017-11-13T14:57:05Z

I agree with @abellotti. You may want to look into what has been done on delete, you may be able to do something similar to handle both scenarios if this is required.

douglasgabriel · 2017-11-13T19:44:31Z

@abellotti and @jntullo I believe that with the changes on here this problem is solved, right?

jntullo · 2017-11-13T20:05:07Z

@douglasgabriel that update has unintended consequences to other controllers as can be seen by the Travis failures. For now, I would keep the update contained to this particular method.

douglasgabriel · 2017-11-14T14:12:02Z

Sorry about that @jntullo, I fixed this

jntullo · 2017-11-14T14:46:12Z

@douglasgabriel can you please add tests? Thanks!

abellotti · 2017-11-15T20:23:30Z

app/controllers/api/base_controller.rb

+    def single_resource?
+      !@req.json_body.key?('resources')
+    end
+


I think this method should probably go in lib/api/request_adapter.rb, WDYT @jntullo ?

@abellotti yeah, i agree with that

@abellotti @jntullo Done 👍

abellotti · 2017-11-20T21:54:27Z

LGTM. @jntullo ?

miq-bot · 2017-11-21T15:59:57Z

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

abellotti · 2017-11-28T16:35:45Z

ping @imtayadeway @jntullo any updates needed here ?

imtayadeway

Thanks for your effort here! This looks agreeable in general....but I think the codeclimate complaints are justified and it would be good to address that in this PR. It's also not clear why you needed to rescue error in multiple places from your single test - I think we need at least one test for every path that's described by your changes here. Thanks!

imtayadeway

@douglasgabriel thanks for your patience! It took me a while to work through. I think I can now pinpoint what I was unsure about before: this change introduces a change of behavior for all the different actions that physical servers support, but there's only one test, for one of the actions. We are going to need tests for each action, and also ensure that we have good coverage for each action in the case that we want to preserve (bulk actions always respond with 200) as a prerequisite to getting this merged.

Indeed, the only change that's required for your existing test to pass is the following:

@@ -54,6 +54,9 @@ module Api
 
     def change_resource_state(state, type, id)
       raise BadRequestError, "Must specify an id for changing a #{type} resource" unless id
+      if single_resource? && !collection_class(type).exists?(id)
+        raise NotFoundError
+      end
 
       api_action(type, id) do |klass|
         begin

And I think that might be OK, too. You'll need to add something similar to the refresh action. In a pinch, you could extract that into a private method (though try to avoid using predicate methods as guard clauses, as you have with id_present?. ensure_id_present or similar would be better). I'd recommend trying to do that instead of trying to refactor refreshes to be a kind of state change. Even though there's incidental duplication there, that's OK - a refresh is not a state change per se and the implementation might change for different reasons to the other actions.

douglasgabriel · 2017-12-15T11:31:40Z

Hm, good point and good suggestion, I agree with you

douglasgabriel · 2017-12-15T13:29:18Z

It's done @imtayadeway 👍

abellotti · 2017-12-15T15:32:58Z

app/controllers/api/physical_servers_controller.rb

+    end
+
+    def ensure_resource_exists(type, id)
+      raise ActiveRecord::RecordNotFound if single_resource? && !collection_class(type).exists?(id)


why is this method needed ? resource_search would throw the NotFoundError plus do the necessary RBAC validation.

@abellotti it does, but we don't want it caught by the api_action block, which will always respond with a 200. Performing another resource_search would be unnecessarily expensive if we only want to check for existence.

@douglasgabriel you may want to use our own NotFoundError here

imtayadeway · 2017-12-15T15:57:25Z

app/controllers/api/physical_servers_controller.rb

@@ -41,7 +41,8 @@ def restart_mgmt_controller_resource(type, id, _data)
    end

    def refresh_resource(type, id, _data = nil)
-      raise BadRequestError, "Must specify an id for refreshing a #{type} resource" unless id


I think it would be OK not to extract this into a private method, since that alters the message to talk about "changing" here, instead of "refreshing". Again, I think it's an acceptable structural similarity.

imtayadeway · 2017-12-15T15:59:12Z

app/controllers/api/physical_servers_controller.rb

@@ -41,7 +41,8 @@ def restart_mgmt_controller_resource(type, id, _data)
    end

    def refresh_resource(type, id, _data = nil)
-      raise BadRequestError, "Must specify an id for refreshing a #{type} resource" unless id
+      ensure_id_present(type, id)
+      ensure_resource_exists(type, id)


I think it may be more appropriate to say:

ensure_resource_exists(type, id) if single_request?

Since we're trying to express that we don't care if a resource exists at this point for a bulk operation, because it's handled elsewhere.

imtayadeway · 2017-12-15T16:00:11Z

app/controllers/api/physical_servers_controller.rb

@@ -68,10 +70,6 @@ def change_resource_state(state, type, id)
      end
    end

-    def server_ident(server)


Was there a reason this was moved? I think it would be better to leave it here if it is not essential to fixing the bug

Well, ok. it was just for code organization reason

…t physical server

miq-bot · 2017-12-15T18:31:22Z

Checked commit douglasgabriel@124743a with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

imtayadeway

Thanks @douglasgabriel for your work on this! LGTM

douglasgabriel · 2017-12-15T19:34:24Z

Thank you for the support 👍

Return 404 error if perform an action against a non existent Physical Server (cherry picked from commit d17fd18)

simaishi · 2017-12-19T14:05:47Z

Gaprindashvili backport details:

$ git log -1
commit a5e20f83f4b4a760e4e1b839589a3496ed17e7e2
Author: Chris Arcand <chrisarcand@users.noreply.github.com>
Date:   Mon Dec 18 08:42:29 2017 -0600

    Merge pull request #202 from douglasgabriel/XCS-503
    
    Return 404 error if perform an action against a non existent Physical Server
    (cherry picked from commit d17fd1859f46e7fb83f1ef604418efc669a3550d)

douglasgabriel changed the base branch from master to gaprindashvili November 13, 2017 12:42

douglasgabriel changed the base branch from gaprindashvili to master November 13, 2017 12:43

agrare requested review from imtayadeway and abellotti November 13, 2017 13:30

agrare added bug providers/physical labels Nov 13, 2017

agrare assigned abellotti Nov 13, 2017

abellotti requested a review from jntullo November 13, 2017 14:09

miq-bot added the gaprindashvili/yes label Nov 13, 2017

douglasgabriel force-pushed the XCS-503 branch from d04a9aa to 50be6c4 Compare November 13, 2017 19:41

douglasgabriel force-pushed the XCS-503 branch from 7c1defb to 6304023 Compare November 14, 2017 12:45

douglasgabriel force-pushed the XCS-503 branch from 6304023 to 6566c81 Compare November 14, 2017 17:51

abellotti reviewed Nov 15, 2017

View reviewed changes

douglasgabriel force-pushed the XCS-503 branch from 6566c81 to 5aaa399 Compare November 16, 2017 11:46

abellotti approved these changes Nov 20, 2017

View reviewed changes

douglasgabriel force-pushed the XCS-503 branch from 5aaa399 to a511ced Compare November 21, 2017 11:55

miq-bot added the unmergeable label Nov 21, 2017

douglasgabriel force-pushed the XCS-503 branch from a511ced to 3720ee5 Compare November 21, 2017 18:09

miq-bot removed the unmergeable label Nov 21, 2017

douglasgabriel force-pushed the XCS-503 branch from 3720ee5 to 9c4ff0d Compare November 21, 2017 18:41

douglasgabriel force-pushed the XCS-503 branch 2 times, most recently from 1bb94f8 to 9d0a2c4 Compare November 21, 2017 19:20

imtayadeway suggested changes Nov 28, 2017

View reviewed changes

douglasgabriel force-pushed the XCS-503 branch 3 times, most recently from 2280b22 to 20d93a8 Compare December 13, 2017 13:44

imtayadeway suggested changes Dec 14, 2017

View reviewed changes

douglasgabriel force-pushed the XCS-503 branch from 20d93a8 to fd9edb3 Compare December 15, 2017 13:13

abellotti reviewed Dec 15, 2017

View reviewed changes

imtayadeway reviewed Dec 15, 2017

View reviewed changes

douglasgabriel force-pushed the XCS-503 branch from fd9edb3 to 7ed92cf Compare December 15, 2017 17:33

returning 404 error if try to perform an action against a non existen…

124743a

…t physical server

douglasgabriel force-pushed the XCS-503 branch from 7ed92cf to 124743a Compare December 15, 2017 18:21

imtayadeway approved these changes Dec 15, 2017

View reviewed changes

chrisarcand merged commit d17fd18 into ManageIQ:master Dec 18, 2017

chrisarcand assigned chrisarcand and unassigned abellotti Dec 18, 2017

simaishi pushed a commit that referenced this pull request Dec 19, 2017

Merge pull request #202 from douglasgabriel/XCS-503

a5e20f8

Return 404 error if perform an action against a non existent Physical Server (cherry picked from commit d17fd18)

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Dec 19, 2017

douglasgabriel deleted the XCS-503 branch December 19, 2017 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return 404 error if perform an action against a non existent Physical Server #202

Return 404 error if perform an action against a non existent Physical Server #202

douglasgabriel commented Nov 13, 2017

douglasgabriel commented Nov 13, 2017

douglasgabriel commented Nov 13, 2017

abellotti commented Nov 13, 2017

jntullo commented Nov 13, 2017

douglasgabriel commented Nov 13, 2017

jntullo commented Nov 13, 2017

douglasgabriel commented Nov 14, 2017

jntullo commented Nov 14, 2017

abellotti Nov 15, 2017

jntullo Nov 15, 2017

douglasgabriel Nov 20, 2017

abellotti commented Nov 20, 2017

miq-bot commented Nov 21, 2017

abellotti commented Nov 28, 2017

imtayadeway left a comment

imtayadeway left a comment •

edited

Loading

douglasgabriel commented Dec 15, 2017

douglasgabriel commented Dec 15, 2017

abellotti Dec 15, 2017

imtayadeway Dec 15, 2017

imtayadeway Dec 15, 2017

imtayadeway Dec 15, 2017

imtayadeway Dec 15, 2017

douglasgabriel Dec 15, 2017

miq-bot commented Dec 15, 2017

imtayadeway left a comment

douglasgabriel commented Dec 15, 2017

simaishi commented Dec 19, 2017

Return 404 error if perform an action against a non existent Physical Server #202

Return 404 error if perform an action against a non existent Physical Server #202

Conversation

douglasgabriel commented Nov 13, 2017

douglasgabriel commented Nov 13, 2017

douglasgabriel commented Nov 13, 2017

abellotti commented Nov 13, 2017

jntullo commented Nov 13, 2017

douglasgabriel commented Nov 13, 2017

jntullo commented Nov 13, 2017

douglasgabriel commented Nov 14, 2017

jntullo commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Nov 20, 2017

miq-bot commented Nov 21, 2017

abellotti commented Nov 28, 2017

imtayadeway left a comment

Choose a reason for hiding this comment

imtayadeway left a comment • edited Loading

Choose a reason for hiding this comment

douglasgabriel commented Dec 15, 2017

douglasgabriel commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Dec 15, 2017

imtayadeway left a comment

Choose a reason for hiding this comment

douglasgabriel commented Dec 15, 2017

simaishi commented Dec 19, 2017

imtayadeway left a comment •

edited

Loading