Can structural equality be implemented for McpServerConfig?

[AArnott]

The McpServerConfig type is a record, which tends to implement structural equality by default for scalar values. However, it has an array and a dictionary, which requires special handling to achieve structural equality.

csharp-sdk/src/ModelContextProtocol/Configuration/McpServerConfig.cs

Lines 30 to 38 in 25bcb44

	/// <summary>
	/// Arguments (if any) to pass to the executable.
	/// </summary>
	public string[]? Arguments { get; init; }
	/// <summary>
	/// Additional transport-specific configuration.
	/// </summary>
	public Dictionary<string, string>? TransportOptions { get; init; }

For my use case, when we're looking at identifying duplicate server configurations across files or across reloads of a changed mcp.json file, it's useful to be able to recognize when two server config objects are identical.

I can implement this myself with a custom IEqualityComparer, but as the config may change over time to get new properties, having it build into the type itself would be great.

[eiriktsarpalis]

For this out of the box without a custom comparer we would need to use a custom collection type that supports structural equality comparison, such as the ones used in the dotnet/runtime source generators or PolyType. The problem is that these collections are immutable, as best practice dictates that we shouldn't implement IEquatable<T> for mutable types (because they could end up as keys in a hash table).

[AArnott]

Why use init accessors on all the properties (which conveys an idea of immutability) when your collection types are mutable? Why not make them immutable as well?

[eiriktsarpalis]

It's mostly inheritance from the mcpdotnet library I would think. @halter73 is working towards making all configuration types mutable so that they better work using the options pattern.

[AArnott]

Is there another use case for McpServerConfig than to pass to a factory for IMcpClient objects? That's all I use them for, so they are 'usable once only' for me, except that I keep them to compare them with the next time I deserialize mcp.json to see if an existing MCP server is equivalent to one I just read the configuration for.

[eiriktsarpalis]

No other use case exists to my knowledge. Me and @stephentoub have been discussing the possibility of ditching the type altogether in favor of McpClientFactory just accepting a transport (or transport factory). This means that individual transport implementations would have their own factories or constructors accepting bespoke configuration that may or may not require catch-all property bags. This would require a bit more work on establishing a formal separation between a transport and a session and I believe @halter73 is working towards that.

[stephentoub]

Me and @stephentoub have been discussing the possibility of ditching the type altogether in favor of McpClientFactory just accepting a transport (or transport factory)

I'm not sure we want to remove it altogether. There's enough input here that a) it would make a big signature to accept as flattened arguments, and b) it's likely there are additional arguments we'd want to add in the future, and it's much easier to do that by adding a property to an options bag. But changing the type to be iequatable with whatever immutability requirements are necessary, or just including a concrete iequalitycomparer implementation in the lib as well, would be fine.

[eiriktsarpalis]

I'm not sure we want to remove it altogether. here's enough input here that a) it would make a big signature to accept as flattened arguments, and b) it's likely there are additional arguments we'd want to add in the future, and it's much easier to do that by adding a property to an options bag.

To be clear, I'm not suggesting we remove options objects altogether but that instead each transport implementation should define its own options object if necessary. The current approach means that some properties are applied to slightly different concepts depending on the transport:

csharp-sdk/src/ModelContextProtocol/Configuration/McpServerConfig.cs

Lines 24 to 28 in 674cb15

	/// <summary>
	/// For stdio transport: path to the executable
	/// For HTTP transport: base URL of the server
	/// </summary>
	public string? Location { get; set; }

and we're being incentivized to include catch-alls in case we need to adopt it to third-party transports:

csharp-sdk/src/ModelContextProtocol/Configuration/McpServerConfig.cs

Lines 35 to 38 in 674cb15

	/// <summary>
	/// Additional transport-specific configuration.
	/// </summary>
	public Dictionary<string, string>? TransportOptions { get; init; }

[stephentoub]

each transport implementation should define its own options object if necessary

That sounds reasonable.

[halter73]

@eiriktsarpalis Should we close this now that you removed McpServerConfig in #230?

[eiriktsarpalis]

I think we still need to do it for the individual option types used by the transports. Right @AArnott?

[AArnott]

I haven't seen the version where you removed McpServerConfig, so I can't say for sure yet.
If you're splitting data up across types that were once on one type, it may be more convenient for me to just do structural equality testing on my own type that I use to deserialize data from mcp.json where all the config info is together. If that's the case, I don't need this.
But we may also get MCP config from information outside of mcp.json, so ... it might be useful after all.