Skip to content

Relationship Data - missing or by design? #218

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
mattgi opened this issue Dec 22, 2017 · 18 comments · Fixed by #284
Closed

Relationship Data - missing or by design? #218

mattgi opened this issue Dec 22, 2017 · 18 comments · Fixed by #284
Milestone

Comments

@mattgi
Copy link

mattgi commented Dec 22, 2017

Should I always see relationship data, regardless of link config or include query?

By this, I mean /items currently produces:

{
    "data": [
        {
            "type": "items",
            "id": "item-de4e78d3-b2ec-48dd-8dd7-6088881a2bfe",
            "attributes": {
                "type": 0,
                "title": "New York",
                "cover": null,
                "slug": null,
                "version": 1
            },
            "relationships": {
                "owner": {
                    "links": {
                        "self": "/items/item-de4e78d3-b2ec-48dd-8dd7-6088881a2bfe/relationships/owner",
                        "related": "/items/item-de4e78d3-b2ec-48dd-8dd7-6088881a2bfe/owner"
                    },
                "createdBy": {},
                "updatedBy": {},
                "deletedBy": {}
                }
            }
        }
    ]
}

If I include owner (/items?include=owner):

{
    "data": [
        {
            "type": "items",
            "id": "item-de4e78d3-b2ec-48dd-8dd7-6088881a2bfe",
            "attributes": {
                "type": 0,
                "title": "New York",
                "cover": null,
                "slug": null,
                "version": 1
            },
            "relationships": {
                "owner": {
                    "links": {
                        "self": "/items/item-de4e78d3-b2ec-48dd-8dd7-6088881a2bfe/relationships/owner",
                        "related": "/items/item-de4e78d3-b2ec-48dd-8dd7-6088881a2bfe/owner"
                    },
                    "data": {
                        "type": "users",
                        "id": "user-b607e7dc-3a9d-4360-8495-dd37c4c99de8"
                    }
                },
                "createdBy": {},
                "updatedBy": {},
                "deletedBy": {}
            }
        }
    ],
    "included": [
        {
            "type": "users",
            "id": "user-b607e7dc-3a9d-4360-8495-dd37c4c99de8",
            "attributes": {
                "email": "[email protected]",
                "firstName": "Matthew",
                "lastName": "Gint2y",
                "slug": "slugger",
                "version": 1
            },
            "relationships": {
                "createdBy": {},
                "updatedBy": {},
                "deletedBy": {}
            }
        }
    ]
}

Even when I am not asking for includes (or if links are turned off), I was expecting the relationships to always have data:

