Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Fixes #1286: String Collections support in $select. #1282
base: release-8.x
Are you sure you want to change the base?
Fixes #1286: String Collections support in $select. #1282
Changes from 32 commits
857aefb
ea34a17
3c2ab5e
79727fa
c126703
cfd9ae0
411cf73
7fc15a8
5c12e84
85be658
2c9a286
77e9ec0
e638c4d
d9c2a05
8d1b843
e4ea1d8
ac53066
e75b803
629a7b9
b780b77
5946b66
861beda
380c97a
ed8b203
c086e15
8d617fa
e41fb01
706f1eb
2275cc2
6cbec7a
d8ae66e
6105b07
18ce012
0039e0a
41ebcb0
167b3e8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primitive types covered here are not sufficient. You can refer to IsPrimitiveType(Type clrType) in ODL which contains all the types that we consider primitive both
Nullable
orNon-Nullable
primitive types. Also looking at this method, you will notice that we do not considerUri
primitive type.Please use it to rewrite this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WanjohiSammy, I am well-aware of the correct primitive types but I had to cover all the types that are not only affected by the bug but were also documented to support Primitive Collections in the EF Core 8 change log.
That being said, I took a look at said method it's not immediately evident to me exactly which types of mine you're concerned about. It appears that the only difference between that method and mine is that I explicitly added cases for some types that the other function might still support.
I am almost certain that there's not a single type mentioned there that wasn't first confirmed to be affected.
Since you've already mentioned Uri,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the confusion here stems from the fact that these two methods have similar names but serve very different purposes. The core
IsPrimitiveType()
test for types that can be mapped to EDM primitive types. But this method checks for types that should be treated as primitive collections in the context of EF core, without regards to EDM types. Maybe using a different name, more specific to this use case would alleviate the confusion. But I think it's fine to restrict this method to only the types that matter with respect to the EF core bug.