Fixes #37015 - add validation error message for host_owner setting

[nofaralfasi]

No description provided.

[nofaralfasi]

When users attempt to update the host_owner setting through Hammer/API, they may not be aware of the accepted value format.
In the UI, we provide a selection list containing the names of users and usergroups, allowing users to make a choice. The selected value is then saved as id_and_type. For example, for the admin user with ID 1, the value would be 1-Users.

If a username is sent as the value for host_owner, an error occurs. In this PR, I propose modifying the error message that occurs in such cases to inform the user about the correct format.
As discussed with @Gauravtalreja1, we are actively seeking the best possible solution here.

[ekohl]

We have the id_and_type in more places. Is this the only place where it shows up?

I wonder if OwnerClassifier should raise an exception instead of silently returning nil on invalid input.

[ekohl]

If you're doing this, I think it should be clear that ID- is also the prefix for Usergroups. Right now that isn't obvious to me. And that ID must be replaced with an actual value but Users or Usergroups. I'd either go with ${id}-[Users|Usergroups] or $id-$type where type is Users or Usergroups.

[ShimShtein]

Why would you raise an exception and immediately rescue it?
You can add a more detailed message to the errors array instead.

[ekohl]

I suggested that OwnerClassifier would raise the exception, not the model.

[nofaralfasi]

We have the id_and_type in more places. Is this the only place where it shows up?

I didn't find any other places except here.

I wonder if OwnerClassifier should raise an exception instead of silently returning nil on invalid input.

While I agree that this is how it should be, this change is causing a lot of errors in our testing.

[nofaralfasi]

Unless someone has a better idea, I suggest moving the error messages back to the validate_host_owner method.

[stejskalleos]

The CI is failing, not sure if it's due to the changes or just unstable pipeline

[nofaralfasi]

The CI is failing, not sure if it's due to the changes or just unstable pipeline

This is due to recent changes. These modifications are raising an error in the user_or_usergroup method, which the tests do not anticipate. This is happening because other instances, such as this, also invoke the user_or_usergroup method.

So my question is, how should we approach this? Should we consider changing the implementation? I don't believe fixing the tests is the solution, given other uses of the user_or_usergroup method. We could either raise the exceptions outside the user_or_usergroup method but still within the OwnerClassifier class, or keep it as it was, adding the error only to the validate_host_owner method.

[ekohl]

I know you're refactoring, but while you're at it it's better to use .match? if you don't use the captured value. In Ruby 3.2+ you also can't use nil =~ anymore, or on other non-string values. So this is future proofing it.

Suggested change

	@id_and_type =~ /^\d+-(Users|Usergroups)$/
	@id_and_type.is_a?(String) && @id_and_type.match?(/^\d+-(Users|Usergroups)$/)

[ekohl]

Implicitly these methods can now also raise exceptions:

foreman/app/models/concerns/hostext/ownership.rb

Lines 37 to 49 in c109691

	# virtual attributes which sets the owner based on the user selection
	# supports a simple user, or a usergroup
	# selection parameter is expected to be an ActiveRecord id_and_type method (see Foreman's AR extentions).
	def is_owned_by=(selection)
	owner = OwnerClassifier.new(selection).user_or_usergroup
	self.owner = owner
	end
	def owner_suggestion
	owner_id_and_type = Setting[:host_owner]
	owner = OwnerClassifier.new(owner_id_and_type).user_or_usergroup
	self.owner || owner || User.current
	end

I wonder if that could cause problems. Setting[:host_owner] is validated, but I doubt we have code right now that ensures the ID it uses actually exists. For example, the user or group can probably be deleted, even if it's used in the setting. I don't know if there's a good way to prevent that. Perhaps worth checking with QE how far we want to go here.

[ekohl]

You're sure this isn't used anywhere? Normally we add a deprecation and remove it in a future release. Mostly for plugins.

[nofaralfasi]

I didn't find any other uses in Foreman or Katello.
I can keep the old method name, but since we are changing it to a static method, we would still need to make adjustments.

[ShimShtein]

Why not adding a static method in addition to the instance one:

class OwnerClassifier
  def self.classify_owner(owner_id_and_type)
    return self.new(owner_id_and_type).user_or_usergroup
  end

  def user_or_usergroup
    # ...
  end
end

This will keep the old way and also add the static shortcut. It will also be much more testable.

[nofaralfasi]

@ShimShtein, could you check the recent changes and confirm if they match your suggestion?

[ekohl]

I think this could be merged, but please also consider this.

Currently you call the old code from the new code. You can also implement the real logic in the new code. This allows you to change the old code to first emit a deprecation and then call the new code

[ShimShtein]

LGTM

Maybe we should start using globalid feature of rails instead of reinventing the wheel 🤔

[nofaralfasi]

Maybe we should start using globalid feature of rails instead of reinventing the wheel 🤔

I don't see how using it will help us with the API requests. What values does the user pass when setting the host_owner via API?

[stejskalleos]

🍏 LGTM

Works as expected, users and usergroup set correctly

[stejskalleos]

@ekohl could you ACK the changes so it can be released with a follow-up snap?

[ekohl]

Currently you call the old code from the new code. You can also implement the real logic in the new code. This allows you to change the old code to first emit a deprecation and then call the new code

@nofaralfasi would you mind responding to this?

[ekohl]

I'd want to get rid of using an instance of the class. That's the whole concept I'm urging you to deprecate. Here's now I'd implement it:

class OwnerClassifier
  def initialize(id_and_type)
    @id_and_type = id_and_type
  end

  def user_or_usergroup
    Foreman::Deprecation.deprecation_warning("3.12", "`user_or_usergroup` is deprecated, use `OwnerClassifier.classify_owner` instead.")

    self.classify_owner(@id_and_type)
  end

  def self.classify_owner(id_and_type)
    return nil if id_and_type.blank?

    unless id_and_type.is_a?(String) && id_and_type.match?(/^\d+-(Users|Usergroups)$/)
      raise ArgumentError, _("Invalid input format. Please use the format '${id}-[Users|Usergroups]'.")
    end

    cls = id_and_type.end_with?('Users') ? User : Usergroup
    cls.find_by!(id_and_type.to_i)
  end
end

First, it uses find_by_id! to let Rails raise the NotFound exception. This simplifies the code and probably gives a more accurate exception as well.

The bigger style difference is that it removes all the helper methods. This is to remove all state in the class so that eventually we can remove initialize() and the whole .new invocation. Otherwise you still maintain the instance every single time and you get all the overhead of a class, with the downsides of also having a static method.

The cls variable is just to make the code a bit easier to understand. The end_with? is an unimportant micro optimization.

[nofaralfasi]

I'd want to get rid of using an instance of the class. That's the whole concept I'm urging you to deprecate.

While I was trying to keep it, due to other recommendations I received in the review :)
Thank you for the detailed explanation!

[ekohl]

Shouldn't this be find_by_id like it was? Or find_by_id! to raise an exception?

[ekohl]

Ok, I looked it up myself. https://api.rubyonrails.org/classes/ActiveRecord/FinderMethods.html#method-i-find states it will call .to_i for you so it can even be:

Suggested change

	owner_type.find(id_and_type.to_i)
	owner_type.find(id_and_type)

But your code is also valid. I don't mind it being explicit here.

[nofaralfasi]

AFAIK, both find_by_id! and find raise the same exception when the record is not found (in this case). If you believe that find_by_id! enhances readability in this context, I'm open to using it instead.

[ekohl]

Can you keep a test for the deprecated way? That it at least still works.