Refactor SourceType handling to enum by Trenly · Pull Request #6313 · microsoft/winget-cli

Trenly · 2026-06-21T21:09:37Z

📖 Description

This refactor moves internal source-type handling to Repository::SourceType enum paths across repository, CLI, and COM layers, while preserving string conversion only at external boundaries (CLI/JSON/GP/COM input-output). It also removes accidental string-based source construction paths and updates source-related tests/fixtures to use canonical source type values.

Key changes include:

SourceDetails.Type and internal source factory routing now use SourceType.
ISourceFactory::GetForType now takes SourceType.
Source constructor string-type overload removal and default-type string helper removal at call sites.
Source parsing/serialization boundaries explicitly convert using TryConvertToSourceTypeEnum/SourceTypeEnumToString.
Source-related tests updated for enum semantics, including legacy tombstone/override source-list parsing compatibility.

Copilot assisted with implementation.

🔗 References

Resolves #4463

🔍 Validation

Executed source-focused test subset:
- ./src/x64/Debug/AppInstallerCLITests/AppInstallerCLITests.exe -v high -d yes 'Source'

✅ Checklist

Signed the Contributor License Agreement
Linked to an issue
Updated Release Notes (if applicable)
Updated documentation (if applicable)
Updated Copilot instructions (if build, architecture, or conventions changed)

📋 Issue Type

Bug fix
Feature
Task

Microsoft Reviewers: Open in CodeFlow

Migrate SourceDetails and source factory internals to use SourceType enum paths while keeping string conversion at input/output boundaries. Align CLI/COM/repository flows and source-related tests with canonical enum conversion behavior, and keep legacy user source tombstone/override parsing behavior intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

florelis

I haven't finished reviewing. My main comment so far is: Why move where in the code empty types are handled? We could have kept it the same, and then you wouldn't have as many logic changes to verify.

florelis · 2026-06-29T20:23:07Z


 ## Bug Fixes

