Fixes #37824 - Hide taxonomies from parts of api documentation

[adamlazik1]

Some resources like user groups, external user groups, and architectures are not scoped by taxonomies, yet the the api endpoints associated with these resources accept the organization-id and location-id options. I didn't observe any effect of these options on the api call, except for when trying to create an external user group and providing either organization-id or location-id, which causes the action to fail with an error appearing in the logs:
undefined method external_usergroups for #<{Organization/Location}: ...

I have not, however, found any simple way of fixing this. All Api::V2 controllers inherit from Api::V2::BaseController, where the taxonomy options are added through the resource_description method from Apipie. While this method can be overridden in child classes, there appears to be no way (at least I have not found such a way) of removing a parameter once it is added.

The most correct solution would be of course to create a child class inheriting from BaseController, provide the resource description with taxonomy options there, and then have all taxonomy-scoped resource controllers inherit from it. The problem is that there are many plugins in which the controllers inherit from BaseController that would all need to be updated as well. I see too much potential for breaking because of a relatively harmless bug, so in my opinion the risk is not worth to fix the issue this way.

Hence, I propose a partial solution. Hide the taxonomy options from the API documentation of the relevant resources. Hammer can also be updated to not display options with the show => false flag set.

This would not completely solve the issue but in my opinion has the best effort/result/risk reduction ratio.

[ofedoren]

Thanks, @adamlazik1, LGTM and works.

Although, I'd love to see an "automatic" solution instead of putting hide_taxonomy_options wherever it makes sense, since it's prone to human eyes to overlook places. I've tried to come up with something easy, but failed. It looks it will take much more effort within Foreman project. I think we could add such feature in apipie-rails itself though...

For now we can deal with this, since it's easy and clean. For hammer though, we'd need to make some changes to be consistent with this. Currently it won't hide these params for these models.

[ekohl]

We have the taxonomy_scope param group and when I look at the code that appears to be opt-in, but then there's also resource_description that always declares them. Perhaps that part is wrong and we need to drop these lines:

foreman/app/controllers/api/v2/base_controller.rb

Lines 10 to 11 in e937bee

	param :location_id, Integer, :required => false, :desc => N_("Set the current location context for the request")
	param :organization_id, Integer, :required => false, :desc => N_("Set the current organization context for the request")

[adamlazik1]

I can try that and see what that does, but if there is potential for things breaking in plugins then I would rather not undertake this.

[adamlazik1]

We have the taxonomy_scope param group and when I look at the code that appears to be opt-in, but then there's also resource_description that always declares them. Perhaps that part is wrong and we need to drop these lines:

foreman/app/controllers/api/v2/base_controller.rb

Lines 10 to 11 in e937bee

	param :location_id, Integer, :required => false, :desc => N_("Set the current location context for the request")
	param :organization_id, Integer, :required => false, :desc => N_("Set the current organization context for the request")

@ekohl Seems like the solution would not be that simple. I removed the lines and ran this (hostgroups have the taxonomy_scope param group):

hammer hostgroup create  --name "test group" --location-id 2
Could not create the hostgroup:
  Error: Unrecognised option '--location-id'.
  
  See: 'hammer hostgroup create --help'.

There is also a possibility of things breaking in plugins and I don't think this minor fix is worth updating all plugin repos.

[ofedoren]

@ekohl Seems like the solution would not be that simple. I removed the lines and ran this (hostgroups have the taxonomy_scope param group):

hammer hostgroup create  --name "test group" --location-id 2
Could not create the hostgroup:
  Error: Unrecognised option '--location-id'.
  
  See: 'hammer hostgroup create --help'.

Yes. But this param group is used for index action (hammer list command), you tested it with create action (hammer create command), which doesn't use this param group. If you explicitly add this group it should work.

@ekohl although it would be preferable way to explicitly say which params are actually supported, this would lead to a bigger patch and probably across multiple repos, which could lead to overlooks and breaking stuff. Eventually we would like to do so? Probably with APIv3 :D

This patch (explicit removal) is smaller and won't break anything that was already working, so I'd rather stick with that.

There is also a possibility of things breaking in plugins and I don't think this minor fix is worth updating all plugin repos.

👍 But sometimes we do 😢 And this issue is not worth it 😶‍🌫️

[ofedoren]

Waiting for revert to a previous state? :)

[adamlazik1]

Oof, no idea how that happened but I must've force pushed some code that I was testing out. Thank you for pointing this out, I will fix it.

[adamlazik1]

Okay, now it should be in the previous state. Works as expected.

[ofedoren]

The tests are red, but unrelated, PR needs rebasing.