Fixes #37065 - Call report_row with kwargs in Host Statuses report

[jeremylenz]

With Ruby 3-related changes in safemode 1.4 (maybe?), the Host - Statuses report breaks with

ArgumentError

unknown keywords: :Name, :Global

The issue does not occur when using safemode 1.3.8.

In my testing, to trigger this requires both of the following conditions:

1. report_row is called with the result of Hash#merge
2. The merged hash contains at least one key that is a Symbol.

To work around this, we can either (a) make sure none of our Hash keys are symbols; or (b) call report_row with kwargs rather than positional arguments, by adding ** to the call.

[evgeni]

app/views/apipie_dsl/apipie_dsls/help.html.erb also uses a Hash as an example, which is wrong (now)

and then there are app/views/unattended/report_templates/ansible_-_ansible_inventory.erb and app/views/unattended/report_templates/host_-_applied_errata.erb that could use some starts

[jeremylenz]

app/views/apipie_dsl/apipie_dsls/help.html.erb also uses a Hash as an example, which is wrong (now)

and then there are app/views/unattended/report_templates/ansible_-_ansible_inventory.erb and app/views/unattended/report_templates/host_-_applied_errata.erb that could use some starts

I think all of those will still work fine, because to break it you must use both .merge and mixed String and Symbol hash keys. A regular old hash still works.

[evgeni]

🤯

This whole kwargs handling melts my brain every day slightly more. Halp.

[jeremylenz]

I still haven't figured out exactly why this fixes it. thoughts welcome

[jeremylenz]

[test Redmine issues]

[adamruzicka]

Maybe a dead end, but wouldn't a third option be to pass **{} as a second argument to report_row to force ruby to treat the merged hashes as a positional argument?

[jeremylenz]

wouldn't a third option be to pass **{} as a second argument to report_row to force ruby to treat the merged hashes as a positional argument?

Can confirm that this works too! :exploding_head:

[jeremylenz]

[test katello]

[jeremylenz]

One possibility I was thinking about was that in addition to calling report_row with kwargs, we also make it require them. Currently the definition is just

def report_row(args)

but this would change it to

def report_row(**args)

This would make things a bit more safe because there'd be no confusion as to whether we're sending positional or keyword arguments, but

• a lot of custom reports would need updates
• it wouldn't work with older Ruby versions that don't support kwargs; not sure if that'd be an issue

Thoughts?

[ekohl]

@jeremylenz mind setting the target version in Redmine to 3.10? This is a blocker for GA

[jeremylenz]

setting the target version in Redmine to 3.10?

done

[ofedoren]

I've noticed that it works on Ruby 3.0 though... So as a different solution we could bump here safemode to < 1.4.0 until we drop Ruby 2.7 support, in that case we won't need to change anything in templates and won't break any. It seems we could even release safemode 1.3.9 with safe call operator support since it might be used by some templates already.

I think there is a place where due to Ruby 2.7 "feature" a hash is being unpacked into a series of keyword arguments automatically. I'm trying to find a way how to fix that and don't break templates or force users to update their own :/

[jeremylenz]

we could bump here safemode to < 1.4.0 until we drop Ruby 2.7 support, in that case we won't need to change anything in templates and won't break any. It seems we could even release safemode 1.3.9 with safe call operator support since it might be used by some templates already.

This seems like a better solution to me. Updated the PR. :+1:

I think there is a place where due to Ruby 2.7 "feature" a hash is being unpacked into a series of keyword arguments automatically. I'm trying to find a way how to fix that and don't break templates or force users to update their own :/

That certainly seems like what's happening!

[jeremylenz]

@ekohl I don't understand what changes you are requesting? Should I change it back to what it was before?

[ekohl]

