A few lines above where this method is called, a variable named packageIdsNeedingNewSourceMappings is defined, with a comment saying that it's already calculated packages that don't already have source mappings.

I haven't read this code carefully enough to figure out what the answer is, but are "package IDs that don't already have mappings" being calculated twice? Or is one of the two comments no longer correct?

Yes, I think I was retrieving existing mappings again in the save method, and technically those shouldn't be modified during the preview restore, so now I am passing them down into the save.

Firstly, calling this API with the extra parameters doesn't appear to be in this PR, so I assume this PR is setting up the public API for other work which will call it.

However, I have two "bigger picture" thoughts:

1. If the customer is installing a package from a private feed, which has dependencies that come from a public feed, is this a scenario that's supposed to work?
2. Do we think in the future it will be possible to install more than 1 package in a single action?
   • Currently it's possible in PM UI's upgrade tab to update more than one package. But unless the new version changes dependencies, an update generally shouldn't need a different package source mapping.
   • But I'm wondering if a future improvement to PM UI might be to allow installing multiple packages "at once", to reduce waiting time (install package 1, wait for it to finish, install package 2, wait for it to finish).

I'm wondering if "needing more than 1 package source mapping" is an "obvious" future need, and we can avoid future public API changes if we do it now, even if we only call it with 1 package id/source per call initially.

Discussed a bit offline. The Preview restore accepts 1 package ID. I'm not sure I see how it supports batching multiple actions or if this PR should be the thing that implements such batching.
Seeking more clarification...

Github not showing me all the method parameters confused me. I thought this was one of the methods that took List<NuGetAction>. I know the method that has all the logic for install & uninstall is like that, so I made a bad assumption, sorry.

The first point, about a dependency that lives on a different package source stands. In fact, I'm now wondering if this feature will work for packages that have dependencies. If I try to install PackageA, which depende on PackageB, then this method will be called with PackageA, Source1 as parameters, but as far as I can tell, nothing will set up the source mapping for PackageB. Of course, NuGet won't know about PackageB until it resolves the graph, which happens as part of the preview restore. Anyway, all I've managed to do is convince myself this is a difficult feature to automate in a GUI.

Yes, all of the dependencies will be source mapped. The UIActionEngine is taking the addedPackages, which is everything Restore deemed as necessary to add as part of this install. I then diff that with existing mappings and only write mappings that aren't currently satisfied by another mapping.

Added packages are analyzed here: https://github.com/NuGet/NuGet.Client/pull/5109/files/e93884166bf77bc495041788db16dc9efd9b687e#diff-ac2848ccdf455d1a83de39b70803b630e3b17f94534b18d15aa8fcf6c8191921R484

Github not showing me all the method parameters confused me. I thought this was one of the methods that took List<NuGetAction>. I know the method that has all the logic for install & uninstall is like that, so I made a bad assumption, sorry.

The first point, about a dependency that lives on a different package source stands. In fact, I'm now wondering if this feature will work for packages that have dependencies. If I try to install PackageA, which depende on PackageB, then this method will be called with PackageA, Source1 as parameters, but as far as I can tell, nothing will set up the source mapping for PackageB. Of course, NuGet won't know about PackageB until it resolves the graph, which happens as part of the preview restore. Anyway, all I've managed to do is convince myself this is a difficult feature to automate in a GUI.

The newMappingID and newMappingSource pair are related. I propose the introduction of a new type that enforces the relationship between these two and encapsulates the logic of what happens when one is null and not the other?

Currently, I am not seeing a test that checks what will happen if only one of these strings is null and the other is not. But rather than just adding that test, extracting this into a type that either allows or disallows that and putting that type under test would isolate those rules in one place and allow it to be tested there.

The newMappingID and newMappingSource pair are related. I propose the introduction of a new type that enforces the relationship between these two and encapsulates the logic of what happens when one is null and not the other?

