fix/#445 #446

roblankey · 2018-11-13T02:14:45Z

Closes #445

BUG FIX

reproduce issue in tests
fix issue
bump package version

…rceType. updated DefaultEntityRepository to correctly attach relationship when creating a resource

roblankey · 2018-11-13T02:18:12Z

@milosloub Good evening! This update solves your issue directly on the HasOne example. It adds a withEntityType value to the HasOne attribute that you should use when separating the resource and entity. I added a test to ResourceEntitySeparationExample AddTests that mimics your use case with Student and Department successfully.

@jaredcnance /facepalm. that's what i get for not running tests locally first.

…more than one property with that type

jaredcnance

Thanks for doing this! I suspect this issue also exists with HasMany? This is a great first step. Maybe in the future we can determine this during construction of the ResourceGraph. But, maybe not.

jaredcnance · 2018-11-13T04:04:08Z

src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs

-                if (_context.Entry(relationship.Value).State == EntityState.Detached && _context.EntityIsTracked(relationship.Value) == false)
-                    _context.Entry(relationship.Value).State = EntityState.Unchanged;
+            {
+                if (relationship.Key.GetType() != typeof(HasOneAttribute))


Under what circumstances does this happen?

This is totally just a guard clause since the dictionary key is RelationshipAttribute.

jaredcnance · 2018-11-13T04:13:00Z

src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs

+                else
+                {
+                    if (_context.Entry(relationship.Value).State == EntityState.Detached && _context.EntityIsTracked(relationship.Value) == false)
+                        _context.Entry(relationship.Value).State = EntityState.Unchanged;


Let's refactor this a bit so it's clearer and we don't have separate definitions for how the EF ChangeTracker entry state is modified. The only thing that needs to be in the conditional is how we determine the relatedEntity. so, maybe something like:

IIdentifiable relatedEntity = (hasOne.EntityPropertyName != null) ? entity.GetType().GetProperty(hasOne.EntityPropertyName)?.GetValue(entity) as IIdentifiable : relationship.Value; if(relatedEntity != null && _context.Entry(relatedEntity).State == EntityState.Detached && _context.EntityIsTracked(relatedEntity) == false) { _context.Entry(relatedEntity).State = EntityState.Unchanged; }

jaredcnance · 2018-11-13T05:28:20Z

Also, please go ahead and bump to 3.1.0. I'm going to follow the 3.0.0 release with a 3.1.0-beta as soon as this PR lands.

roblankey · 2018-11-13T12:17:10Z

Hah. Yeah, I was thinking the same thing last night @jaredcnance
The more I mess with the entity/resource separation, the more I want to just update ContextG..., I mean ResourceGraph =), with the ability to track both there.

…rceType. updated DefaultEntityRepository to correctly attach relationship when creating a resource

…more than one property with that type

jaredcnance · 2018-11-14T02:20:04Z

So, after thinking about this a little bit more, I realized that this is probably the de-serializer's responsibility. In order to set these relationships using a pointer, we have to create an instance of the related type. So, what's happening right now (before this PR) is that the ResourceObject is the object that the json payload gets deserialized into directly and we then instantiate an instance of the the related type and assign it to the navigation property. Then the repository detaches it so EF doesn't try to create it again. So, the path in the de-serializer looks like:

SetHasOneNavigationPropertyValue

JsonApiDotNetCore/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs

Lines 264 to 266 in 8570b21

    
           var includedRelationshipObject = GetIncludedRelationship(rio, included, hasOneAttr); 
        
           if (includedRelationshipObject != null) 
        
               hasOneAttr.SetValue(entity, includedRelationshipObject);

which calls into GetIncludedRelationship to create an instance of the related type:

JsonApiDotNetCore/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs

Lines 305 to 308 in 8570b21

    
           private IIdentifiable GetIncludedRelationship(ResourceIdentifierObject relatedResourceIdentifier, List<ResourceObject> includedResources, RelationshipAttribute relationshipAttr) 
        
           { 
        
               // at this point we can be sure the relationshipAttr.Type is IIdentifiable because we were able to successfully build the ResourceGraph 
        
               var relatedInstance = relationshipAttr.Type.New<IIdentifiable>();

So, this is where I think we need to instantiate the correct type. So, maybe something like:

var relatedEntityType = relationshipAttr?.TargetEntityType ?? relationshipAttr.Type;
var relatedInstance = relatedEntityType.New<IIdentifiable>();

and then the HasOne API becomes something like:

[HasOne("department", targetEntity: typeof(Department))]
public DepartmentResource Department { get; set; }

What do you think?

milosloub · 2018-11-14T07:46:26Z

@jaredcnance
Looks good. Just for sure - do you know, if it's possible with this approach to have multi relations pointing to same entity?
For example this:

    public class SomeResource: Identifiable
    {
        [HasOne("parent", targetEntity: typeof(Resource))]
        public SomeResource Parent { get; set; }

        [HasOne("child", targetEntity: typeof(Resource))]
        public SomeResource Child { get; set; }
    }

roblankey · 2018-11-14T12:24:02Z

@jaredcnance
Looks good. Just for sure - do you know, if it's possible with this approach to have multi relations pointing to same entity?
For example this:
    public class SomeResource: Identifiable
    {
        [HasOne("parent", targetEntity: typeof(Resource))]
        public SomeResource Parent { get; set; }

        [HasOne("child", targetEntity: typeof(Resource))]
        public SomeResource Child { get; set; }
    }

That's actually an issue I ran into with the first pass! typeof won't work as the mapping because you could have multiple of the same type! That's why I switched it to the property name. Good catch!

roblankey · 2018-11-14T12:29:55Z

So, after thinking about this a little bit more, I realized that this is probably the de-serializer's responsibility. In order to set these relationships using a pointer, we have to create an instance of the related type. So, what's happening right now (before this PR) is that the ResourceObject is the object that the json payload gets deserialized into directly and we then instantiate an instance of the the related type and assign it to the navigation property. Then the repository detaches it so EF doesn't try to create it again. So, the path in the de-serializer looks like:
SetHasOneNavigationPropertyValue
  [JsonApiDotNetCore/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs](https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/8570b216c7ede3a7b5f9fc7a4912404a14926df5/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs#L264-L266)


    Lines 264 to 266
  in
  [8570b21](/json-api-dotnet/JsonApiDotNetCore/commit/8570b216c7ede3a7b5f9fc7a4912404a14926df5)





    
      
       var includedRelationshipObject = GetIncludedRelationship(rio, included, hasOneAttr); 
    

    
      
       if (includedRelationshipObject != null) 
    

    
      
           hasOneAttr.SetValue(entity, includedRelationshipObject);
which calls into GetIncludedRelationship to create an instance of the related type:
  [JsonApiDotNetCore/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs](https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/8570b216c7ede3a7b5f9fc7a4912404a14926df5/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs#L305-L308)


    Lines 305 to 308
  in
  [8570b21](/json-api-dotnet/JsonApiDotNetCore/commit/8570b216c7ede3a7b5f9fc7a4912404a14926df5)





    
      
       private IIdentifiable GetIncludedRelationship(ResourceIdentifierObject relatedResourceIdentifier, List<ResourceObject> includedResources, RelationshipAttribute relationshipAttr) 
    

    
      
       { 
    

    
      
           // at this point we can be sure the relationshipAttr.Type is IIdentifiable because we were able to successfully build the ResourceGraph 
    

    
      
           var relatedInstance = relationshipAttr.Type.New<IIdentifiable>();
So, this is where I think we need to instantiate the correct type. So, maybe something like:
var relatedEntityType = relationshipAttr?.TargetEntityType ?? relationshipAttr.Type;
var relatedInstance = relatedEntityType.New<IIdentifiable>();
and then the HasOne API becomes something like:
[HasOne("department", targetEntity: typeof(Department))]
public DepartmentResource Department { get; set; }
What do you think?

So I was thinking about this more last night. The problem is with Resource/Entity separation, which means that the Resource isn't necessarily the same thing as the Entity when it gets to the Repository. We would have to expose the IResourceMapper further up.

But then we're already doing the resource mapping within the ResourceService so I started thinking about what we're trying to do with this whole pointers thing and I realized that, since the spec doesn't support creating the relationship entity on a POST, that all we really need to do it grab the ID from the Entity at the resource level, look up the affected relationship properties (that we can identify via the EntityPropertyName of the attribute) and detach them.

The question that I really have is whether or not EF will even care. I didn't think it would update the navigation property attributes by default on a save. I'm going to do some testing outside of the project to confirm that, because if that's the case, we don't need this at all I don't think.

jaredcnance · 2018-11-14T16:47:34Z

@roblankey can you push a repo with your test and I can see what the difference is? But, generally EF will try to create both resources if I do:

var article = new Article { Author = new Author { Id = 1 } };
dbContext.Articles.Add(article);
await dbContext.SaveChangesAsync();

roblankey · 2018-11-14T16:48:29Z

@jaredcnance

Just had an "A-Ha" moment. My original tests were only populating the navigation property identifier. When I populate the the navigation property entity as well, with only that Id, EF does throw that exception.

To that end, what do you think about the DeSerializer not serializing HasOne / HasMany inputs?, instead just utilizing the identifers? The thought process there being that the specification is NOT supposed to allow specification of attributes on the relationship object.

roblankey · 2018-11-14T16:51:33Z

The relevant pieces of the spec:

If a relationship is provided in the relationships member of the resource object, its value MUST be a relationship object with a data member. The value of this key represents the linkage the new resource is to have.

`
Resource Linkage
Resource linkage in a compound document allows a client to link together all of the included resource objects without having to GET any URLs via links.

Resource linkage MUST be represented as one of the following:

null for empty to-one relationships.
an empty array ([]) for empty to-many relationships.
a single resource identifier object for non-empty to-one relationships.
an array of resource identifier objects for non-empty to-many relationships.
`

jaredcnance · 2018-11-14T16:51:43Z

There's a reason we had to do it that way (which I can't remember right now -- will comment back later when I have a better answer and more time). But, how would that work with EF and HasMany relationships since the target resource is the independent side of the relationship (no FKs).

roblankey · 2018-11-14T16:53:52Z

Cool, would be good to know if we ran into other issues. Good question on the HasMany, hadn't thought of that yet. That may make us detach anyway. I'll run some more tests as well.

…o fix/#445

roblankey · 2018-11-18T09:00:35Z

Got the HasMany piece complete! Sorry it took so long!

jaredcnance

Nice work @roblankey! Sorry for the delay. Feel free to ping me on our Gitter channel if you need to nag me a bit. I'm going to approve and merge this in. I'll probably release it as an alpha in the next couple days so you guys can start using it. But I want to spend some more time on this when I'm available before releasing it as a stable release.

roblankey added 4 commits November 12, 2018 19:14

add rider to ignore

e651a3e

update git ignore to ignore all rider

b9923f3

change Type to ResourceType and add EntityType that defaults to Resou…

b395945

…rceType. updated DefaultEntityRepository to correctly attach relationship when creating a resource

bump package version?

649648f

roblankey requested a review from jaredcnance November 13, 2018 02:14

re-change the typing since it was causing issues when the entity had …

a7a22bb

…more than one property with that type

roblankey mentioned this pull request Nov 13, 2018

Can't create resource with relationship in Entity-Resource separation mode #445

Closed

jaredcnance reviewed Nov 13, 2018

View reviewed changes

roblankey added 5 commits November 13, 2018 07:22

add rider to ignore

0938c33

update git ignore to ignore all rider

d513658

change Type to ResourceType and add EntityType that defaults to Resou…

3233869

…rceType. updated DefaultEntityRepository to correctly attach relationship when creating a resource

bump package version?

f1a0cf6

re-change the typing since it was causing issues when the entity had …

6bc8187

…more than one property with that type

roblankey added 3 commits November 18, 2018 03:04

Merge branch 'fix/#445' of github.com:roblankey/JsonApiDotNetCore int…

135853b

…o fix/#445

add entity check to hasmany

5ee3068

fix copy pasta

bf3cec4

jaredcnance approved these changes Nov 29, 2018

View reviewed changes

jaredcnance merged commit e58a48d into json-api-dotnet:master Nov 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix/#445 #446

fix/#445 #446

roblankey commented Nov 13, 2018

roblankey commented Nov 13, 2018

jaredcnance left a comment

jaredcnance Nov 13, 2018

roblankey Nov 13, 2018

jaredcnance Nov 13, 2018

jaredcnance commented Nov 13, 2018

roblankey commented Nov 13, 2018

jaredcnance commented Nov 14, 2018 •

edited

Loading

milosloub commented Nov 14, 2018 •

edited

Loading

roblankey commented Nov 14, 2018

roblankey commented Nov 14, 2018

jaredcnance commented Nov 14, 2018 •

edited

Loading

roblankey commented Nov 14, 2018 •

edited

Loading

roblankey commented Nov 14, 2018

jaredcnance commented Nov 14, 2018

roblankey commented Nov 14, 2018

roblankey commented Nov 18, 2018

jaredcnance left a comment •

edited

Loading

fix/#445 #446

fix/#445 #446

Conversation

roblankey commented Nov 13, 2018

BUG FIX

roblankey commented Nov 13, 2018

jaredcnance left a comment

Choose a reason for hiding this comment

jaredcnance Nov 13, 2018

Choose a reason for hiding this comment

roblankey Nov 13, 2018

Choose a reason for hiding this comment

jaredcnance Nov 13, 2018

Choose a reason for hiding this comment

jaredcnance commented Nov 13, 2018

roblankey commented Nov 13, 2018

jaredcnance commented Nov 14, 2018 • edited Loading

milosloub commented Nov 14, 2018 • edited Loading

roblankey commented Nov 14, 2018

roblankey commented Nov 14, 2018

jaredcnance commented Nov 14, 2018 • edited Loading

roblankey commented Nov 14, 2018 • edited Loading

roblankey commented Nov 14, 2018

jaredcnance commented Nov 14, 2018

roblankey commented Nov 14, 2018

roblankey commented Nov 18, 2018

jaredcnance left a comment • edited Loading

Choose a reason for hiding this comment

jaredcnance commented Nov 14, 2018 •

edited

Loading

milosloub commented Nov 14, 2018 •

edited

Loading

jaredcnance commented Nov 14, 2018 •

edited

Loading

roblankey commented Nov 14, 2018 •

edited

Loading

jaredcnance left a comment •

edited

Loading