Fixes #37065 - report_row now explicitly accepts args and kwargs

[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.

With this change, we make report_row explicitly accept both args and kwargs. This makes it so you don't have to do either of these workarounds.

Update: Added a test that fails without the report_row change.

[jeremylenz]

You may say, "But these hash keys look like strings!" Which is what I thought too. But Ruby always makes it a Symbol if you use the more modern Hash syntax. To get true string keys you'd have to use =>.

[ekohl]

AFAIK this has always been the case. Only => leaves the keys untouched, : there has always converted it to symbols.

[jeremylenz]

I think JavaScript object syntax has ruined my brain 🙈

[ekohl]

AFAIK this has always been the case. Only => leaves the keys untouched, : there has always converted it to symbols.

[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! 🤯

[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. 👍

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!

[ekohl]

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.

[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 93ac254. 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.

[ekohl]

Do we have a test that failed prior to this and is now solved? I'd like to have some regression testing for this.

I also fail to understand why this is an issue. In plain Ruby:

def meth(row_data)
  puts row_data.inspect
end

data = { x: 'y' }
meth(data)
meth(data.merge('a' => 'b'))

$ ruby --version
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]
$ ruby kwargs.rb 
{:x=>"y"}
{:x=>"y", "a"=>"b"}

# ruby --version
ruby 2.7.8p225 (2023-03-30 revision 1f4d455848) [x86_64-linux]
# ruby kwargs.rb 
{:x=>"y"}
{:x=>"y", "a"=>"b"}

So could this be something that safemode does? Have you tried to run the report with safemode turned off?

[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.

[ofedoren]

Thanks, @jeremylenz, LGTM!

[jeremylenz]

@ofedoren API docs updated per your comment.

[ofedoren]

Thanks a lot, @jeremylenz! @ekohl, any concerns?