Drop ensure_builtin_repositories() (#6012)

* Drop ensure_builtin_repositories

With the new Repository classes we have the is_builtin property, so we
can easily make sure that built-ins are not removed. This allows us to
further cleanup the code by removing the ensure_builtin_repositories
function and the ALL_BUILTIN_REPOSITORIES constant.

* Make sure we add built-ins on load

* Reuse default set and avoid unnecessary copy

Reuse default set and avoid unnecessary copying during validation if
the default is not being used.

supervisor/backups/backup.py

@@ -931,5 +931,5 @@ class Backup(JobGroup):
         Return a coroutine.
         """
         return self.sys_store.update_repositories(
-            self.repositories, issue_on_error=True, replace=replace
+            set(self.repositories), issue_on_error=True, replace=replace
         )

supervisor/store/__init__.py

@@ -21,7 +21,7 @@ from .addon import AddonStore
 from .const import FILE_HASSIO_STORE, BuiltinRepository
 from .data import StoreData
 from .repository import Repository
-from .validate import SCHEMA_STORE_FILE, ensure_builtin_repositories
+from .validate import DEFAULT_REPOSITORIES, SCHEMA_STORE_FILE
 
 _LOGGER: logging.Logger = logging.getLogger(__name__)
 
@@ -63,11 +63,14 @@ class StoreManager(CoreSysAttributes, FileConfiguration):
         return self.repositories[slug]
 
     async def load(self) -> None:
-        """Start up add-on management."""
+        """Start up add-on store management."""
-        # Init custom repositories and load add-ons
+        # Make sure the built-in repositories are all present
-        await self.update_repositories(
+        # This is especially important when adding new built-in repositories
-            self._data[ATTR_REPOSITORIES], issue_on_error=True
+        # to make sure existing installations have them.
+        all_repositories: set[str] = (
+            set(self._data.get(ATTR_REPOSITORIES, [])) | DEFAULT_REPOSITORIES
         )
+        await self.update_repositories(all_repositories, issue_on_error=True)
 
     @Job(
         name="store_manager_reload",
@@ -223,7 +226,7 @@ class StoreManager(CoreSysAttributes, FileConfiguration):
     @Job(name="store_manager_update_repositories")
     async def update_repositories(
         self,
-        list_repositories: list[str],
+        list_repositories: set[str],
         *,
         issue_on_error: bool = False,
         replace: bool = True,
@@ -231,14 +234,8 @@ class StoreManager(CoreSysAttributes, FileConfiguration):
         """Update repositories by adding new ones and removing stale ones."""
         current_repositories = {repository.source for repository in self.all}
 
-        # Determine changes needed
+        # Determine repositories to add
-        if replace:
+        repositories_to_add = list_repositories - current_repositories
-            target_repositories = set(ensure_builtin_repositories(list_repositories))
-            repositories_to_add = target_repositories - current_repositories
-        else:
-            # When not replacing, just add the new repositories
-            repositories_to_add = set(list_repositories) - current_repositories
-            target_repositories = current_repositories | repositories_to_add
 
         # Add new repositories
         add_errors = await asyncio.gather(
@@ -259,7 +256,7 @@ class StoreManager(CoreSysAttributes, FileConfiguration):
             repositories_to_remove: list[Repository] = [
                 repository
                 for repository in self.all
-                if repository.source not in target_repositories
+                if repository.source not in list_repositories
                 and not repository.is_builtin
             ]
 

supervisor/store/const.py

@@ -35,7 +35,3 @@ class BuiltinRepository(StrEnum):
             return URL_HASSIO_ADDONS
         else:
             return self.value  # For URL-based repos, value is the URL
-
-
-# All repositories that are considered "built-in" and protected from removal
-ALL_BUILTIN_REPOSITORIES = {repo.value for repo in BuiltinRepository}

supervisor/store/validate.py

@@ -4,7 +4,7 @@ import voluptuous as vol
 
 from ..const import ATTR_MAINTAINER, ATTR_NAME, ATTR_REPOSITORIES, ATTR_URL
 from ..validate import RE_REPOSITORY
-from .const import ALL_BUILTIN_REPOSITORIES, BuiltinRepository
+from .const import BuiltinRepository
 
 # pylint: disable=no-value-for-parameter
 SCHEMA_REPOSITORY_CONFIG = vol.Schema(
@@ -17,15 +17,6 @@ SCHEMA_REPOSITORY_CONFIG = vol.Schema(
 )
 
 
-def ensure_builtin_repositories(addon_repositories: list[str]) -> list[str]:
-    """Ensure builtin repositories are in list.
-
-    Note: This should not be used in validation as the resulting list is not
-    stable. This can have side effects when comparing data later on.
-    """
-    return list(set(addon_repositories) | ALL_BUILTIN_REPOSITORIES)
-
-
 def validate_repository(repository: str) -> str:
     """Validate a valid repository."""
     if repository in BuiltinRepository:
@@ -44,10 +35,12 @@ def validate_repository(repository: str) -> str:
 
 repositories = vol.All([validate_repository], vol.Unique())
 
+DEFAULT_REPOSITORIES = {repo.value for repo in BuiltinRepository}
+
 SCHEMA_STORE_FILE = vol.Schema(
     {
         vol.Optional(
-            ATTR_REPOSITORIES, default=list(ALL_BUILTIN_REPOSITORIES)
+            ATTR_REPOSITORIES, default=lambda: list(DEFAULT_REPOSITORIES)
         ): repositories,
     },
     extra=vol.REMOVE_EXTRA,

tests/store/test_custom_repository.py

@@ -17,7 +17,7 @@ from supervisor.exceptions import (
 from supervisor.resolution.const import SuggestionType
 from supervisor.store import StoreManager
 from supervisor.store.addon import AddonStore
-from supervisor.store.const import ALL_BUILTIN_REPOSITORIES
+from supervisor.store.const import BuiltinRepository
 from supervisor.store.repository import Repository
 
 
@@ -50,7 +50,9 @@ async def test_add_valid_repository(
         patch("pathlib.Path.exists", return_value=True),
     ):
         if use_update:
-            await store_manager.update_repositories(current + ["http://example.com"])
+            await store_manager.update_repositories(
+                set(current) | {"http://example.com"}
+            )
         else:
             await store_manager.add_repository("http://example.com")
 
@@ -70,7 +72,7 @@ async def test_add_invalid_repository(coresys: CoreSys, store_manager: StoreMana
         ),
     ):
         await store_manager.update_repositories(
-            current + ["http://example.com"], issue_on_error=True
+            set(current) | {"http://example.com"}, issue_on_error=True
         )
 
         assert not await get_repository_by_url(
@@ -96,7 +98,9 @@ async def test_error_on_invalid_repository(
         pytest.raises(StoreError),
     ):
         if use_update:
-            await store_manager.update_repositories(current + ["http://example.com"])
+            await store_manager.update_repositories(
+                set(current) | {"http://example.com"}
+            )
         else:
             await store_manager.add_repository("http://example.com")
 
@@ -118,7 +122,7 @@ async def test_add_invalid_repository_file(
         patch("pathlib.Path.exists", return_value=False),
     ):
         await store_manager.update_repositories(
-            current + ["http://example.com"], issue_on_error=True
+            set(current) | {"http://example.com"}, issue_on_error=True
         )
 
         assert not await get_repository_by_url(
@@ -146,7 +150,7 @@ async def test_add_repository_with_git_error(
     current = coresys.store.repository_urls
     with patch("supervisor.store.git.GitRepo.load", side_effect=git_error):
         await store_manager.update_repositories(
-            current + ["http://example.com"], issue_on_error=True
+            set(current) | {"http://example.com"}, issue_on_error=True
         )
 
     assert "http://example.com" in coresys.store.repository_urls
@@ -175,7 +179,9 @@ async def test_error_on_repository_with_git_error(
         pytest.raises(StoreError),
     ):
         if use_update:
-            await store_manager.update_repositories(current + ["http://example.com"])
+            await store_manager.update_repositories(
+                set(current) | {"http://example.com"}
+            )
         else:
             await store_manager.add_repository("http://example.com")
 
@@ -189,7 +195,9 @@ async def test_preinstall_valid_repository(
 ):
     """Test add core repository valid."""
     with patch("supervisor.store.git.GitRepo.load", return_value=None):
-        await store_manager.update_repositories(list(ALL_BUILTIN_REPOSITORIES))
+        await store_manager.update_repositories(
+            {repo.value for repo in BuiltinRepository}
+        )
 
         def validate():
             assert store_manager.get("core").validate()
@@ -213,7 +221,7 @@ async def test_remove_repository(
     assert test_repository.slug in coresys.store.repositories
 
     if use_update:
-        await store_manager.update_repositories([])
+        await store_manager.update_repositories(set())
     else:
         await store_manager.remove_repository(test_repository)
 
@@ -241,7 +249,7 @@ async def test_remove_used_repository(
         match="Can't remove 'https://github.com/awesome-developer/awesome-repo'. It's used by installed add-ons",
     ):
         if use_update:
-            await store_manager.update_repositories([])
+            await store_manager.update_repositories(set())
         else:
             await store_manager.remove_repository(
                 coresys.store.repositories[store_addon.repository]
@@ -252,7 +260,7 @@ async def test_update_partial_error(coresys: CoreSys, store_manager: StoreManage
     """Test partial error on update does partial save and errors."""
     with patch("supervisor.store.repository.RepositoryGit.validate", return_value=True):
         with patch("supervisor.store.git.GitRepo.load", return_value=None):
-            await store_manager.update_repositories([])
+            await store_manager.update_repositories(set())
 
         store_manager.data.update.assert_called_once()
         store_manager.data.update.reset_mock()
@@ -268,7 +276,7 @@ async def test_update_partial_error(coresys: CoreSys, store_manager: StoreManage
             pytest.raises(StoreError),
         ):
             await store_manager.update_repositories(
-                current + ["http://example.com", "http://example2.com"]
+                set(current) | {"http://example.com", "http://example2.com"}
             )
 
     assert len(coresys.store.repository_urls) == initial + 1
@@ -303,7 +311,7 @@ async def test_add_with_update_repositories(
         ),
         patch("pathlib.Path.exists", return_value=True),
     ):
-        await store_manager.update_repositories(["http://example.com"], replace=False)
+        await store_manager.update_repositories({"http://example.com"}, replace=False)
 
     assert test_repository.source in coresys.store.repository_urls
     assert "http://example.com" in coresys.store.repository_urls
@@ -322,7 +330,7 @@ async def test_add_repository_fails_if_out_of_date(
     ):
         if use_update:
             await store_manager.update_repositories(
-                coresys.store.repository_urls + ["http://example.com"],
+                set(coresys.store.repository_urls) | {"http://example.com"}
             )
         else:
             await store_manager.add_repository("http://example.com")