Skip to content

200: fixed check of element type for IEnumerable<T> #204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 11, 2017
Merged

200: fixed check of element type for IEnumerable<T> #204

merged 1 commit into from
Dec 11, 2017

Conversation

shuebner
Copy link

@shuebner shuebner commented Dec 8, 2017 •
edited by jaredcnance
Loading

Closes #200

BUG FIX

  • reproduce issue in tests
  • fix issue
  • bump package version

jaredcnance
jaredcnance reviewed Dec 8, 2017
View reviewed changes
src/JsonApiDotNetCore/Extensions/TypeExtensions.cs Outdated
throw new ArgumentException($"{nameof(enumerable)} of type {enumerable.GetType().FullName} does not implement a generic variant of {nameof(IEnumerable)}");
}

if (enumerableTypes.Count() > 1)
Copy link
Contributor

@jaredcnance jaredcnance Dec 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this! Only comment I have is that we could just call SingleOrDefault rather than calling GetEnumerator three times with Any, Count and then Single (which also does a check on Count).

We can catch the InvalidOperationException (if there is more than one element), do a null check (in case there are no elements) and throw the ArgumentExceptions in the appropriate cases.

Great work!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I too felt it was a bit convoluted under the hood, but left it that way because of the readability and expressiveness of LINQ.
I am not a fan of catching exceptions that are easily avoidable and whose conditions are perfectly transparent.
So, I am going to suggest a middle ground without calling GetEnumerator multiple times, but also without catching provoked exceptions.

@shuebner shuebner force-pushed the 200_documentbuilder_crashes_when_building_arrays branch from 2b6ed9e to 0dfeb40 Compare December 11, 2017 06:48
@shuebner
Copy link
Author

shuebner commented Dec 11, 2017 •
edited
Loading

My force push seems to have removed your comment from this pull request chat.
Sorry, this is my first pull request and I thought you might not want to have multiple commits for such a simple issue.

@jaredcnance
Copy link
Contributor

jaredcnance commented Dec 11, 2017 •
edited
Loading

No worries! Comments are still there:
image

It looks good to me. I'm going to merge this a little later today. I need to PR this back to v2.1 as well so I want to allow myself some time to manage the releases. Thanks again!

Checklist for myself:

  • Merge to master
  • Bump master version to 2.2.0-beta2
  • Create release
  • PR back to v2.1
  • Bump v2.1 to v2.1.11
  • Create v2.1.11 tag and validate NuGet release

@jaredcnance jaredcnance merged commit 1226747 into json-api-dotnet:master Dec 11, 2017
@shuebner shuebner deleted the 200_documentbuilder_crashes_when_building_arrays branch December 14, 2017 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredcnance jaredcnance jaredcnance left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DocumentBuilder crashes when building Arrays
3 participants
@shuebner @jaredcnance @shuebner-zeiss