[Debug] IndexOutOfRangeException when extension data doesn't have two generic parameters

[alanisaac]

WIP for #33011

@layomia, I added the proposed tests and code change, but I still see the tests failing in debug through both VS and the CLI (EX: Process terminated. Assertion failed).

I'm new to contributing to the runtime, so I wasn't sure if it was better to put this discussion in an issue or PR, but chose PR to better discuss the code changes. Also, pardon my newbie questions:

• The proposed change was surprising to me: should I expect that #if DEBUG would change the behavior of tests running in Debug configuration?
• Is there a reason to keep the Debug.Assert statements here at all? If conceptually System.Text.Json supports dictionaries that don't match the conditions tested by the Debug.Assert statements, what is their purpose?

[layomia]

so I wasn't sure if it was better to put this discussion in an issue or PR, but chose PR to better discuss the code changes

A PR is a fine choice to discuss code changes, particularly to fix an issue.

should I expect that #if DEBUG would change the behavior of tests running in Debug configuration?

Simply wrapping the Debug.Assert calls as this PR does has no effect. They only run in debug mode to begin with.

This #if directive allows us to execute code that will only run in debug builds of System.Text.Json. The reason I had it in https://gist.github.com/layomia/d77aeea5d3250785de5a7aa4dee95c46#file-create_data_extension_property-cs-L8-L20 was to make the following assignments which we can use to perform assertions in debug mode.

Type underlyingIDictionaryType = jsonPropertyInfo.DeclaredPropertyType.GetCompatibleGenericInterface(typeof(IDictionary<,>))!;
Type[] genericArgs = underlyingIDictionaryType.GetGenericArguments();

We don't want to do this in release mode as these calls have a non-trivial perf cost.

underlyingIDictionaryType is the type of IDictionary<,> the extension data implements. This is the type for which we should perform the checks that the first generic parameter is string, and the second generic parameter is object or JsonElement. Performing these checks on the declared type is what leads to the IndexOutOfRangeException (because the assertions may not hold).

Is there a reason to keep the Debug.Assert statements here at all?

The reason we want the assertions is to ensure that no code changes upstream that violates our expectations of the type we think we are processing in this method.

If conceptually System.Text.Json supports dictionaries that don't match the conditions tested by the Debug.Assert statements, what is their purpose?

This area of code is only concerned with initializing extension data dictionaries. We support dictionaries with string keys and values of any type, but extension properties must be of types assignable to IDictionary<string, object/JsonElement>.

[alanisaac]

https://gist.github.com/layomia/d77aeea5d3250785de5a7aa4dee95c46#file-create_data_extension_property-cs-L10-L11

🤦‍♂ I only saw the DEBUG part of the whole gist. This whole thing makes 100% more sense. Will fix.

[alanisaac]

fixed in e398543

Now, I'm finding that dictionaries that implement IDictionary<string,object> but not IDictionary run into an InvalidCastException when attempting to cast to (IDictionary?) at

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs

Line 130 in e398543

extensionData = (IDictionary?)jsonPropertyInfo.RuntimeClassInfo.CreateObject();

The same cast is used beforehand at:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs

Lines 110 to 111 in e398543

	IDictionary? extensionData = (IDictionary?)jsonPropertyInfo.GetValueAsObject(obj);
	if (extensionData == null)

Based on your comment above:

We support dictionaries with string keys and values of any type, but extension properties must be of types assignable to IDictionary<string, object/JsonElement>.

IDictionary<TKey,TValue> does not inherit the non-generic IDictionary. Must ExtensionData properties also implement IDictionary as a conceptual constraint, or is that something that should change?

FWIW, tests pass locally in netcoreapp5.0 if I remove the casts, but I don't know if changing the behavior of the InvalidCastException is desired or not.

[layomia]

Yes, we should remove the casts; particularly because they are to the wrong type. The requirement is IDictionary<string,object/JsonElement>. The IDictionary cast must have been an oversight.

We've already verified the type, and are only creating the object here; so we don't need to cast it.

[alanisaac]

fixed in d19fff9

[layomia]

nit: no need for this line.

[alanisaac]

fixed in 721b807

[layomia]

We should validate the results here.

[ahsonkhan]

[Debug] IndexOutOfRangeException when extension data doesn't have two generic parameters

I am curious, what happens in release for such test cases (without the fix in this PR)? Do things work as expected (i.e. is the issue only specific to the debug builds)?

[ahsonkhan]

Other than additional test validation, looks good.

[layomia]

[Debug] IndexOutOfRangeException when extension data doesn't have two generic parameters

I am curious, what happens in release for such test cases (without the fix in this PR)? Do things work as expected (i.e. is the issue only specific to the debug builds)?

Yes, things work as expected in release.

[layomia]

Looks good. Merging to avoid CI reset. Will tack on the additional validation in an oncoming PR.