[FEATURE] Add a bundle level relationship to entity level resources

Just some starting feedback. I would need more focus to review it more in depth.

1. +++ /dev/null
   @@ -1,57 +0,0 @@
   -
   -  /**
   -   * Validates the JSONAPI parameter names.
   -   *
   -   * @param \Symfony\Component\HttpFoundation\Request $request
   -   *   The request.
   -   *
   -   * @return \Drupal\Core\Access\AccessResult
   -   *   The access result.
   -   */
   -  public function access(Request $request) {
   

   You like killing stuff, aren't you :)

2. +++ b/src/Normalizer/ContentEntityNormalizer.php
   @@ -147,23 +154,18 @@ class ContentEntityNormalizer extends NormalizerBase implements DenormalizerInte
   -    return $fields;
   +    return $bundle_id ?
   +      $fields :
   +      $this->getResourceBundleRelationshipFields($entity, $fields);
   

   Nice idea to extract the method and not do another if, like many other people would do

3. +++ b/src/Relationship.php
   @@ -0,0 +1,135 @@
   + * Class Relationship.
   + *
   + * Use this class to create a relationship in your normalizer without having an entity reference field.
   + *
   + * @package Drupal\jsonapi
   + */
   +class Relationship implements RelationshipInterface {
   

   Nice, a proper value object

4. +++ b/src/Relationship.php
   @@ -0,0 +1,135 @@
   +  public function __construct(ResourceConfigInterface $target_resource_config, $field_name, $cardinality = FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED, array $values = []) {
   ...
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setValues(array $values = []) {
   +    $this->values = $values;
   +  }
   +
   

   Are there usecases for this object to be muteable or would it be usually enough to just have the constructor and that's it?

5. +++ b/tests/src/Kernel/Normalizer/DocumentRootNormalizerTest.php
   @@ -277,6 +292,61 @@ class DocumentRootNormalizerTest extends KernelTestBase {
   +    $route = $this->prophesize(Route::class);
   +    $route->getPath()->willReturn('/node_type/{node_type}');
   

   One thing I try to avoid is to mock value objects. They don't have any kind of behaviour, but are rather just a data store, in which case mocking that is more effort than it has to be.

I like where this is going. I think it's certainly a requirement (or a "very nice to have") to provide generic entity level resources in addition to a bundle specific resource.

Regarding this patch specifically, I'm a bit uncomfortable with creating a resource-bundle "field" internally. This doesn't feel like the right abstraction. Already, the field idea is leading to some errors for me. When I follow the generated link to /api/node/1/resource-bundle?_format=api_json, I get a PHP Fatal error coming out of core about not being able to find the 'resource-bundle' field. I know we could code around this to prevent the error, but that feels a bit brittle.

Generally, it seems more appropriate to frame this in terms of whether to include additional attributes than it does to frame it in terms of a different resource altogether, as is implied by a relationship.

Perhaps a more semantically appropriate approach would be to use a custom query parameter and/or custom JSONAPI member field. We might provide a custom parameter like _bundleInclude=true and a custom top-level member like _bundleAttributes for all the fields that are bundle specific.

This will also sidestep the possibly-awkward HTTP semantics surrounding something like PATCH /node/1/resource-bundle and GET /node/1/resource-bundle not returning a collection.

Edit: fixed custom attribute name per the JSON API specification

Edit 2: If we do decide to stick with 'resource-bundle' relationships, I'd like to tie it in with this: https://www.drupal.org/node/2753803#comment-11330169

@gabesullice I understand that your concerns are around GET /node/1/resource-bundle and PATCH /node/1/resource-bundle. For those I would say that we can detect those calls and return an error along the lines of Use /node/article/1 instead.

Fixing the links/endpoints makes certainly makes me more comfortable with this proposal, although I'm still not convinced that the semantics of a relationship are ideal. Perhaps we should look at this JSON API issue for some guidance/ideas: Multiple/Hierarchical Types. That issue seems to be addressing our needs pretty closely.

+++ b/src/Normalizer/ContentEntityNormalizer.php
@@ -193,4 +195,36 @@ class ContentEntityNormalizer extends NormalizerBase implements DenormalizerInte
+    $field_name = 'bundle-resource';
+    $fields[$field_name] = new Relationship($target_resource_config, $field_name, 1, [
+      'target_id' => $entity->id(),
+    ]);
+    $fields[$field_name]->setHostEntity($entity);

This is what I was thinking of when I said we might be making something a bit brittle. We're creating a somewhat "magical" field here since 'bundle-resource' is not a true field. I'm worried we're going to end up carving out lots of little exceptions because of this. I think you may have already noticed this as well:

Note that since this bundle-resource is not a field trying to traverse it with the query builder to add filters will throw an error. We probably want to catch that case and throw an appropriate exception.

---

I want to refactor the relationships so all the relationships implement the RelationshipInterface. Including the EntityReference relationships. When that happens we'll have a consistent way to generate the links that we need.

Yes please! :) See #2753803: [FEATURE] Relationships should be dynamically registerable.

After digesting the JSON API issue a bit more. I think the "meta" member may be perfect for this. We can expose meta information about which attributes are entity fields and what are bundle fields without exposing the entity-bundle implementation in Drupal. What do you think of this:

GET /api/node
{
  "data": [
    {
      "type": "article"
      "id": 1,
      "attributes": {
        "created": 1234678,
        "updated": 1234678,
        "body": "This is some body text."
      }
      "relationships": {
        "uid": {
          "data": { "type": "user", "id": 3 }
        },
        "author": {
          "data": { "type": "user", "id": 4 }
        }
      }
      "meta": {
        "labels": ["node", "article"]
        "labelKeys": {
          "node": {
            "attributes": ["created", "updated"],
            "relationships": ["uid"],
          },
          "article": {
            "attributes": ["created", "updated", "body"],
            "relationships": ["uid", "author"],
          }
        }
      }
    }, {
      "type": "universityApplication"
      "id": 2,
      "attributes": {
        "created": 1234678,
        "updated": 1234678,
        "essay": "This is an essay."
      }
      "relationships": {
        "uid": {
          "data": { "type": "user", "id": 5 }
        },
        "applicant": {
          "data": { "type": "user", "id": 6 }
        }
        "cv": {
          "data": { "type": "file", "id": 7 }
        }
      }
      "meta": {
        "labels": ["node", "universityApplication"]
        "labelKeys": {
          "node": {
            "attributes": ["created", "updated"],
            "relationships": ["uid"],
          },
          "article": {
            "attributes": ["created", "updated", "essay"],
            "relationships": ["uid", "applicant", "cv"],
          }
        }
      }
    }
  ]
}

By exposing our type information in metadata, we can sidestep any custom extensions, hide the entity-bundle implementation details, and avoid any confusion about a bundle-resource being a field or separate object in the system.

I really hate to keep being a stick in the mud, but the entity-bundle relationship is so prevalent in Drupal, our decision here could end up being a big source of developer pain. Better to get it right this time than to have to worry about BC later.

I've updated the issue summary a small bit. I think it would be best for us to discuss in a Google Hangout, since I'm afraid we're both being misunderstood in writing.

We are not building support for filters and sorts in the included attributes. You cannot do ?filter[resource-bundle.category]=foo. Anything that includes the dot notation is unsupported for computed properties because EntityQuery doesn't know about those.

That is a bit sad, because on the pure SQL level, this is kinda possible. I guess we can iterate later here.

In general for me it seems to be okay that those computed relationships have a different behaviour by default, as long we make that clear as part of the documentation. For example can we include that something into the jsonschema one day? Maybe as a special label.

I've been mulling over the conversation that @e0ipso and I had a couple days ago. I had not considered some of the points that @e0ipso brought up and I'm glad we had an opportunity to discuss those concerns.

I would actually flip some of the agreement/disagreements around.

Disagreement

It should not depend on the bundle of the entity. That implies that we cannot have bundle fields in the entity type level resources.

I am of the opinion that at the /node/1 URL, we must be able to satisfy all the attribute properties of being a node, but we should not be precluded from provided attributes specific to the actual entity it represents, meaning I'm fine with including bundle level fields. This would be akin to NodeInterface.

I recognize, however, that the JSON Specification is remains silent on the issue of hierarchical and composed types (if it is not, I would much appreciate a link to the relevant documentation. This should say either say that we CAN or CANNOT provide a superset of attributes as long as we satisfy the node subset).

@e0ipso is concerned that my proposal in #13 approaches the level of a custom extension. I do not believe it does, since it does not break implementations or require additional business logic.

Explicit support for hierarchical or composed types seems to be a missing piece of the JSON API specification. Perhaps this is an opportunity for us to contribute back. A proposal for this issue was supposedly scheduled for December of 2015, but I have not seen any follow up to it.

While the JSON API specification is agnostic to the issue, the JSON Schema specification does have a mechanism to support hierarchical/composed types: via the "anyOf", "allOf" and "oneOf" schema validations. See http://json-schema.org/latest/json-schema-validation.html#anchor75

This is very similar in spirit to my proposal.

Agreement
It is useful to provide entity and bundle level URLs, especially for administrative/migration purposes.

Linking from /node/1 to /node/article/1 (and /node/2 to /node/page/2) is where we start to disagree.

Perhaps we agree more than we think. I have no issue with linking to a bundle level resource. This could validly be done under the "links" member.

"links": {
  "bundleResource": {
    "href": "http://example.com/api/node/article/1",
  }
}

Summary
The heart of my disagreement is simply with treating a bundle as a separate resource, be it through a relationship or a computed field. My fear is that this will be non-intuitive to Drupal developers and deceptive to those unfamiliar with Drupal's node-bundle concept.

The root problem we're addressing is the problem of hierarchical types. We can solve this cleanly in the abstract. I feel we can do so in a way that does not force the consumer to realize the node-bundle concept at all.

By addressing the bundle concept without treating it as an inherited type, we are creating many unsolved issues for ourselves. This thread already has some examples. See #2 (QueryBuilder will throw an error), #9 (need to throw errors for PATCH|DELETE /node/1/resource-bundle), and #16 (sorting/filtering). Some of the same problems with sorting/filtering also mean that sparse field sets become an issue as well.

Issue summary updated