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

Jump to bottom

Rails7 fixes #1263

Merged
merged 2 commits into from
Jul 23, 2024
Merged

Rails7 fixes #1263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fb626a
Rails 7 added details to the message, so simplify the check
jrafanie Jun 27, 2024
3ebce41
Remove testing of v_workflow_class
jrafanie Jul 23, 2024
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
15 changes: 6 additions & 9 deletions spec/requests/requests_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,12 @@
request.add_tag(t.name, t.children.first.name)

api_basic_authorize action_identifier(:requests, :read, :resource_actions, :get)
get api_request_url(nil, request), :params => { :attributes => "workflow,v_allowed_tags,v_workflow_class" }
get api_request_url(nil, request), :params => {:attributes => "workflow,v_allowed_tags"}

expected_response = a_hash_including(
"id" => request.id.to_s,
"workflow" => a_hash_including("values"),
"v_allowed_tags" => [a_hash_including("children")],
"v_workflow_class" => a_hash_including(
"instance_logger" => a_hash_including("klass" => request.workflow.class.to_s))
Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea why this passed on rails 6.1 🤣

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, instance_logger is an instance variable coming from the Vmdb::Logging stuff. Not sure why this was coming back previously. Maybe json dump of the class object included the ivars. That's why I'm changing it to return the Class#name as it's weird to store the Class object.

Copy link
Member

Choose a reason for hiding this comment

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

where is v_workflow_class defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_request.rb#L51

Copy link
Member Author

Choose a reason for hiding this comment

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

The rescue nil in the class method drives me mad and I think it's related to the NilClass {} above. I really have no idea how any of this stuff works but this instance_logger in the test was completely wrong and thankfully rails 7 detected it. Not sure why it worked in 6.1

Copy link
Member

@kbrock kbrock Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Wow, I had so much trouble tracking down where this method was defined delegate is not very grep friendly. It was added in ManageIQ/manageiq#13441

  virtual_column  :v_workflow_class,     :type => :string,   :uses => :workflow
  delegate :class,                       :to => :workflow,   :prefix => :v_workflow

Interestingly, there is a workflow_class method that we probably should be using instead.
But more importantly, it is declared to return a String (which it probably should be), but it returns a Class. It is possible that our serialization of a Class may have changed.

It appears that the example is not properly creating a workflow, so nil.class is returning a NilClass.

I put a byebug into the class but getting all sorts of errors:

These all work in rails c for me, but fail when I'm running this specific spec.

(byebug) MiqRequest.new.description
*** NameError Exception: uninitialized constant MiqRequest::TASK_DESCRIPTION

  default_value_for(:message)       { |r| "#{r.class::TASK_DESCRIPTION} - Request Created" }
(byebug) request.class
MiqProvisionRequest
(byebug) MiqProvisionRequest.new.description
nil
(byebug) request.v_workflow_class
*** NameError Exception: uninitialized constant MiqRequestWorkflow::DialogFieldValidation

  include DialogFieldValidation
          ^^^^^^^^^^^^^^^^^^^^^

(byebug) require "miq_request_workflow/dialog_field_validation"
(byebug) MiqProvisionRequest.new.v_workflow_class
(byebug) MiqProvisionRequest.new.workflow_class
^^^ all give the same error

I tried redefining the DialogFieldValidation to the long style (class MiqProvisionWorkflow; module DialogFieldValidator; end; end instead of module parent::DialogFieldValidator) but that did not help.

This is a tangent, but this feels a little chicken and egg.
If you want to define the child module, define the parent class (which references the child module)

Copy link
Member

Choose a reason for hiding this comment

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

update:

The MiqProvisionRequest.workflow_class references a class, but constantize throws that exception listed above. Match that with a rescue nil and we get a nil coming out. nil.class == NilClass

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, is the test and code busted or just the test? I can try to create a proper workflow to fix the test.

Copy link
Member

Choose a reason for hiding this comment

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

Thnx Jason: once I updated all my repos, I was able to load this just fine.

One thing about the code, it compares the workflow.class and the workflow_class. So if it gets busted (goes from a value to nil), then it will always pass the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm punting on this (see below). We'll tackle this if we determine the correct behavior it should have. Right now, it doesn't make sense why it's ok on rails 6.1 when it's clearly wrong. It's possible no one is using it and not reported a bug. Either way, we can handle it when it's a problem.

"id" => request.id.to_s,
"workflow" => a_hash_including("values"),
"v_allowed_tags" => [a_hash_including("children")]
)

expect(response.parsed_body).to match(expected_response)
Expand Down Expand Up @@ -314,11 +312,10 @@
:source_type => vm_template.class.name)

api_basic_authorize action_identifier(:requests, :read, :resource_actions, :get)
get api_request_url(nil, request), :params => { :attributes => "workflow,v_allowed_tags,v_workflow_class" }
get api_request_url(nil, request), :params => {:attributes => "workflow,v_allowed_tags"}

expected_response = a_hash_including(
"id" => request.id.to_s,
"v_workflow_class" => {}
"id" => request.id.to_s
)

expect(response.parsed_body).to match(expected_response)
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/servers_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@
server = FactoryBot.create(:miq_server, :status => 'started')

expect { post(api_server_url(nil, server), :params => gen_request(:delete)) }.to change(MiqServer, :count).by(0)
expect_single_action_result(:success => false, :message => 'Failed to destroy the record')
expect_single_action_result(:success => false, :message => 'Failed to destroy')
Copy link
Member Author

Choose a reason for hiding this comment

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

Rails change the method here to add more record information so it's easier to use a simpler string check to satisfy this on rails 6.1 and 7.

end

it "can delete a server with DELETE if the server is deletable" do
Expand Down