Make "data" member of "relationships" optional

[bduisenov]

This is a follow-up enhancement to #282 . After adding links attribute to relationships, data attribute should become optional.

@DeathByTape, @clayreimann proposal from my side is to modify PersistentResource#getRelationshipsWithRelationshipFunction to filter out null relationships retrieved from relationshipFunction, so that when DataStoreTransaction#getRelation returns null it would not be included.

[clayreimann]

@bduisenov I disagree for two reasons. First, filtering null, but not [] create an inconsistency between to-one and to-many relationship types. Second–and more importantly–I think there is value in letting people know that this relationship is empty. And second-and-a-half, less compliant consumers may fail spectacularly if, in some cases, the data member is not present.

That being said, if you feel that there is a strong reason to not pass data when it is empty, then I suppose we can hide this behind a builder flag so that when constructing and Elide service authors can decide if they want this behavior.

[DennisMcWherter]

Sorry for the delay @bduisenov.

Let me make sure I understand correctly (I started writing a response and then realized I misunderstood 😄), you want to ignore any null members and perhaps even [] (to comply with @clayreimann's comment) for relationships?

I don't think it's appropriate to hide the relationships field if there exist relationships on the object (regardless of their values). Since the jsonapi spec states MAY, this is truly an optional field but I think we should present it when we have anything meaningful to display. What I mean is that I would be happy to hide the following:

{
  ...
  "relationships": {}
  ...
}

but would be very hesitant to suggest that we omit something like this, however:

{
  ...
  "relationships": {
    "myToOneRelationship": null
  }
  ...
}

Since that is actually information about the status of an existing relationship that the user has access to. In general, it's probably better for the server to be more explicit on these things than encoding this data implicitly on whether or not it exists. This could become even more problematic for any breaking-changes to the schema (assuming/relying on values that no longer exist, etc.).

My 2cents.

[bduisenov]

Hi @DeathByTape, @clayreimann.

Thanks for your replies.

What I want to have, is an option in Elide, not to even try to fetch relationships data upfront and to give a client only a link instead.

f.ex. with introduction of #282 the response would be:

{
  "relationships": {
     "author": {
      "links": {
        "self": "/articles/1/relationships/author",
        "related": "/articles/1/author"
      },
      "data": { "type": "people", "id": "9" }
    }
  }
}

But I want:

{
  "relationships": {
     "author": {
      "links": {
        "self": "/articles/1/relationships/author",
        "related": "/articles/1/author"
      }
    }
  }
}

instead.

My first idea was to override DataStoreTransaction#getRelation to return either null or empty collection or whatsoever to indicate that data should not be included into relationships. But concept is broken and there should be another, better way how to implement this. Do you have an idea?

Cheers, Babur

[DennisMcWherter]

The goal for this improvement is not performance, is it? Unless you are actually include'ing (in which case we will fetch and display a full relationship object), the amount of work that would have to be done is the same to construct a link or the data object. That is, you would need to fetch the id for the object. Since we're currently relying on Hibernate's proxy behavior, this isn't always very efficient.

We are reworking the entire DataStoreTransaction interface for Elide 3.0. iirc, this is exactly one of the sort of things we would like to better support. @clayreimann @wcekan @aklish we should really move our discussions online here to better involve the community in this discussion. At the very least, we could spread awareness of the changes we plan to make. Specifically, I think we will be moving away from relying on Hibernate's proxy'ing and moving toward an HQL-based interface. Moreover, this will improve our overall design for DataStoreTransaction since we plan on creating (better) DataStore's for non-ORM -based interfaces.

Anyway, do you guys have a good suggested workaround in the meantime? One suggestion (from the perspective of hibernate) is to make sure you are setting your relationships to FetchType.LAZY and the id's may need AccessType.PROPERTY. That may be one thing that helps mitigate this issue.

[bduisenov]

Hi @DeathByTape,

The goal for this improvement is not performance, is it? - yes, it is.

Let's assume I have a complex system where DTO's are constructed by calling multiple populators/converters. For example: ProductDTO might be constructed by calling Price/Images/Description/Classification/Url/Reviews/Promotions/etc. populators. Now, I want to build JSONAPI having Facades with this DTO's.

I've added JPA annotations to DTO's (ugly) together with Elide annotations. Wrote delegators/adaptors for DataStoreTransaction and DataStore to call thisFacades. When I call /products/1 it returns me a product, all good. But, I also have CategoryDTO which has toMany relationship with products. In this case, when I call /category/1 the data attribute would be included into relationships containing id's of all 5000+ products 😄. I don't want to load ProductDTOs just for showing their ids in this case, instead showing a relationship link would be perfect for me.