+* Refactored source type handling to use a `SourceType` enum with centralized string conversion, and added stronger `source add` type validation (#4463).


I'm not sure we need to mention this here. An internal refactor doesn't seem too relevant to someone asking "what's new in this release?"

Fair - 🤖 was just following default instructions, will remove

florelis · 2026-06-29T20:23:51Z

+        PredefinedInstalled,
+        PredefinedWriteable,
+        PackageTracking,
+        ConfigurableTest,


Should this be gated by the test hooks being enabled?

I opted to leave the enum alone and have the test hooks handle the implementation / backend so that enum indexes don't change between debug / test hook builds and release builds

florelis · 2026-06-29T21:04:29Z

+    std::optional<SourceType> TryConvertToSourceTypeEnum(std::string_view sourceType);
+
+    // Converts a SourceType enum to the corresponding canonical string.
+    std::string_view SourceTypeEnumToString(SourceType sourceType);


Nit: Is this the naming scheme we usually follow for these enum<->string conversions? I see the one below is just ToString

florelis · 2026-06-29T21:05:13Z

+        // Defaults to PreIndexedPackage if not specified, moved here from call sites during migration from
+        // String type to Enum type to avoid conversion in multiple places.


Is there any reason we'd need this historical note?

Probably not - but I thought it might help a review

florelis · 2026-06-29T21:05:49Z

+    // Converts a source type string to the corresponding SourceType enum.
+    SourceType ConvertToSourceTypeEnum(std::string_view sourceType);
+
+    // Attempts to convert a source type string to the corresponding SourceType enum.
+    std::optional<SourceType> TryConvertToSourceTypeEnum(std::string_view sourceType);


Nit: Do we need both? Do we usually have both?

We usually don't have both, and instead have <Enum>::Unknown as the fallback. The reason for having both here was to avoid needing the Unknown type in the enum

florelis · 2026-06-29T23:49:45Z

+    std::unique_ptr<ISourceFactory> ISourceFactory::GetForType(SourceType type)
    {
 #ifndef AICLI_DISABLE_TEST_HOOKS
        // Tests can ensure case matching


Remove outdated comment

florelis · 2026-06-29T23:50:03Z

            return Microsoft::ConfigurableTestSourceFactory::Create();
-        }
+#else
+        case SourceType::ConfigurableTest:


florelis · 2026-06-29T23:59:03Z

+        // If the source type was empty, the constructor of sourceDetails will have defaulted it
+        AICLI_LOG(Repo, Info, << "Adding source: Name[" << sourceDetails.Name << "], Type[" << SourceTypeEnumToString(sourceDetails.Type) << "], Arg[" << sourceDetails.Arg << "]");


I wonder if it would be worth it to keep the semantics of "type can be empty and that means default" to simplify the changes in logic for this PR. I'm not sure how much we win changing the meaning now, and whether we may be missing some place.

That's actually why I added the comment - because I wanted it to be clear that by movng to a strongly typed enum here we would be losing some of that nuance of "Type can be empty"

I'm sure that I could find a way to keep that nuance, but semantically I'd rather always have an explicit type, that way there isn't any potential confusion about what empty actually means. If empty is passed into the constructor it gets defaulted, and IMO that makes all the downstream logic simpler because callsites don't have to worry whether it is empty or not, it becomes a case they don't need to worry about.

florelis · 2026-06-30T00:02:31Z

-            if (sourceType.empty()
-                || Utility::CaseInsensitiveEquals( Repository::Microsoft::PredefinedInstalledSourceFactory::Type(), sourceType))
+            auto sourceTypeEnum = ::AppInstaller::Repository::TryConvertToSourceTypeEnum(sourceType).value_or(::AppInstaller::Repository::Source::GetDefaultSourceType());
+            if (sourceTypeEnum == ::AppInstaller::Repository::SourceType::PreIndexedPackage || sourceTypeEnum == ::AppInstaller::Repository::SourceType::PredefinedInstalled)


I'm not sure what this does, but the logic changed. If you get the 'preindexed package' as input, you will execute this conditional.

This is an artifact of not allowing an empty source type. If sourceType were empty in this new flow, TryConvertToSourceTypeEnum is an empty optional and, according to the old logic, means 'default'. This is handled by the .value_or setting it to the default.

Then, because the source type could be default or predefined installed, I check both.

In either case, I believe that the PreIndexed package is intended to follow into this conditional as it is setting the weights for the progress bar which depend on downloading and installing the index, which are both part of the pre-indexed package flow. Excluding it here would cause it to not have the same behavior

Trenly and others added 4 commits June 21, 2026 16:08

Revert unintentional whitespace change

c9ff483

Spaces instead of tabs

9b9695b

Remove noops

31af78c

Trenly marked this pull request as ready for review June 21, 2026 21:30

Trenly requested a review from a team as a code owner June 21, 2026 21:30

Merge branch 'master' into trenly/source-type-enum-4463

e79d3e3

florelis reviewed Jun 30, 2026

View reviewed changes


		## Bug Fixes

		* Refactored source type handling to use a `SourceType` enum with centralized string conversion, and added stronger `source add` type validation (#4463).

		// Defaults to PreIndexedPackage if not specified, moved here from call sites during migration from
		// String type to Enum type to avoid conversion in multiple places.

		// If the source type was empty, the constructor of sourceDetails will have defaulted it
		AICLI_LOG(Repo, Info, << "Adding source: Name[" << sourceDetails.Name << "], Type[" << SourceTypeEnumToString(sourceDetails.Type) << "], Arg[" << sourceDetails.Arg << "]");

Uh oh!

Conversation

Trenly commented Jun 21, 2026 • edited by microsoft-github-policy-service Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📖 Description

🔗 References

🔍 Validation

✅ Checklist

📋 Issue Type

Microsoft Reviewers: Open in CodeFlow

Uh oh!

florelis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trenly Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Trenly commented Jun 21, 2026 •

edited by microsoft-github-policy-service Bot

Loading

Trenly Jun 30, 2026 •

edited

Loading