"relationships": {
                "owner": {
                    "data": {
                        "type": "users",
                        "id": "user-b607e7dc-3a9d-4360-8495-dd37c4c99de8"
                    }
                },

Is this expected behavior or am I potentially missing something on my model?

      [HasOne("owner")]
      public virtual User Owner { get; set; }

      public string OwnerId { get; set; }

Based on http://jsonapi.org/format/#document-resource-objects and their { article } snippet, it suggests data should be there.. I must be missing something somewhere?

@jaredcnance
Copy link
Contributor

I think this is an oversight. In the case of a dependent HasOne relationship we can include this easily and I think maybe we should. However, on the independent side of a HasOne (in the case of a 1:(0-1) relationship) we will not include this because it would necessitate a SQL JOIN and would be unexpected behavior.

@jaredcnance
Copy link
Contributor

jaredcnance commented Dec 22, 2017

I think we need to do some setup when we build the ContextGraph so we don't do any extra request-time reflection. That would involve modifying the ContextGraphBuilder to set some HasOneAttribute.IsDependent and maybe even a reference to the RelationshipId PropertyInfo. Then the DocumentBuilder will need to pull out the value. With tests, I would expect it to take me a couple hours. I'd also be happy to work with you if you want to take a stab at it.

@mattgi
Copy link
Author

mattgi commented Dec 22, 2017

Ha, I just deleted my comment.

I was going to just try messing with the DocumentBuilder

Will start to get me used to the code if nothing else.

@mattgi
Copy link
Author

mattgi commented Dec 22, 2017

A very quick hack was to:

  1. copy the source DocumentBuilder into my local codebase,
  2. Inject the copy in startup via services.AddScoped<IDocumentBuilder, JsonBuilder>();
  3. Make this hack to AddRelationships:
private void AddRelationships(DocumentData data, ContextEntity contextEntity, IIdentifiable entity) {
...
  if (RelationshipIsIncluded(r.PublicRelationshipName)) {
...
  } else {
    relationshipData.SingleData = GetSimpleRelationship(entity, r.InternalRelationshipName);
  }
...
  1. Add the corresponding method:
    private Dictionary<string, string> GetSimpleRelationship(object entity, string name) {
      var entityType = entity.GetType();
      var contextEntity = _contextGraph.GetContextEntity(entityType);
      var property = contextEntity.Relationships.First(f => f.InternalRelationshipName == name);
      if (property == null) {
        return null;
      }

      var propertyInfo = entityType.GetProperty(string.Concat(name, "Id"));
      if (propertyInfo == null) {
        return null;
      }

      var id = (string) (propertyInfo.GetValue(entity, null));
      if (id == null) {
        return null;
      }

      return new Dictionary<string, string> {
        {
          "type",
          property.Type.Name
        },
        { "id", id }
      };
    }

It assumes that when you have a relationship named Something on the entity that there will be a prop named SomethingId that stores the PK for that related entity (and that it's a string).

Very hacky, and I probably need to consider more complex models if I run in to them, but it quickly unblocked the clientside dev.

FWIW, I started to go down the rabbit hole of ContextGraph changes but quickly came back up for air before I sunk a day on it. :)

Thanks for pointing me in the right direction for now.

@jaredcnance jaredcnance added this to the v2.2 milestone Jan 3, 2018
@crfloyd
Copy link
Contributor

crfloyd commented May 7, 2018

I currently have the need to include "data" (without attributes) for each relationship by default. From what I could tell, the only thing preventing this is the check in DocumentBuilder.cs in the AddRelationships() method:
if (RelationshipIsIncluded(r.PublicRelationshipName))
removing this check just includes the data for the relationship by default. I am using a document database and not the default EntityFramework style so I do not know if there are any implications to doing this outside my use-case. It would be nice to have the ability to at least configure the framework to allow for the default inclusion of data for resources.

@jaredcnance
Copy link
Contributor

The only challenge I see is this requirement from the spec (let me know if I'm just missing something here):

If an endpoint supports the include parameter and a client supplies it, the server MUST NOT include unrequested resource objects in the included section of the compound document.

Without that check, I think there are conditions under which we would no longer comply with the specification. As a framework we have a strict constraint of complying with the spec by default. However, I 100% believe we should make it simple for applications to deviate as needed.

A possible option is to make RelationshipIsIncluded protected so you can simply protected override bool RelationshipIsIncluded(string relationshipName) => true; but I'm not sure that's the best solution.

@crfloyd
Copy link
Contributor

crfloyd commented May 7, 2018

But isn't that referring to the "included" section of the document response? This is different from the inclusion of the "data" field in the "relationships" section. Below is an example response showing a response with included data. This response returns the included information requested in the "included" section but still includes the "data" at the relationship level. This would always be present regardless of what included information was requested.
image

@jaredcnance
Copy link
Contributor

Ah, yes sorry my mistake. You’re correct.

crfloyd pushed a commit to crfloyd/JsonApiDotNetCore that referenced this issue May 8, 2018
…her than only when "include" for that relationship is set.
crfloyd pushed a commit to crfloyd/JsonApiDotNetCore that referenced this issue May 11, 2018
…n resource relationships with id and type set.
jaredcnance added a commit that referenced this issue May 15, 2018
#218 Defaults data to be included in relationships rather than only w…
jaredcnance added a commit that referenced this issue May 15, 2018
in all requests for the dependent side of the relationship by default
@jaredcnance jaredcnance mentioned this issue May 15, 2018
3 tasks
@wurmrobert
Copy link

wurmrobert commented Jun 9, 2018

Hy.

I'm using Version 2.1.0 and would like to receive the relationships data object an every GET request. Is there any possibility to get only the { type, id } relationship infos without the included object?

some example:

{
  "data": {
    "type": "photos",
    "attributes": {
      "title": "Ember Hamster",
      "src": "http://example.com/images/productivity.png"
    },
    "relationships": {
      "photographer": {
        "data": { "type": "people", "id": "9" }
      }
    }
  }
}

@jaredcnance
Copy link
Contributor

@wurmrobert please try the latest version (2.2.4) that fixes this issue. If that does not work for you, let me know.
https://github.com/json-api-dotnet/JsonApiDotNetCore/releases/tag/v2.2.4

@wurmrobert
Copy link

@jaredcnance i have updated to the latest version (2.2.4) but i still do not get the relationships data object (i only obtain the links object). Do i have to put any options?

@jaredcnance
Copy link
Contributor

jaredcnance commented Jun 9, 2018

@wurmrobert this is working for me running the example app. And no configuration options are required, this is default behavior. As previously mentioned, we will not perform a join to get dependent relationship side of a 1:(0-*) relationship from the independent side. Can you answer a few questions to help me understand your setup:

  • Can you share your models?
  • Can you share your HTTP request?
  • Have you made any customizations?
  • Can you verify that your object actually has an independent relationship? For example, if Article has one Author through Article.AuthorId, make sure the foreign key value is not null.
curl -X GET "http://localhost:5000/api/v1/todo-items/8"
{
  "data": {
    "attributes": { /* ... */ },
    "relationships": {
      "owner": {
        "links": { /* ... */ },
        "data": {
          "type": "people",
          "id": "3"  // <------- TodoItem.OwnerId
        }
      },
      "assignee": { /* ... */ },
      "collection": { /* ... */ },
    },
    "type": "todo-items",
    "id": "8"
  }
}

image

@wurmrobert
Copy link

@jaredcnance thanks a lot for your help!

my models:

public class Customer: Identifiable {
        
        [Attr("first_name")]
        public string FirstName { get; set; }

        [HasMany("devices")]
        public virtual List<Device> Devices { get; set; }
}
public class Device: Identifiable {
        [Attr("mac_address")]
        public string MacAddress { get; set; }

        [HasOne("customer")]
        public virtual Customer Customer { get; set; }
}

curl -X GET "http://localhost:5000/api/v1/devices"
screen shot 2018-06-10 at 00 09 16

With Postico i can see that device has a relation to a customer:
screen shot 2018-06-10 at 00 09 59

@jaredcnance
Copy link
Contributor

jaredcnance commented Jun 10, 2018

Device needs a CustomerId property exposed on your model.

public class Device: Identifiable {
  [Attr("mac_address")]
  public string MacAddress { get; set; }

  [HasOne("customer")]
  public virtual Customer Customer { get; set; }
  public int CustomerId { get; set; } // we need EF to set this property
}

@wurmrobert
Copy link

I already tried to add CustomerId to device.
However, if I add this attribute i get a foreign key violation exception during post a new device:

screen shot 2018-06-10 at 08 20 09

@jaredcnance
Copy link
Contributor

jaredcnance commented Jun 10, 2018

I am unable to reproduce this behavior (we also have tests that run this request). A few other things you might check:

  • Are there any interesting logs produced when this request is executed?
  • Can you share your Startup code where you add JADNC services?
  • Is the a DbSet<Customer> Customers { get; set; } property on your DbContext?
  • Have you done any special mapping of your DbContext (using the ModelBuilder) ?
  • Can you debug your app and see what the incoming Device object looks like in the controller? Has the Device.CustomerId property actually been set?

@wurmrobert
Copy link

wurmrobert commented Jun 11, 2018

Really strange.

  • No there aren´t any interesting logs
  • I don´t do any special mapping with ModelBuilder
  • The Device.CustomerId on the incoming device is not set.

I have put the source to a public repo: https://github.com/wurmrobert/jsonapi-graphql-example

The startup class:

namespace backend {
    public class Startup {
        public Startup (IConfiguration configuration) {
            Configuration = configuration;
        }

        public IConfiguration Configuration { get; }

        public void ConfigureServices (IServiceCollection services) {
            ....
            var connectionString = Configuration["DbContextSettings:ConnectionString"];
            services.AddEntityFrameworkNpgsql ()
                .AddDbContext<MasterdataContext> (options => options.UseNpgsql (connectionString));

            // add jsonapi dotnet core
            services.AddJsonApi<MasterdataContext> (
                opt => opt.Namespace = "api/v1"
            );
            ...
            services.AddMvc ();
        }

        public void Configure (IApplicationBuilder app, IHostingEnvironment env) {
            if (env.IsDevelopment ()) app.UseDeveloperExceptionPage ();
            ...
            app.UseJsonApi ();
            ...
        }
    }
}

@wurmrobert
Copy link

wurmrobert commented Jun 22, 2018

Hy @jaredcnance.

After setting customerId as optional:

public int? CustomerId { get; set; }

the GET request returns all data relationships. But on the POST Method dotnet quits unexpectedly.

jaredcnance pushed a commit that referenced this issue Aug 12, 2018
…hen "include" for that relationship is set.
jaredcnance pushed a commit that referenced this issue Aug 12, 2018
jaredcnance added a commit that referenced this issue Aug 12, 2018
#218 Defaults data to be included in relationships rather than only w…
jaredcnance added a commit that referenced this issue Aug 12, 2018
in all requests for the dependent side of the relationship by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants