Skip to content

Feat/#151: Many-to-Many Support via [HasManyThrough] #413

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

Closed
wants to merge 16 commits into from

Conversation

jaredcnance
Copy link
Contributor

@jaredcnance jaredcnance commented Sep 26, 2018

Closes #151

FEATURE

  • write tests that address the requirements outlined in the issue
  • fulfill the feature requirements

TODO

  • check the generated SQL (specifically when loading the oldLinks)
  • what happens if the through type has other properties besides the navigation properties? will this override any existing property values to default(T)? we should be able to attach the link relationship rather than loading and removing it…which should save SQL calls and prevent overriding other properties
  • document as part of the docfx migration
  • ensure this is compatible with Attribute Naming Conventions #293
  • complete API documentation
  • how well does this fit with separate entity-resource types?

@jaredcnance jaredcnance changed the base branch from master to feat/#293 September 26, 2018 04:54
@jaredcnance jaredcnance force-pushed the feat/#151 branch 2 times, most recently from 945813f to a5786d4 Compare September 26, 2018 05:14

namespace JsonApiDotNetCoreExample.Models
{
public class ArticleTag : Identifiable
Copy link
Contributor

@milosloub milosloub Sep 26, 2018

Choose a reason for hiding this comment

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

Is it necessary to inherit from Identifiable?
This leads to define many to many without this config line:

modelBuilder.Entity<ArticleTag>()
        .HasKey(bc => new { bc.ArticleId, bc.TagId });

Many to many is then identified by Id not by composite key. In this case unique constraint is not available.

Copy link
Contributor

Choose a reason for hiding this comment

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

From a purely necessary standpoint, probably not, but when I took a look at it before, the use of Identifiable is so pervasive in the ability to support the autogeneration of everything, that it was far too much effort to remove for the results.

I am curious here though @jaredcnance, what happens when the Many-To-Many has additional properties beyond just the composite key. For instance, in my Entity/Resource split example, the student/course many-to-many relationship would potentially have additional properties like "grade". From a resource standpoint, it seems like the "courses" for a student would include the join properties, plus the properties of the other side of the relationship, and vice versa.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand as I look closer to entity-resource separation system.
In that case, it should be performed still as composite key for EF core, but also Identifiable for JsonApi purposes.

I didn't try, but maybe somethink like this can solve the problem:

public class ArticleTag : Identifiable
{
        [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
        public override int Id { get => base.Id; set => base.Id = value; }

// other properties
}

I'll try this later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@milosloub it shouldn't be necessary unless you also want the join entity exposed as a json:api resource. I'll remove it for now and see if there are any issues.

when the Many-To-Many has additional properties beyond just the composite key

@roblankey generally, I would think this should be exposed as its own resource, but that is an interesting idea. I'll give it some thought and see how we might go about doing it (although I'm not 100% sure we should). In the current implementation we have an issue where I believe any additional properties on the join entity will actually be lost on updates -- so there's a bit of work left on this front.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jaredcnance that's totally valid. when we start factoring in many-to-many entities with how they map to resources, it can start getting pretty hairy!

Copy link
Contributor

@milosloub milosloub Sep 27, 2018

Choose a reason for hiding this comment

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

After few tests I could make composite key and identifiable works together.
Solution is really define join-entity via standard recommendations of EF Core documentation and then make Id override with DatabaseGenerated attribute (see above).

I tested this on MSSQL, MySql and SQLite. MSSQL and MySql works like a charm, but
SQLite throws "SQLite Error 19: 'NOT NULL constraint failed: ArticleTag .Id".
I think this can be problem to acceptance tests with in-memory SQLite database. Solution is to choose composite key or Id primary key on this database

Edit: it's known issue:
dotnet/efcore#11550

Copy link
Contributor

@roblankey roblankey left a comment

Choose a reason for hiding this comment

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

Really like this Jared. Great work! Regardless of whether additional properties on the Many-to-Many entity cause an issue, this is at least a great first step!

/// <summary>
/// Get the DbSet when the model type is unknown until runtime
/// </summary>
public static IQueryable<object> Set(this DbContext context, Type t)
Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow. this is a really slick way of generalizing the relationship

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't take much credit here. Most of this was done by @jasolko 😝

@@ -10,5 +12,10 @@ public class Article : Identifiable
[HasOne("author")]
public Author Author { get; set; }
public int AuthorId { get; set; }

[NotMapped]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really interesting @jaredcnance. Is the idea here that you include a List to the other side as well as the joining entity? I think that makes sense from the ability to be generic about it. I think I'll update my entity/resource example to do this as well, see how it works there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. In the case of a separate resource class, the NotMapped shouldn't be required. I definitely need to see how well all of this fits when resources and db entities have been separated. I'll add that to the TODO list.


namespace JsonApiDotNetCoreExample.Models
{
public class ArticleTag : Identifiable
Copy link
Contributor

Choose a reason for hiding this comment

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

From a purely necessary standpoint, probably not, but when I took a look at it before, the use of Identifiable is so pervasive in the ability to support the autogeneration of everything, that it was far too much effort to remove for the results.

I am curious here though @jaredcnance, what happens when the Many-To-Many has additional properties beyond just the composite key. For instance, in my Entity/Resource split example, the student/course many-to-many relationship would potentially have additional properties like "grade". From a resource standpoint, it seems like the "courses" for a student would include the join properties, plus the properties of the other side of the relationship, and vice versa.

@@ -1,3 +1,5 @@
.DS_Store
Copy link
Contributor

Choose a reason for hiding this comment

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

Hah. Great catch!

@jaredcnance jaredcnance changed the base branch from feat/#293 to master October 3, 2018 04:07
@jaredcnance jaredcnance closed this Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants