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

Major overhaul of the "relationships" documentation #4269

Merged
merged 10 commits into from
Mar 30, 2023

Conversation

ajcvickers
Copy link
Contributor

Fixes #4145
Fixes #4053
Fixes #3900
Fixes #3810
Fixes #3668
Fixes #3648
Fixes #3282
Fixes #3282
Fixes #2924
Fixes #2807
Fixes #2494
Fixes #2321
Fixes #2266

Part of #3996

@ajcvickers ajcvickers force-pushed the PresidentialRelationships0219 branch 3 times, most recently from 1ba858c to bb39d2d Compare February 25, 2023 13:55
@ajcvickers ajcvickers requested a review from a team February 25, 2023 13:57
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.

Here's a first review, this PR is huuuge :)

I know we don't completely agree on questions of docs length/exhaustiveness, but I'd still like to make sure that a new user sees friendly, accessible docs, with an option to go into denser, longer advanced ones if they choose.

For example, big parts of content is duplicated across one-to-many, one-to-one, many-to-many (shadow foreign key, without navigation, without any navigations, alternate key, composite foreign key, without cascade delete, self-referencing). That content seems largely duplicated again in dedicated pages: Foreign and principal keys, Navigations, and to a certain extent some stuff in Conventions as well.

I think the docs would be more accessible if we exposed the basics for each navigation type in its own page (one-to-many, one-to-one...), and then just had the more advanced topics in their own pages (Foreign and principal keys, Navigations). This would make the basic pages much shorter and less intimidating, and would create a natural intro/beginner vs. advanced topics split which I think is needed.

entity-framework/core/modeling/relationships.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/relationships.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/relationships.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/relationships.md Outdated Show resolved Hide resolved
@ajcvickers ajcvickers force-pushed the PresidentialRelationships0219 branch 6 times, most recently from b073928 to 99e85ed Compare March 19, 2023 18:50
@ajcvickers ajcvickers requested a review from roji March 19, 2023 18:52
@ajcvickers
Copy link
Contributor Author

ajcvickers commented Mar 19, 2023

@roji Updated.


With this configuration the columns corresponding to `ShippingAddress` will be marked as non-nullable in the database.
## Find out more
Copy link
Member

@AndriySvyryd AndriySvyryd Mar 28, 2023

Choose a reason for hiding this comment

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

Don't we have a standard title for this? Like "See more" or "Additional resources"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, although I think "Additional resources" is somewhat different. This is very directed next steps. Happy to call it whatever.

- [Eagerly](xref:core/querying/related-data/eager) as part of a LINQ query, using `Include`.
- [Lazily](xref:core/querying/related-data/lazy) using lazy-loading proxies, or lazy-loading without proxies.
- [Explicitly](xref:core/querying/related-data/explicit) using the `Load` or `LoadAsync` methods.
- Relationships can be used in [data seeding](xref:core/modeling/data-seeding).
Copy link
Member

Choose a reason for hiding this comment

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

Except that we don't support navigations yet

- `<navigation property name>Id`
- `<principal entity type name><principal key property name>`
- `<principal entity type name>Id`

Copy link
Member

Choose a reason for hiding this comment

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

We also try to use the dependent PK as the FK if the dependent end was configured explicitly


## Cascade delete

By convention, non-nullable foreign key properties are configured to [cascade delete](xref:core/saving/cascade-delete). Nullable foreign key properties are configured to not cascade delete.
Copy link
Member

Choose a reason for hiding this comment

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

Technically it's for required relationships (they can still have nullable FKs).

```sql
CREATE INDEX "IX_Post_ContainingBlogId1_ContainingBlogId2" ON "Post" ("ContainingBlogId1", "ContainingBlogId2");
```

Copy link
Member

Choose a reason for hiding this comment

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

We should also mention that EF doesn't create indexes for properties already covered by other indexes or keys


## Principal keys

By convention, foreign keys are constrained to the primary key at the principal end of the relationship. However, an alternate key can be used instead. This is achieved using `HasPrincipalKey` on the model building API. For Example, for a single property foreign key:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
By convention, foreign keys are constrained to the primary key at the principal end of the relationship. However, an alternate key can be used instead. This is achieved using `HasPrincipalKey` on the model building API. For Example, for a single property foreign key:
By convention, foreign keys are constrained to the primary key at the principal end of the relationship. However, an alternate key can be used instead. This is achieved using `HasPrincipalKey` on the model building API. For example, for a single property foreign key:

@ajcvickers ajcvickers force-pushed the PresidentialRelationships0219 branch from 3329db9 to df7b4ba Compare March 28, 2023 09:00
@ajcvickers
Copy link
Contributor Author

Updated.

@ajcvickers ajcvickers requested a review from AndriySvyryd March 28, 2023 09:08
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.

This is a really significant improvements over our current docs, really nice.

entity-framework/core/modeling/relationships.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/relationships.md Outdated Show resolved Hide resolved
@ajcvickers ajcvickers force-pushed the PresidentialRelationships0219 branch from df7b4ba to ed737d9 Compare March 30, 2023 11:31
@ajcvickers ajcvickers merged commit ebc0394 into main Mar 30, 2023
@slch
Copy link
Contributor

slch commented Mar 30, 2023

Thanks

DickBaker pushed a commit to DickBaker/EntityFramework.Docs that referenced this pull request Jan 4, 2024
typical signature overloads have params (joinEntityName, configureRight, configureLeft, configureJoinEntityType) in that order, but all the docs invert the left / right delegate order.

This can give rise to join table with conflicting field names and FK navs, so is dangerous for learning "skip navigation".

As Arthur wrote earlier "the code gets complicated quickly and its easy to make a mistake", yup! The Issue dotnet#4269 
' Major overhaul of the "relationships" documentation' needs correcting, so I offer this PR. HTH.
ajcvickers pushed a commit that referenced this pull request Jan 9, 2024
typical signature overloads have params (joinEntityName, configureRight, configureLeft, configureJoinEntityType) in that order, but all the docs invert the left / right delegate order.

This can give rise to join table with conflicting field names and FK navs, so is dangerous for learning "skip navigation".

As Arthur wrote earlier "the code gets complicated quickly and its easy to make a mistake", yup! The Issue #4269 
' Major overhaul of the "relationships" documentation' needs correcting, so I offer this PR. HTH.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment