Alert Definitions: add hash_expression #76

zakiva · 2017-09-26T11:03:02Z

Depends on: ManageIQ/manageiq#16397

Changes included:

Currently, it is possible to create an alert definition via API only with an expression as MiqExpression. Following the changes in Add hash_expression to MiqAlert manageiq#15315, this PR adds the ability to create an alert with hash expression.
Backward Compatibility: we would like to keep the option to create an alert with expression as MiqExpression without explicitly passing a miq_expression attribute. So the user will have now 3 options when sending a create request and will need to pass exactly one of the following:

expression which will be saved as MiqExpression (same as before).
miq_expression.
hash_expression.

Allowing editing an alert definition (with similar logic).
Deep symbolize for data.

(This PR replaces ManageIQ/manageiq#14979 with different implementation)

zakiva · 2017-09-26T11:03:59Z

@miq-bot add_label enhancement

zakiva · 2017-09-26T11:38:27Z

@imtayadeway @moolitayer Please review
cc @simon3z @ilackarms

moolitayer · 2017-09-26T12:32:05Z

app/controllers/api/alert_definitions_controller.rb

+        raise BadRequestError, "Must not specify more than one expression for editing a #{type} resource"
+      elsif exp_values.size == 1
+        if data["hash_expression"]
+          data["miq_expression"] = nil


So as we discussed with @zakiva this handles an edge case where a user wants to replace a hash expression with a miq expression or vice versa. Honestly I don't know if we should support that

moolitayer · 2017-09-26T12:33:04Z

@imtayadeway @abellotti We need an API review here.

ilackarms · 2017-09-28T16:45:18Z

@zakiva can you provide an example of a curl / HTTP request to add an alert definition?

zakiva · 2017-10-01T11:20:13Z

@zakiva can you provide an example of a curl / HTTP request to add an alert definition?

curl -X POST -H "Content-Type: application/json" -d "@alert.json" http://localhost:3000/api/alert_definitions --user admin:smartvm

example of alert.json file:

{
   "action" : "create",
   "resource" : {
   	"description" : "Alert Description",
   	"db": "ContainerNode",
   	"miq_expression": {
   		"mode": "internal",
   		"options": {
   			},
   		"eval_method": "hostd_log_threshold"
   	},
   	"options": {
   		"notifications": {
   			"email": {
   				"from": "",
                                "to": []
            }
          }
        }
   }
}

you can use expression/miq_expression/hash_expression as you like.

zakiva · 2017-10-01T11:21:56Z

ping @imtayadeway Can you please review?

zakiva · 2017-10-23T14:44:53Z

ping @abellotti @imtayadeway

imtayadeway · 2017-10-24T17:03:31Z

app/controllers/api/alert_definitions_controller.rb


    before_action :set_additional_attributes

    def create_resource(type, id, data = {})
      assert_id_not_specified(data, type)
      assert_all_required_fields_exists(data, type, REQUIRED_FIELDS)
+      if expression_values(data).size != 1


This looks like business logic to me. What's more - it's duplicated on line 26. This should be added to the model as a validation, which will greatly simplify this code. The code here should be almost entirely concerned with deserializing the JSON request body and passing data to the model.

removed that, will add a validation in the model later on.

imtayadeway · 2017-10-24T17:04:33Z

app/controllers/api/alert_definitions_controller.rb

@@ -1,26 +1,39 @@
 module Api
  class AlertDefinitionsController < BaseController
-    REQUIRED_FIELDS = %w(description db expression options).freeze
+    REQUIRED_FIELDS = %w(description db options).freeze


I know that you're only changing this part - but I'm also confused as to why these "required fields" aren't expressed in the model too.

Agreed, I can remove them in this PR and then add the validations in the model.

zakiva · 2017-11-01T16:34:31Z

@imtayadeway PTAL

imtayadeway

@zakiva this looks better 😁 but I think there is still too much here that could be pushed further down into the model for better encapsulation

imtayadeway · 2017-11-01T16:49:43Z

app/controllers/api/alert_definitions_controller.rb

@@ -1,26 +1,31 @@
 module Api
  class AlertDefinitionsController < BaseController
-    REQUIRED_FIELDS = %w(description db expression options).freeze



You can nix this empty line too 😁

imtayadeway · 2017-11-01T16:51:18Z

app/controllers/api/alert_definitions_controller.rb

      begin
-        data["expression"] = MiqExpression.new(data["expression"])
+        create_miq_expression(data)
        data["enabled"] = true if data["enabled"].nil?


Again, I know you're not touching this here (and feel free to leave for a follow up if appropriate) but this is more business logic - "an alert definition is enabled by default" - that belongs in the model

Right, removing that too. This should be handled explicitly by the user.
cc @moolitayer @elad661

imtayadeway · 2017-11-01T16:55:08Z

app/controllers/api/alert_definitions_controller.rb

+    end
+
+    def create_miq_expression(data)
+      data["expression"] = MiqExpression.new(data["expression"]) if data["expression"]


I'm not sure I understand why this is necessary. Type coercion should be handled by the setter in the model - if that's not currently the case then that needs to be added so we don't have to munge the data here (and anywhere else for that matter).

OK, moved it to the model: ManageIQ/manageiq#16397

imtayadeway · 2017-11-01T16:56:25Z

app/controllers/api/alert_definitions_controller.rb

      rescue => err
        raise BadRequestError, "Failed to create a new alert definition - #{err}"
      end
    end

    def edit_resource(type, id = nil, data = {})
      raise BadRequestError, "Must specify an id for editing a #{type} resource" unless id
+      if expression_values(data).size == 1


Is this still needed? Again, this is another business rule that needs to be encapsulated in the model.

I will remove that too.
The intention here was to handle the case of replacing a miq expression with hash expression (and vice versa) when editing an alert, but we can leave that to the user to explicitly nullify these fields.
cc @moolitayer @elad661

imtayadeway · 2017-11-01T16:57:37Z

app/controllers/api/alert_definitions_controller.rb

        data["enabled"] = true if data["enabled"].nil?
-        super(type, id, data).serializable_hash.merge("expression" => data["expression"])
+        super(type, id, data.deep_symbolize_keys).serializable_hash.merge("expression" => expression_values(data).first)


Why is the deep_symbolize_keys needed? If it's for a particular serialized column that is keyed with symbols, I think it would be better to make that explicit rather than converting everything wholesale

We want the created alerts via API to be identical to alerts created in other places.
It is not a particular (single) column, so I think it will be nicer not to list them.

zakiva · 2017-11-06T12:15:06Z

@imtayadeway Thanks for the review! I made the requested changes.

elad661 · 2017-11-19T09:10:03Z

@imtayadeway now that the change in the model ( ManageIQ/manageiq#16397 ) is merged, can we get this PR merged too, or are there any other required changes?

bazulay · 2017-11-21T13:30:08Z

@imtayadeway @abellotti PTAL

elad661 · 2017-12-04T14:00:15Z

@imtayadeway Can you please take a look? This PR has been waiting for a re-review for almost a month now...

bazulay · 2017-12-04T15:46:33Z

@gtanzillo ping

imtayadeway

@zakiva looking good, but there is some surprising behavior that I've outlined above that I think we need to address before this is merged. Thanks!

imtayadeway · 2017-12-04T17:42:02Z

app/controllers/api/alert_definitions_controller.rb

@@ -31,5 +28,10 @@ def edit_resource(type, id = nil, data = {})
    def set_additional_attributes
      @additional_attributes = %w(expression)
    end
+
+    def update_miq_expression(data)
+      data["miq_expression"] = data["expression"]


I'm still not sure why this is needed. Doesn't the model support this via both #expression= and #miq_expression= ?

This is needed because the setter in the model for expression= looks like this: https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_alert.rb#L59-L65

if exp.kind_of?(MiqExpression) self.miq_expression = exp elsif exp.kind_of?(Hash) self.hash_expression = exp

And the data we get from the api user is always a "hash" (but not a Hash Expression), because it's parsed json.

The model will only try to create a new MiqExpression from hash data if you explicitly use the miq_expression setter, so we need to check if the user provided us an "expression" parameter, and if so explicitly say we want a miq_expression and not an hash expression.

Since hash expressions are just ruby hashes, there's no cleaner way to do this - I don't think we'd want the model to attempt parsing hashes to see if they fit the "miq expression" or "hash expression" forms and magically decide which type to use.

This is OK for now....can we look at deprecating the old API (i.e. MiqAlert#expression) in a follow up PR? Then I think a lot of the complexity of this controller could go away.

I'm fine with deprecating the MiqAlert#expression, and I can do a follow-up PR.

@moolitayer is there any reason to keep the old MiqAlert#expression API around?

In the model we would need to keep the expression due to the alerts screen.
In the API I agree we should deprecate and then drop.
(deprecate in Gaprindashvili and Drop in H?)

imtayadeway · 2017-12-04T17:45:04Z

app/controllers/api/alert_definitions_controller.rb

-        data["enabled"] = true if data["enabled"].nil?
-        super(type, id, data).serializable_hash.merge("expression" => data["expression"])
+        update_miq_expression(data) if data["expression"]
+        alert = super(type, id, data.deep_symbolize_keys).serializable_hash


I'm concerned that this deep_symbolize_keys will corrupt an incoming MiqExpression:

{"=" => {"field" => "foo", "value" => "bar"}}.deep_symbolize_keys # => {:"=" => {:field => "foo", :value => "bar"}}

imtayadeway · 2017-12-04T17:46:13Z

spec/requests/alert_definitions_spec.rb

@@ -81,7 +81,7 @@
    alert_definition = MiqAlert.find(response.parsed_body["results"].first["id"])
    expect(alert_definition).to be_truthy
    expect(alert_definition.expression.class).to eq(MiqExpression)
-    expect(alert_definition.expression.exp).to eq(sample_alert_definition["expression"])
+    expect(alert_definition.expression.exp).to eq(sample_alert_definition["expression"].deep_symbolize_keys)


This looks wrong, because miq expressions are not keyed with symbols (see above)

imtayadeway · 2017-12-04T17:47:11Z

spec/requests/alert_definitions_spec.rb

+    alert_definition = MiqAlert.find(response.parsed_body["results"].first["id"])
+    expect(alert_definition).to be_truthy
+    expect(alert_definition.expression.class).to eq(MiqExpression)
+    expect(alert_definition.expression.exp).to eq(sample_alert_definition["miq_expression"].deep_symbolize_keys)


imtayadeway · 2017-12-04T17:51:34Z

spec/requests/alert_definitions_spec.rb

+    sample_alert_definition = {
+      "description"    => "Test Alert Definition",
+      "db"             => "ContainerNode",
+      "miq_expression" => { "eval_method" => "dwh_generic", "mode" => "internal", "options" => {} },


Is this supposed to be a hash expression standing in place of an miq expression? Even if this "works", I wouldn't want us to officially support this surprising behavior

imtayadeway · 2017-12-04T17:51:49Z

spec/requests/alert_definitions_spec.rb

+      "description"     => "Test Alert Definition",
+      "db"              => "ContainerNode",
+      "hash_expression" => { "eval_method" => "nothing", "mode" => "internal", "options" => {} },
+      "miq_expression"  => { "eval_method" => "dwh_generic", "mode" => "internal", "options" => {} },


imtayadeway · 2017-12-04T17:52:15Z

spec/requests/alert_definitions_spec.rb

+      api_alert_definition_url(nil, alert_definition),
+      :params => {
+        :action          => "edit",
+        :miq_expression  => exp,


imtayadeway · 2017-12-04T17:52:27Z

spec/requests/alert_definitions_spec.rb

+      :params => {
+        :action          => "edit",
+        :hash_expression => { :eval_method => "event_threshold", :mode => "internal", :options => {} },
+        :miq_expression  => { :eval_method => "mw_heap_used", :mode => "internal", :options => {} }


It looks like this is still a "hash_expression" in place of an "miq_expression"?

oh, you're right, I didn't actually fix this one when I said it was. I fixed it now.

elad661 · 2017-12-06T16:51:33Z

Thanks for the review. I fixed the tests and changed the controller to only do .deep_symbolize_keys on hash expressions (since those do need to be symbolized).

miq-bot · 2017-12-06T19:40:32Z

Checked commit zakiva@5715db3 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. 🍰

abellotti · 2017-12-19T19:11:45Z

spec/requests/alert_definitions_spec.rb

@@ -71,7 +71,7 @@
    sample_alert_definition = {
      "description" => "Test Alert Definition",
      "db"          => "ContainerNode",
-      "expression"  => { "eval_method" => "dwh_generic", "mode" => "internal", "options" => {} },
+      "expression"  => {"exp" => {"=" => {"field" => "Vm-name", "value" => "foo"}}, "context_type" => nil},


Just curious about these changes, description mentioned being backward compatible, why are these needed ?

Because, as @imtayadeway noted in this comment #76 (comment), the expression there was not actually a MiqExpression, so that means the test was wrong. It wasn't supposed to "work" like that.

👍 thanks for clarifying.

abellotti · 2017-12-19T19:37:27Z

Thanks @zakiva for the API Enhancement and everyone for the reviews. This LGTM!! 👍

miq-bot added the enhancement label Sep 26, 2017

zakiva force-pushed the alert_hash_expression branch from 17ff0e2 to ac335f9 Compare September 26, 2017 11:29

moolitayer reviewed Sep 26, 2017

View reviewed changes

zakiva force-pushed the alert_hash_expression branch 2 times, most recently from c6c6671 to ba951e7 Compare September 26, 2017 13:59

zakiva changed the title ~~Alert Definitions: add hash_expression and symbolize options~~ Alert Definitions: add hash_expression Sep 26, 2017

imtayadeway suggested changes Oct 24, 2017

View reviewed changes

zakiva force-pushed the alert_hash_expression branch from ba951e7 to 9a06176 Compare October 25, 2017 13:02

imtayadeway suggested changes Nov 1, 2017

View reviewed changes

elad661 mentioned this pull request Nov 2, 2017

Add ManageIQ alerts module ansible/ansible#32136

Merged

zakiva force-pushed the alert_hash_expression branch 2 times, most recently from 9669ae4 to 79bbc02 Compare November 6, 2017 11:57

zakiva mentioned this pull request Nov 6, 2017

Add miq_expression setter to MiqAlert ManageIQ/manageiq#16397

Merged

imtayadeway suggested changes Dec 4, 2017

View reviewed changes

imtayadeway assigned abellotti Dec 4, 2017

imtayadeway requested a review from gtanzillo December 4, 2017 18:11

elad661 force-pushed the alert_hash_expression branch from 79bbc02 to cbe1aa7 Compare December 6, 2017 16:49

elad661 force-pushed the alert_hash_expression branch from cbe1aa7 to 07336f3 Compare December 6, 2017 19:29

Alert Definitions: add hash_expression

5715db3

elad661 force-pushed the alert_hash_expression branch from 07336f3 to 5715db3 Compare December 6, 2017 19:32

imtayadeway approved these changes Dec 7, 2017

View reviewed changes

abellotti reviewed Dec 19, 2017

View reviewed changes

abellotti approved these changes Dec 19, 2017

View reviewed changes

abellotti added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 19, 2017

abellotti merged commit 339ce22 into ManageIQ:master Dec 19, 2017

Alert Definitions: add hash_expression #76

Alert Definitions: add hash_expression #76

Conversation

zakiva commented Sep 26, 2017 • edited Loading

zakiva commented Sep 26, 2017

zakiva commented Sep 26, 2017

Choose a reason for hiding this comment

moolitayer commented Sep 26, 2017

ilackarms commented Sep 28, 2017

zakiva commented Oct 1, 2017 • edited Loading

zakiva commented Oct 1, 2017

zakiva commented Oct 23, 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

zakiva commented Nov 1, 2017

imtayadeway left a 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

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

zakiva commented Nov 6, 2017

elad661 commented Nov 19, 2017

bazulay commented Nov 21, 2017

elad661 commented Dec 4, 2017

bazulay commented Dec 4, 2017

imtayadeway left a 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

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

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

elad661 commented Dec 6, 2017

miq-bot commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Dec 19, 2017

zakiva commented Sep 26, 2017 •

edited

Loading

zakiva commented Oct 1, 2017 •

edited

Loading