Use the pre-convention type mapping configuration in query

[AndriySvyryd]

Part of #25084

[AndriySvyryd]

@smitpatel @ajcvickers This change propagates the model to the query services by making them scoped. This worked better than I expected and won't break providers as long as they use the correct service registration pattern (our NTS plugins didn't).

However now I'm wondering whether this is what the users would expect to happen. They configure the mapping for Properties(), but we apply it for values that aren't mapped to any property, like constants and function returns. Perhaps we should introduce a separate method on ModelConfigurationBuilder just for this, something like Scalars()

[AndriySvyryd]

Also Properties() lets one to configure properties using the base type or generic definition, e.g. Properties<MyWrappedType<>>(). For scalars this is probably less useful and might be unexpected. It also adds complexity to type mapping finding code.

I propose that Scalars<T>() will only apply to T and T? if it's a value type and we will throw for Scalars<T?>() or if T is non-instantiable.

[ajcvickers]

I agree with the concerns over Properties, but I worry that figuring out when to use Properties and when to use Scalars will be hard. Even figuring what is and isn't a "scalar" can be hard. But I can also see that a single set of semantics (i.e. one method, however it is named) doesn't work very well. I wonder if we should punt on this for 6.0 and wait for feedback?

[AndriySvyryd]

I agree with the concerns over Properties, but I worry that figuring out when to use Properties and when to use Scalars will be hard. Even figuring what is and isn't a "scalar" can be hard. But I can also see that a single set of semantics (i.e. one method, however it is named) doesn't work very well.

This will probably require good documentation or perhaps just start with a warning that Scalars is not commonly used. In testing I found that it is rarely needed as we do a good job of inferring mappings, however in places where it is needed there are no workarounds except writing a type mapper plugin.

I wonder if we should punt on this for 6.0 and wait for feedback?

Or we can ship it in RC1 and wait for feedback.

[ajcvickers]

Okay with me to ship in RC1.

[roji]

This change propagates the model to the query services by making them scoped. This worked better than I expected and won't break providers as long as they use the correct service registration pattern (our NTS plugins didn't).

Just spent a few hours tracking down some seemingly unrelated test failures to this...

[AndriySvyryd]

Just spent a few hours tracking down some seemingly unrelated test failures to this...

Any suggestions for other providers? Should we emphasize that TryAddProviderSpecificServices should only be used for provider-specific services?

[roji]

Yeah... In theory would it be possible to actually check if services registered via TryAddProviderSpecificServices are actually not provider-specific, and throw an informative message? Though if that's complicated it may not be worth it...