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.

Sign up for GitHub

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

Alert Definitions: add hash_expression #76

Merged
merged 1 commit into from
Dec 19, 2017
+149 −15
Merged

Alert Definitions: add hash_expression #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions app/controllers/api/alert_definitions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
module Api
class AlertDefinitionsController < BaseController
REQUIRED_FIELDS = %w(description db expression options).freeze

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)
begin
data["expression"] = MiqExpression.new(data["expression"])
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 = if data["hash_expression"]
super(type, id, data.deep_symbolize_keys).serializable_hash
else
super(type, id, data).serializable_hash
end
alert.merge("expression" => alert["miq_expression"] || alert["hash_expression"])
rescue => err
raise BadRequestError, "Failed to create a new alert definition - #{err}"
end
Expand All @@ -19,8 +20,12 @@ def create_resource(type, id, data = {})
def edit_resource(type, id = nil, data = {})
raise BadRequestError, "Must specify an id for editing a #{type} resource" unless id
begin
data["expression"] = MiqExpression.new(data["expression"]) if data["expression"]
super(type, id, data)
update_miq_expression(data) if data["expression"]
if data["hash_expression"]
super(type, id, data.deep_symbolize_keys)
else
super(type, id, data)
end
rescue => err
raise BadRequestError, "Failed to update alert definition - #{err}"
end
Expand All @@ -31,5 +36,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"]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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?)

data.delete("expression")
end
end
end
138 changes: 131 additions & 7 deletions spec/requests/alert_definitions_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks for clarifying.

"options" => { "notifications" => {"delay_next_evaluation" => 600, "evm_event" => {} } },
"enabled" => true
}
Expand All @@ -91,6 +91,66 @@
)
end

it "creates an alert definition with miq_expression" do
sample_alert_definition = {
"description" => "Test Alert Definition",
"db" => "ContainerNode",
"miq_expression" => {"exp" => {"=" => {"field" => "Vm-name", "value" => "foo"}}, "context_type" => nil},
"options" => { "notifications" => {"delay_next_evaluation" => 600, "evm_event" => {} } },
"enabled" => true
}
api_basic_authorize collection_action_identifier(:alert_definitions, :create)
post(api_alert_definitions_url, :params => sample_alert_definition)
expect(response).to have_http_status(:ok)
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"])
expect(response.parsed_body["results"].first).to include(
"description" => sample_alert_definition["description"],
"db" => sample_alert_definition["db"],
"expression" => a_hash_including(
"exp" => sample_alert_definition["miq_expression"]
)
)
end

it "creates an alert definition with hash_expression" do
sample_alert_definition = {
"description" => "Test Alert Definition",
"db" => "ContainerNode",
"hash_expression" => { "eval_method" => "dwh_generic", "mode" => "internal", "options" => {} },
"options" => { "notifications" => {"delay_next_evaluation" => 0, "evm_event" => {} } },
"enabled" => true
}
api_basic_authorize collection_action_identifier(:alert_definitions, :create)
post(api_alert_definitions_url, :params => sample_alert_definition)
expect(response).to have_http_status(:ok)
alert_definition = MiqAlert.find(response.parsed_body["results"].first["id"])
expect(alert_definition).to be_truthy
expect(alert_definition.expression.class).to eq(Hash)
expect(alert_definition.expression).to eq(sample_alert_definition["hash_expression"].deep_symbolize_keys)
expect(response.parsed_body["results"].first).to include(
"description" => sample_alert_definition["description"],
"db" => sample_alert_definition["db"],
"expression" => sample_alert_definition["hash_expression"]
)
end

it "fails to create an alert definition with more than one expression" do
sample_alert_definition = {
"description" => "Test Alert Definition",
"db" => "ContainerNode",
"hash_expression" => { "eval_method" => "nothing", "mode" => "internal", "options" => {} },
"miq_expression" => { "exp" => {"=" => {"field" => "Vm-name", "value" => "foo"}}, "context_type" => nil },
"options" => { "notifications" => {"delay_next_evaluation" => 600, "evm_event" => {} } },
"enabled" => true
}
api_basic_authorize collection_action_identifier(:alert_definitions, :create)
post(api_alert_definitions_url, :params => sample_alert_definition)
expect(response).to have_http_status(:bad_request)
end

it "deletes an alert definition via POST" do
api_basic_authorize action_identifier(:alert_definitions, :delete, :resource_actions, :post)
alert_definition = FactoryGirl.create(:miq_alert)
Expand Down Expand Up @@ -122,7 +182,7 @@
api_basic_authorize(action_identifier(:alert_definitions, :edit, :resource_actions, :post))
alert_definition = FactoryGirl.create(
:miq_alert,
:expression => { :eval_method => "mw_heap_used", :mode => "internal", :options => {} },
:expression => { "exp" => {"=" => {"field" => "Vm-name", "value" => "foo"}}},
:options => { :notifications => {:delay_next_evaluation => 0, :evm_event => {} } }
)

Expand All @@ -140,11 +200,7 @@
)

expected = {
"expression" => {
"eval_method" => "mw_heap_used",
"mode" => "internal",
"options" => {}
},
"expression" => alert_definition.expression,
"options" => {
"notifications" => {
"delay_next_evaluation" => 60,
Expand All @@ -156,6 +212,74 @@
expect(response.parsed_body).to include(expected)
end

it "edits an alert definition with miq_expression" do
api_basic_authorize(action_identifier(:alert_definitions, :edit, :resource_actions, :post))
alert_definition = FactoryGirl.create(
:miq_alert,
:miq_expression => MiqExpression.new("exp" => {"=" => {"field" => "Vm-name", "value" => "foo"}}),
:options => { :notifications => {:delay_next_evaluation => 0, :evm_event => {} } }
)

exp = { :eval_method => "nothing", :mode => "internal", :options => {} }

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

expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include("hash_expression" => exp.stringify_keys)
end

it "edits an alert definition with hash_expression to replace with miq_expression" do
api_basic_authorize(action_identifier(:alert_definitions, :edit, :resource_actions, :post))
alert_definition = FactoryGirl.create(
:miq_alert,
:hash_expression => { :eval_method => "nothing", :mode => "internal", :options => {} },
:options => { :notifications => {:delay_next_evaluation => 0, :evm_event => {} } }
)

exp = {"=" => {"field" => "Vm-name", "value" => "foo"}}

post(
api_alert_definition_url(nil, alert_definition),
:params => {
:action => "edit",
:miq_expression => exp,
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

:hash_expression => nil
}
)

expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include(
"miq_expression" => {"exp" => exp, "context_type" => nil}
)
end

it "fails to edit an alert definition with more than one expression" do
api_basic_authorize(action_identifier(:alert_definitions, :edit, :resource_actions, :post))
alert_definition = FactoryGirl.create(
:miq_alert,
:hash_expression => { :eval_method => "nothing", :mode => "internal", :options => {} },
:options => { :notifications => {:delay_next_evaluation => 0, :evm_event => {} } }
)

post(
api_alert_definition_url(nil, alert_definition),
:params => {
:action => "edit",
:hash_expression => { :eval_method => "event_threshold", :mode => "internal", :options => {} },
:miq_expression => { "exp" => {"=" => {"field" => "Vm-name", "value" => "foo"} } }
}
)

expect(response).to have_http_status(:bad_request)
end

it "edits alert definitions" do
api_basic_authorize collection_action_identifier(:alert_definitions, :edit)
alert_definitions = FactoryGirl.create_list(:miq_alert, 2)
Expand Down