find() may raise NoUniqueMatch due to regexp matching of names
andy-maier opened this issue · 2 comments
PR #1363 (already released in 1.12.3 and 1.13.0) fixed the inconsistency that when the resource name was specified as the only filter argument for find() and findall() it was not matched via regexp matching but via string comparison.
Before this change, find() and findall() used regexp matching for the resource name only when additional filter arguments were specified. list() has always used regexp matching for the resource name regardless of whether it was the only filter argument.
The issue with regexp matching for the resource name is that find() with the name filter is usually used to select the single resource with that exact name. In cases where the resource name contains regexp special characters, it can now match multiple resources in which case find() raises NoUniqueMatch.
Real world example from M224M: It has partitions colo.test and colo_test. The call to find(name='colo.test') now raises NoUniqueMatch because both partitions match. This came up during the end2end test of a recent PR that picked random partitions and by chance happened to pick the one that surfaced the problem (see PR zhmcclient/zhmc-ansible-modules#873).
I think that the use of find() with the resource name as a single filter should not perform regexp matching on the name, because the intention usually is to get back the exactly matching one partition. find() is used by packages on top of zhmcclient to get to the resource when its name is specified (e.g. in zhmccli, zhmc ansible, zhmc prometheus exporter). When the resource name and additional filters are specified on find(), then the name filter is not considered unique enough and that's why there are additional filters specified, so that fits well with the regexp matching for the name filter. findall() and list() are not usually used for that purpose, so we could continue to match a resource name filter via regexp regardless of whether it is the only filter argument.
Just to be complete: There is also find_by_name() which has always performed a string comparison for the name, so in theory we could say that users of find() with resource name need to transition to use find_by_name(). However, there are so many uses of find() with a single name filter even in our own projects, so that the incompatibility introduced by the PR was simply larger than originally anticipated. I think that users out there have the same problem.
PS: In hindsight, we should only have list() and find_by_name(). The find() and findall() methods are redundant to that and would not have been needed. But we started out with them from the beginning.
DISCUSSION NEEDED
We need to act quickly on this, and roll it back into both 1.12 and 1.13 because these versions include the support for repexp name matching for find() and findall() in PR #1363.
Hi Andy, I think general tendency here we see is, to use find() with single name argument which did the string comparison and fetched the expected results to all users as exact name match, so they didn't understand/look for variation of find for specific string matches using find_by_name().
Considering PR #1363 fix (is/will/could) impact users in large amount as they were using it from beginning of this project, I vote to roll it back.
I created PR #1403 which undoes the regexp matching for find(), when the name is in the filter arguments. It keeps the regexp matching for findall().