Fix System.ArgumentException with WebAPI args

[JustArchi]

args in this function can either be provided by the consumer, or is initialized by the function.

When it's initialized by the function (no args provided by the consumer), everything is fine since it's not possible for the args to have duplicate elements in this local scope.

When args is provided by the consumer, it is possible for the dictionary to already have the same elements in it, and therefore clash with the two we set in the lib.

This can happen as a genuine mistake, for example when user set format: json himself and the function throws an exception now because it tries to add format: vdf, but it can also happen in a more "expected" manner, for example in my use case, where I retry the request (with the same collection) upon failure. This means that if the first request fails, the dictionary that I've provided now has additional entries already added by previous SK2 call, and in result it now fails upon second try, as if I provided them myself (but I did not).

If by any chance you decide that this solution is not appropriate (for example due to first scenario above), it will be required for us to actually copy the dictionary over first and then append the entries, in order to still correct the consumer in invalid usage of providing format/key himself, while not throwing on the same dictionary used twice which was modified and returned to the consumer, and then called again. If we go this route it makes sense to mark the dictionary in the input as IReadOnlyDictionary to signalize that we're not modifying it. As it is right now, it's signalized we do.

Third option is to tell consumer "you can't re-use the same dictionary twice", but I find this argument very silly in regards to what this function really does (no reason to not go the way I suggest right now).

I believe that we don't really have to throw on the consumer in the first case. If he already provided format and/or key in the args, then we'll simply correct it as we had to anyway in order for the call to succeed. This is why I've decided to fix the code for the second scenario above, rather than re-writing the logic for dictionary cloning.

Edit: I improved the logic upon @xPaw initial thought and as it is right now we do throw on format that is not vdf, so the first use case should be met in addition to the original fix.

[JustArchi]

Ah right, I forgot to show example case where this is triggered:

Example code from ASF (interesting part):

Dictionary<string, object> arguments = new(!string.IsNullOrEmpty(tradeToken) ? 3 : 2, StringComparer.Ordinal) {
	{ "key", steamApiKey! },
	{ "steamid_target", steamID }
};

if (!string.IsNullOrEmpty(tradeToken)) {
	arguments["trade_offer_access_token"] = tradeToken!;
}

KeyValue? response = null;

// Notice the loop here
for (byte i = 0; (i < WebBrowser.MaxTries) && (response == null); i++) {
	using WebAPI.AsyncInterface econService = Bot.SteamConfiguration.GetAsyncWebAPIInterface(EconService);

	econService.Timeout = WebBrowser.Timeout;

	try {
		response = await WebLimitRequest(
			WebAPI.DefaultBaseAddress,

			// ReSharper disable once AccessToDisposedClosure
			async () => await econService.CallAsync(HttpMethod.Get, "GetTradeHoldDurations", args: arguments).ConfigureAwait(false)
		).ConfigureAwait(false);
	} catch (TaskCanceledException e) {
		Bot.ArchiLogger.LogGenericDebuggingException(e);
	} catch (Exception e) {
		Bot.ArchiLogger.LogGenericWarningException(e);
	}
}

You can see how the dictionary is preserved and comes back with additional entries starting from i = 1.

Then it crashes with:

2021-05-27 19:20:14|ArchiSteamFarm-448|WARN|abrynos|GetTradeHoldDurationForUser() System.ArgumentException: An item with the same key has already been added. Key: format
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at SteamKit2.WebAPI.AsyncInterface.CallAsync(HttpMethod method, String func, Int32 version, Dictionary`2 args)
   at ArchiSteamFarm.Steam.Integration.ArchiWebHandler.<>c__DisplayClass63_1.<<GetTradeHoldDurationForUser>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at ArchiSteamFarm.Steam.Integration.ArchiWebHandler.WebLimitRequest[T](Uri service, Func`1 function)
   at ArchiSteamFarm.Steam.Integration.ArchiWebHandler.GetTradeHoldDurationForUser(UInt64 steamID, String tradeToken)

@yaakov-h thanks in advance for considering 🙂.

[Abrynos]

As the one who found this: If you want a more detailed log, please hit me up. Otherwise: Thank you for considering the inclusion of this PR.

[xPaw]

My opinion on this is that probably should throw if you pass a format, as your current fix just silently overrides it.

I'm not so sure about key, it will only throw if you already passed in a key in the SteamConfiguration, which passes the key down to the API when you call GetAsyncWebAPIInterface, and passing in key in args is probably a mistake.

[JustArchi]

It's not a mistake, I do it all the time as I retrieve and cache the API key when it's needed during request, and not set it up in SteamConfiguration when I don't even know if I'll make any. I'll take into account your suggestion though.

[xPaw]

If you don't pass a key into SteamConfiguration, and the pass key in args, it does not throw. Is that not the case?

[JustArchi]

Yes, I fail at format, but I don't see how using the api key in the interface and crashing in the same scenario should be wanted, I fixed two problems here, even though I'm affected by one of them.

[JustArchi]

@xPaw feel free to re-evaluate after change 🙂.

[xPaw]

I missed the part where you retry the request, hence the confusing example. What's happening is that SK modifies the args dictionary, which when you retry the request it's already been modified before.

Isn't a better fix here is to make a local copy?

[JustArchi]

Copying the dict is costly, and puts pressure on the GC. Now it's not some super overhead, and this is how I'd do it if I wrote the function myself, but somebody has decided on the current approach (perhaps due to performance/overhead reason?), so at this stage I'm interested in fixing the problem rather than changing the interface.

If we decide that it's incorrect, somebody interested can always change it to local copy after this PR. I'm trying to not alter the original intention behind it, as it was not me who decided on it - there can always be a reason I don't see.

But yes, I was also surprised it's not IReadOnlyDictionary in the input with the function creating a local copy of it.

[xPaw]

somebody has decided on the current approach

If I had to guess, when the function was written it wasn't obvious that someone would reuse the arguments.

[yaakov-h]

Well this is interesting. An indeed, reusing arguments it not something I think we expected.

Just one of the many reasons that I now default to methods using Immutable collections where reasonable.

Archi, can you add a unit test for this? IIRC you should be able to use SteamConfiguration.HttpClientFactory to stub out the core HttpeMessageHander so that the test doesn't actually hit the API servers.

Ideally I'd rather not do dictionary lookups twice (TryGetValue -> Add, ContainsKey -> Add) but unfortunately there's no API on the standard Dictionary that will avoid that. 😢

I also don't think copying the dictionary should be a problem, unless it becomes a performance bottleneck (which is highly unlikely).

However, if we can rewrite the append args bit of code just below that, we should be able to avoid modifying or copying a dictionary input. Looking at it again now I think I'd like to rewrite it anyway because geez does it have a lot of unnecessary intermediate string allocations.

[JustArchi]

@yaakov-h I've added 4 unit tests that should cover the scenarios I've presented in the original issue, all are (currently) passing.