I agree it could be more convenient to have a type, especially in the future if we want to support more configurable properties from the client. However, I think it's safe to iterate on this since the API isn't one that we ship, and as far as I know nothing else in VS depends on this API. In other words, we don't have to minimize our changes to this API right now, and when we have more interesting capabilities we want to support that warrant it, then we can create and swap this over to a new type.

Currently, I am not seeing a test that checks what will happen if only one of these strings is null and the other is not.

The source code only uses them if both are not null. I'm not sure a separate test for one being null and not the other would be a useful test.

There's a test for this code path with both being null (which exercises the same code as only 1 being null):

and there's a test for both being not null, ensuring it affects the restore summary:

Do we need this new static utility class? The method here mutates the state of the PackageSourceMappingProvider, so it would seem more appropriate for this functionality to exist on that class directly rather than having it in a separate class.

The reason I didn't put these UI related concepts into the PackageSourceMappingProvider is to keep a separation of concerns and to minimize the public API changes. I wouldn't want that provider knowing about things like UserAction.

It may be that the provider can have additional logic later, but I don't want to create public APIs until we know exactly what that provider's surface should look like.

The word "save" means different things in this class. In the method SavePackageSourceMappings, it seems to add items to a list, but not persist to disk.

This variable ShouldSkipSave reflects whether the settings should be persisted to disk. This inconsistency is confusing, especially when SavePackageSourceMappings is called in places where there are comments that say it is specifically not writing to disk. Consider updating ShouldSkipSave to use a different prefix such as SkipPersisting or SkipWritingToDisk

This inconsistency is confusing, especially when SavePackageSourceMappings is called in places where there are comments that say it is specifically not writing to disk.

I could only find one comment in this helper method, which is technically accurate, but I removed it based on your feedback as well as the fact that I'm not enforcing the helper to receive a provider which has ShouldSkipSave==false.

Consider updating ShouldSkipSave to use a different prefix such as SkipPersisting or SkipWritingToDisk

The nomenclature "save" does have multiple meanings in our repo. We save changes to an ISettings in memory, and we also SaveToDisk. At least we're explicit about saving to disk, since we want that to be obvious. I don't think there's a common understanding that "save" must always mean "to disk". If so, we should probably update the terms across the entire repo at some point.
The naming I've used here was chosen to be consistent with other providers, such as

It looks like this PackageSourceMappingProvider takes in a ISettings and manipulates it for package source mapping, and decides when to write it to disk, and in some cases, skips the part about persisting to disk. A better design would be to have an implementation of ISettings that is the in-memory only representation of the settings and whose SaveToDisk method implementation no-ops.

This would also make it so we don't have to restore the settings' previous state after we've made modifications to it. We would make a copy that is an in-memory only representation but still implements ISettings, and pass that around, then throw it away when we're done with it and the settings we were passed remain untouched. It also would make it so we don't need the ShouldSkipSave on the PackageSourceMappingProvider and this class can function exactly the same way no matter what settings it was passed, without having to have deeper knowledge of whether it was supposed to persist the specific settings or not.

Yes, this is how our providers work. They orchestrate an ISettings. The ShouldSkipSave pattern and terminology is also seen in the package source provider:

If I'm understanding this correctly, it seems the responsibility of PackageSourceMappingProvider is to mutate the settings to add or remove details about Package Source Mappings based on supplied parameters about package source mappings and decide whether to persist the settings or not. If that is the case, the name PackageSourceMappingProvider doesn't seem to fit its responsibility. It also appears that the details about package source mapping items is actually provided by an outside source for most or all of this class' methods. I'd like to understand the overall intention of this class better before I could suggest an alternate name. Happy to chat offline or in the next PR review meeting.

Yes, our providers typically handle loading and manipulation of objects. For example, package sources have a provider here:

The only thing I'd consider whether we should do differently is whether anything that gets brought in via a * should actually be explicitly mapped if direct.

I don't think there's a good answer here without CD ofc :)