Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@YohDeadfall
Copy link
Contributor

Fixes #38435. I will write some tests to cover the second commit (read only and fixed size collections), but otherwise it's ready to review.

/cc @ahsonkhan @steveharter @layomia @buyaa-n

@ahsonkhan
Copy link

cc @JeremyKuhne

@YohDeadfall
Copy link
Contributor Author

I found what tends the test to fail:

IList list = (IList)state.Current.JsonPropertyInfo.GetValueAsObject(state.Current.ReturnValue);
if (list == null)
{
state.Current.JsonPropertyInfo.SetValueAsObject(state.Current.ReturnValue, value);
}
else
{
list.Add(value);
}

The code should be changed to fall into the first block and not the second one, but I can't understand what it is for. In which case the Add method should be called on the list?

@ahsonkhan ahsonkhan added this to the 3.0 milestone Jun 27, 2019
@steveharter
Copy link
Contributor

The code should be changed to fall into the first block and not the second one, but I can't understand what it is for. In which case the Add method should be called on the list?

IIRC, the first branch is when the property should be set explicitly and Add() not called. This is for arrays (the final set, not the temporary list to build up the values) and perhaps other collection types that aren't an IList.

@YohDeadfall
Copy link
Contributor Author

I see. It's just strange how it's implemented and behaves now. The first solution is to set a null value, but I don't like it. So will research how to solve it another way. On the weekend I will have enouhg time to finish it.

@YohDeadfall YohDeadfall marked this pull request as ready for review July 11, 2019 13:35
@YohDeadfall
Copy link
Contributor Author

@steveharter I haven't found a better way than this to fix the bug. Take a look, please.

@steveharter
Copy link
Contributor

@steveharter I haven't found a better way than this to fix the bug. Take a look, please.

@YohDeadfall this is more involved than I thought. I'll take over the issue and close the PR. Thanks for your research into this.

The best approach is to do this early during warm-up so we don't have to call the property getter in order to inspect every collection instance that doesn't have a setter.

@YohDeadfall YohDeadfall deleted the replace-original-collection branch July 12, 2019 17:12
@YohDeadfall
Copy link
Contributor Author

@steveharter Did you mean that you want to store the current collection in ReturnValue and not to access the setter each time? I thought about it too, but haven't touched yet. This will improve performance and make the code more observable and clear.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JsonSerializer throwing "System.NotSupportedException : Collection was of a fixed size" for some array/collection parsing

3 participants