Fixes #39619 - Prevent ArgumentError when find_by_attributes receives all nil values

[amolpati30]

Inside find_by_attributes, .compact strips all nils: where_attributes = {}

Operatingsystem.find_by_attributes(...) which calls generate_title(description:, name:, major:, minor:). When facts are empty, all values are nil, .compact strips them, and generate_title gets called missing required name: and major: keywords → ArgumentError.

That one line (return none if ...) makes it safe — when everything is nil, it returns an empty result set instead of all records.
ensures generate_title always receives all 4 keywords (even if nil), so it never crashes.

The steps to reproduce are mentioned in the Jira - SAT-46471

[archanaserver]

generate_title() could still contains nil value even after you pass where_attributes.empty?. Consider changing this to use 'where_attributes' instead-

Suggested change

	scope.or(where(title: generate_title(**attributes.merge(description: description))))
	scope.or(where(title: generate_title(**where_attributes.merge(description: description))))

[amolpati30]

where_attributes.empty? does not consider nil values because I am using .compact before that, which removes any nil values when the user chooses not to gather facts.

[archanaserver]

I'd recommend updating generate_title() to accept optional keyword arguments as to prevent similar crashes:

Suggested change

	def generate_title(description: nil, name: nil, major: nil, minor: nil)

The methods it calls (to_label, fullname) already handle nil gracefully, so my thinking is this would also prevents similar ArgumentError crashes from other code paths while keeping the primary protection, from the way you did at L13

[amolpati30]

Yeah, it will handle the value in the generate_title method via **attributes.merge(description: description). If I don't pass the fact, it will be black, so there is no need to mention it here, as it is already covered above.

[lzap]

There is a clear error thrown, and clear reproducer. I would really love to see a unit test that confirms that, preferably in a separate commit.

The patch mixes some refactoring that is not needed (where_attributes > attributes), would you mind either dropping that, or moving this into another separate commit?

[amolpati30]

There is a clear error thrown, and clear reproducer. I would really love to see a unit test that confirms that, preferably in a separate commit.

The patch mixes some refactoring that is not needed (where_attributes > attributes), would you mind either dropping that, or moving this into another separate commit?

The refactor fixes the issue. I tested it both with and without fact gathering, and the proposed fix does not throw any errors. I would prefer to add the unit test in the same commit rather than creating a separate commit for it.

[lzap]

I would prefer to add the unit test in the same commit rather than creating a separate commit for it.

This is okay, I can test this separately.

[lzap]

Please simplify the patch to the bare minimum.

The current proposal returns none if some conditions match. This is a behavioral change from what this used to do before the refactoring in #10789 I think. I would probably just call the generate_title if the required arguments (3) are present. @ShimShtein ?

[lzap]

Can you drop this rename from the patch and keep it minimal?

[lzap]

Missing newline between the tests.