Skip to content
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

What's new in EF8 Preview 4 #4346

Merged
merged 5 commits into from
May 16, 2023
Merged

What's new in EF8 Preview 4 #4346

merged 5 commits into from
May 16, 2023

Conversation

ajcvickers
Copy link
Contributor

Primitive collections

@ajcvickers ajcvickers requested a review from roji May 14, 2023 10:04
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Looks good! See some nits, no strong feelings on any of them.

@@ -1127,3 +1127,468 @@ In addition to the enhancements described above, EF8 Preview 2 also includes som

- [Configuration to opt out of occasionally problematic SaveChanges optimizations](https://github.com/dotnet/efcore/issues/29916)
- [Add convention types for triggers](https://github.com/dotnet/efcore/issues/28687)

## New in EF8 Preview 4
Copy link
Member

Choose a reason for hiding this comment

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

Should we just have a flat list, do we think it matters which features were introduced in which preview?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. We should discuss.

.Entity<PrimitiveCollections>()
.Property(e => e.Booleans)
.HasMaxLength(1024)
.IsUnicode(false);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe consider not showing IsUnicode(false).. I get that we want to show facet config in general, but some people may copy paste the whole thing or think that it's somehow needed for max length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't it make sense to not use Unicode for a JSON list of bool values?

Copy link
Member

Choose a reason for hiding this comment

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

For bools (non-strings in general) yeah, you're right - it's just that before you discuss only the max length part. Ok with it as-is as well.


#### Primitive collections in JSON documents

In all the examples above, column for primitive collection contains JSON. However, this is not the same as mapping [an owned entity type to a column containing a JSON document](xref:core/what-is-new/ef-core-7#json-columns), which was introduced in EF7. But what if that JSON document itself contains a primitive collection? Well, all the queries above still work in the same way! For example, imagine we move the _days visited_ data into an owned type `Visits` mapped to a JSON document:
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the below works but we don't really support this properly 😬 Since the primitive array is encoded as a string within the owned JSON document, some stuff doesn't work correctly. It may be worth at least mentioning this problem (and maybe even leave this section out until we fix this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will merge for now, and we can discuss. I was impressed that all the queries I tried actually worked. Hopefully nobody looks at the actual JSON. ;-)

ajcvickers and others added 5 commits May 16, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants