From 7e46f803d7682d1a6346045e043776e5062b8e15 Mon Sep 17 00:00:00 2001 From: Till Wegmueller Date: Wed, 19 Aug 2026 20:42:33 +0200 Subject: [PATCH] Fix AttributeError in pkgrepo when an FMRI is in two publisher stores pkg.catalog.Catalog.get_matching_fmris() documents that it returns 'references' as a dict mapping each matching FMRI to the list of patterns that matched it: { fmri1: [pat1, pat2, ...], ... } It actually mapped each FMRI to a single bare pattern string, because the dict comprehension building it used (f, p) rather than accumulating per FMRI. pkg.server.repository.Repository.get_matching_fmris() aggregates the results of every repository store and merges them with dest[k].extend(v), relying on the documented contract. As soon as the same FMRI is returned by two stores, that merge fails: # pkgrepo -s /path/to/repo remove consolidation/userland/userland-incorporation Traceback (most recent call last): ... File ".../pkg/server/repository.py", line 3598, in get_matching_fmris merge(mrefs, references) File ".../pkg/server/repository.py", line 3578, in merge dest[k].extend(v) AttributeError: 'str' object has no attribute 'extend' This is reachable on any repository where a publisher's catalog holds entries belonging to another publisher -- for instance after a rebuild has indexed manifests that ended up below the wrong publisher directory, since rebuild takes the publisher from the manifest's own pkg.fmri and not from the directory name. Once in that state, pkgrepo remove cannot be used on the repository at all unless -p is given to restrict the operation to a single store. Building references as documented also fixes a second, silent defect: when one FMRI was matched by several patterns, all but the last were discarded. The only consumer that read these values as strings is pull.py, which is updated to flatten them. pkgrepo(1) discards the references returned by remove, and pkgsurf/sign never use them. Co-Authored-By: Claude Opus 5 --- src/modules/catalog.py | 18 +++++++++-------- src/pull.py | 6 +++++- src/tests/api/t_catalog.py | 12 ++++++++++++ src/tests/cli/t_pkgrepo.py | 40 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/modules/catalog.py b/src/modules/catalog.py index f8795689c..79650840d 100644 --- a/src/modules/catalog.py +++ b/src/modules/catalog.py @@ -3354,14 +3354,16 @@ def get_matching_fmris(self, patterns): for k, l in six.iteritems(d): proposed_dict.setdefault(k, []).extend(l) - # construct references so that we can know which pattern - # generated which fmris... - references = dict([ - (f, p) - for p in ret.keys() - for flist in ret[p].values() - for f in flist - ]) + # construct references so that we can know which patterns + # generated which fmris... An FMRI can be matched by more + # than one pattern, so each entry is a list; callers (and + # this function's contract) rely on that. + references = {} + for p in ret.keys(): + for flist in ret[p].values(): + for f in flist: + references.setdefault(f, + []).append(p) return proposed_dict, references, unmatched diff --git a/src/pull.py b/src/pull.py index 8e4a7332b..97c168c0e 100755 --- a/src/pull.py +++ b/src/pull.py @@ -608,7 +608,11 @@ def get_matches(src_pub, tracker, xport, pargs, any_unmatched, any_matched, # Track anything that failed to match. any_unmatched.extend(unmatched) - any_matched.extend(set(p for p in refs.values())) + any_matched.extend(set( + p + for plist in refs.values() + for p in plist + )) matches = list(set(f for m in matches.values() for f in m)) else: matches = [f for f in src_cat.fmris()] diff --git a/src/tests/api/t_catalog.py b/src/tests/api/t_catalog.py index 3cb71a807..d0a3f6d21 100644 --- a/src/tests/api/t_catalog.py +++ b/src/tests/api/t_catalog.py @@ -776,6 +776,18 @@ def test_06_operations(self): ["xyzzy", "base"]) self.assertEqual(set(["xyzzy"]), unmatched) + # Verify that references maps each matching FMRI to the list + # of every pattern that matched it. Callers such as + # pkg.server.repository.Repository.get_matching_fmris merge + # these values together, so they have to be lists and not + # bare strings. + pdict, references, unmatched = cat.get_matching_fmris( + ["ba*", "base"]) + self.assertEqual(set(), unmatched) + self.assertEqual([p1_fmri], list(references.keys())) + self.assertEqual(["ba*", "base"], + sorted(references[p1_fmri])) + # Next, verify that removal of an FMRI not in the catalog will # raise the expected exception. Do this by removing an FMRI # and then attempting to remove it again. diff --git a/src/tests/cli/t_pkgrepo.py b/src/tests/cli/t_pkgrepo.py index 52655bfd1..bb8b008a4 100644 --- a/src/tests/cli/t_pkgrepo.py +++ b/src/tests/cli/t_pkgrepo.py @@ -1591,6 +1591,46 @@ def test_09_remove_packages(self): shutil.rmtree(src_repo) shutil.rmtree(dest_repo) + def test_09a_remove_duplicate_fmri(self): + """Verify that remove works when the same FMRI is reachable + from more than one repository store.""" + + repo_path = os.path.join(self.test_root, "dup-fmri-repo") + self.create_repo(repo_path) + self.pkgrepo("set -s {0} publisher/prefix=test".format( + repo_path)) + published = self.pkgsend_bulk(repo_path, self.zoo10) + + # Note where the manifest lives while 'test' is still the only + # store, so that the check made below is unambiguous. + mpath = self.get_repo(repo_path).manifest(published[0]) + + # Duplicate the publisher's storage under a second publisher + # directory. The copied catalog still describes its packages + # as belonging to 'test', so the very same FMRI is now + # returned by two different repository stores. This mirrors + # what a repository looks like after a rebuild has indexed + # manifests that ended up below the wrong publisher. + pub_root = os.path.join(repo_path, "publisher") + shutil.copytree(os.path.join(pub_root, "test"), + os.path.join(pub_root, "test2")) + + # Merging the per-store pattern references used to fail here + # with 'AttributeError: str object has no attribute extend'. + repo = self.get_repo(repo_path) + matching, refs = repo.get_matching_fmris(["zoo"]) + self.assertEqual(["zoo"], list(matching.keys())) + for pats in refs.values(): + self.assertEqual(["zoo"], sorted(set(pats))) + + # ...which meant remove could not be used on such a + # repository at all. + self.pkgrepo("remove -s {0} zoo".format(repo_path)) + self.assertTrue(not os.path.exists(mpath)) + + # Cleanup. + shutil.rmtree(repo_path) + def test_10_list(self): """Verify the list subcommand works as expected."""