If (and that's a big if) you're pinning to an older version you must revert 93ac254d954c46924a78860f86678ff81139fe89. As you can see, the tests are broken on Ruby 2.7. On Ruby 3.0 you also see test failures and we want to support Ruby 3.0 for EL9. So yes, I am requesting to revert to the previous patch or implement a fix in safemode itself.

[jeremylenz]

If (and that's a big if) you're pinning to an older version you must revert 93ac254

Oh yes, I have no desire to revert that. I'll change it back.

[jeremylenz]

Reverted back to passing kwargs.

[ofedoren]

I din't think we can both support Ruby 3 and safemode < 1.4. You will also need to revert the changes to use the safe operator, but I expect the tests to fail with this.

Didn't we have some similar checks anywhere before? Something like "require this gem if the Rails version is < or > than that"? I thought we could do something similar and check for the Ruby version Foreman is being installed with. But if there is no way, then yeah, need to find a different way...

AFAIK, the safe operator support can be released as a different version which doesn't require newer Ruby, so that wouldn't be an issue.

[jeremylenz]

Made the change suggested by @ofedoren. @ekohl I'll squash if you're good with this approach.

[ofedoren]

So could this be something that safemode does?

Yes. In safemode every method is removed and every call must go through method_missing to ensure it's "safe to call". We redefined the signature of method_missing in the lib so it accepts keyword arguments. For Ruby 3.0 it works well since it now actually treats hash as a hash and keywords as keywords. The problem is now with backward compatibility for Ruby 2.7 where a hash (probably if the keys are symbols) is automatically unpacked into list of keywords. This list is then being sent along with the hash, but the original report_row method doesn't accept, well, any keywords.

Here's what's going on in safemode method_missing for report_row on Ruby 2.7:

def method_missing(method, *args, **kwargs, &block)
# ...
  @delegate.send :report_row, *[{ "Build"=>"Installed", ... }], **{:Name=>"foreman.kutak.com", :Global=>"OK"}, &block
# ...
end

And here's what on Ruby 3.0:

def method_missing(method, *args, **kwargs, &block)
# ...
  @delegate.send :report_row, *[{ :Name=>"foreman.kutak.com", :Global=>"OK", "Build"=>"Installed", ... }], **{}, &block
# ...
end

Honestly, I've spent so much time on similar stuff, that it's not even fun anymore... Also, plain Ruby tests won't show you almost anything since it lacks the broken context of our "corner cases".

[ekohl]

Also, plain Ruby tests won't show you almost anything since it lacks the broken context of our "corner cases".

But we can write a test that renders the report using safemode, right? It would be more of an integration test, but I always find those to be useful for cases like this.

[ofedoren]

But we can write a test that renders the report using safemode, right? It would be more of an integration test, but I always find those to be useful for cases like this.

Sure, it should be possible, and I'm all for such tests. Just to be clear, what I meant by plain Ruby tests is going to irb and test some trivial stuff to see how it works. It doesn't always show the whole picture :/

[jeremylenz]

@ekohl @ofedoren Once you two agree on a path forward here, I will make the changes and update.

[ofedoren]

@ekohl @ofedoren Once you two agree on a path forward here, I will make the changes and update.

AFAIU, we agreed that this PR needs an integration like test, which fails without these changes, but passes with them. As Ewoud said:

But we can write a test that renders the report using safemode

[jeremylenz]

@ofedoren @ekohl Updated with a test. Confirmed that it fails on develop:

# Running tests with run options --seed 33583:
...
Error:
ReportScopeTest::#report_render#test_0004_report_row handles symbol keys with safemode merge:
ArgumentError: unknown keywords: :Name, :Global
    app/services/foreman/renderer/scope/report.rb:67:in `report_row'
    app/services/foreman/renderer/safe_mode_renderer.rb:9:in `render'
    app/services/foreman/renderer/base_renderer.rb:18:in `render'
    test/unit/foreman/renderer/scope/report_test.rb:79:in `block (3 levels) in <class:ReportScopeTest>'
    test/unit/foreman/renderer/scope/report_test.rb:78:in `block (2 levels) in <class:ReportScopeTest>'

5 tests, 13 assertions, 0 failures, 1 errors, 0 skips

and passes here:

Finished tests in 0.624685s, 8.0040 tests/s, 20.8105 assertions/s.


5 tests, 13 assertions, 0 failures, 0 errors, 0 skips

[jeremylenz]

@ekohl updated to also assert output.

[jeremylenz]

@ofedoren API docs updated per your comment.