Fixes #37849 - Support Rails 7.0

[ofedoren]

No description provided.

[ofedoren]

This PR doesn't seem to depends on Foreman's one, so it can be merged before that.

Although, still not sure if the changes in routes are OK.

[ekohl]

Technically this adds something to API v1, right? To me Rails routes are always this magic thing that I don't understand so can you explain why this change is needed?

[adamruzicka]

Is that the workaround to make the routes be picked up?

[ofedoren]

Technically this adds something to API v1, right?

I don't think so, it's still within the api namespace, which is defined above: https://github.com/theforeman/foreman_openscap/blob/master/config/routes.rb#L63-L65

Is that the workaround to make the routes be picked up?

Yeap. Still no idea why it started to fail in Rails 7, but without this patch:

With this patch:

[ekohl]

So it's no longer possible to add API routes via Foreman::Application.routes.draw or does it overwrite all the previously defined API routes? Is that a regression somewhere in our code?

And why isn't that an issue in Foreman itself? https://github.com/theforeman/foreman/blob/develop/config/routes/api/puppet/api/v2.rb appears to use the same code and theforeman/foreman#10299 doesn't change that.

[ofedoren]

So it's no longer possible to add API routes via Foreman::Application.routes.draw

It's possible, otherwise we'd not have any plugins' routes, I guess?

or does it overwrite all the previously defined API routes? Is that a regression somewhere in our code?

Hard to tell :/ I've spent a lot of time digging through Rails, but couldn't find why it suddenly changed behavior.

https://github.com/theforeman/foreman/blob/develop/config/routes/api/puppet/api/v2.rb appears to use the same code

Unfortunately, not the best example :/ I didn't test that before, so didn't know. I've tried to test that endpoint now, but it's not even usable anymore. I'd say we should nuke that one anyway because it's been 4 years since deprecation? theforeman/foreman#7719 (comment)

[ekohl]

Unfortunately, not the best example :/ I didn't test that before, so didn't know. I've tried to test that endpoint now, but it's not even usable anymore. I'd say we should nuke that one anyway because it's been 4 years since deprecation? theforeman/foreman#7719 (comment)

I didn't even notice that. I think by now we can safely drop it.

[ekohl]

My biggest issue here is that the bit you're adding to still allows v1:

foreman_openscap/config/routes.rb

Lines 63 to 65 in b6c60d6

	namespace :api, :defaults => { :format => 'json' } do
	scope "(:apiv)", :module => :v2, :defaults => { :apiv => 'v2' },
	:apiv => /v1|v2/, :constraints => ApiConstraints.new(:version => 2, :default => true) do

Foreman dropped its v1 API a long time ago so perhaps we can simply only allow v2 there.

[ekohl]

Today we discussed it may not do anything, but https://guides.rubyonrails.org/routing.html#specifying-constraints actually documents exactly what we do here:

By default the :id parameter doesn't accept dots - this is because the dot is used as a separator for formatted routes. If you need to use a dot within an :id add a constraint which overrides this - for example id: /[^\/]+/ allows anything except a slash.

So that code is there for FQDN support. I think you should be able to verify it by going to /hosts/host.example.com/policies_enc. If we don't have a test for it, perhaps write that too?

[ofedoren]

@ekohl is right, we still must write constraints, otherwise we won't support host fqdn in the URL :/

I've updated the fix (along with removing v1 support), the issue might actually be in , :only => [] part for resources that we're already introduced. Now it works as expected: no conflicts with core routes (see previous screenshot) and supports host's fqdn.

[adamruzicka]

Looks good to me, could you drop the last commit so that we can get it in?

[ofedoren]

@adamruzicka, sure, done.

[adamruzicka]

Thank you @ofedoren & @ekohl !