Allow to include a total count of items to collection responses

I doubt this request will be accepted since maintainers considering a removal of the feature

I'm not vehemently opposed to this proposal.

My opposition to the total count was entirely centered on the fact that framing the number as a "total count" is misleading and would become a source of bug reports.

However, I think I see a compromise position that I never saw before and now it seems glaringly obvious...

What if we rename /meta/count to /meta/dataMax?

I think that rename would express that the given number is an upper bound on the number of resources in a collection, not a total while providing the same information as before. I think we would need a BC-layer for the change, but the event that you've introduced could be the perfect place for an "opt-in". WDYT @BR0kEN?

Oh, and as for the patch, I'm very happy about moving to an explicit API that uses the ResourceTypeBuildEvent instead of using the ResourceTypeRepository hack :)

I'd be a +1 towards supporting something like this as opposed to DI-ing or relying on JSON:API Extras.

Never thought that count is an inaccurate word for expression the total number of items.

When you have access controls and you request /jsonapi/node/article?page[limit]=10 you may only get 5 resource objects in the response. Similarly, the total number of articles on your site may be 500, but if you actually request every page of the articles collection and count the resource objects in every response, you may only get 250 or even none at all. Thus 500 is an upper bound on the number of resource objects you might receive but not a total count of every item in the collection.

If I'm interpreting @gabesullice's point correctly is that "count" is potentially misleading for people who may not be fully ingrained through the various discussion threads (https://www.drupal.org/project/drupal/issues/2942426, https://www.drupal.org/project/jsonapi/issues/2932625 and https://www.drupal.org/project/drupal/issues/2942426 for examples) or RTFMing the pagination documentation.

That is someone may assume that "count" is a reliable max as opposed to the theoretical amount of items available (assuming all access checks have passed). Since I don't imagine https://www.drupal.org/project/drupal/issues/2979294 will be seeing love any time soon I think the approach detailed here is sound beyond some semantic naming. I'll bring this up on the API-First call to try and get some more opinions.

Hi BR0kEN,

I love this change, code seems good. Could you change count to dataMax and then we should be ok.

Also I think we should add a follow up issue to allow settings this value in the ResourceTypeBuildEvent so we expose it in an easy way.

Still getting the hang on when a changerecord is required, but since this is all internal i assume it is not needed. We'll see :)

Sure, yeah, being constistent in the naming is a good idea.

The thing about using count is what @gabesullice already mentioned. Even though it is extremely silly and weird, as soon as any resources are not allowed for the user doing the request there can be gaps in the amount of resources. The fact is you could request, get a count of 1000 and only get 1 result in your first page or even in everything.

Personally i think this is the weirdest thing ever in jsonapi, but unfortunately this is the reality.

If we name it count, people will assume a count of objects, this will bring us problems later. Hopefully we can fix this in the future, and then we could use count. This will take a while though.

totalCount sounds better to me than dataMax.

Hi @BR0kEN, i'll have a focussed look at this friday.

I actually like the count naming, but it does mean some issues there where changes have been made already. What would happen right now to sites where TRUE is the return value there through customization, if this means requests break i think we might have a problem getting this in.

Also the patch is currently targetting 9.2.x, think this should be 9.3.x

1. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -1038,7 +1038,7 @@ protected function respondWithCollection(ResourceObjectData $primary_data, Data
   -      $link_context['total_count'] = $meta['count'] = $primary_data->getTotalCount();
   +      $link_context['total_count'] = $meta[$resource_type->getTotalCountMetaName()] = $primary_data->getTotalCount();
   

   This change is tricky. If in installations or contrib includeCount is enabled this would then change the name and break those sites. Not entirely sure how we should handle this, and if we can do this. This will at least need some explaination in an changerecord to communicatie this change.

   Really do think we should investigate supporting the case where this would return true through a site/contrib change and how that would "break".

2. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -249,14 +258,26 @@ public function isFieldEnabled($field_name) {
   +  public function includeCount(): bool {
   

   I wouldn't change this signature in the patch.

3. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -249,14 +258,26 @@ public function isFieldEnabled($field_name) {
   +  public function getTotalCountMetaName(): ?string {
   

   I like we can do this now.

4. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -338,8 +359,11 @@ public function isVersionable() {
   +  public function __construct($entity_type_id, $bundle, $deserialization_target_class, $internal = FALSE, $is_locatable = TRUE, $is_mutable = TRUE, $is_versionable = FALSE, array $fields = [], string $total_count_meta_name = NULL) {
   

   This will either break #3105318: Add a public API for aliasing resource type names or be broken after that is merged. Something to look out for.

5. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -167,7 +169,8 @@ protected function createResourceType(EntityTypeInterface $entity_type, $bundle)
   diff --git a/core/modules/jsonapi/tests/modules/jsonapi_test_collection_count/jsonapi_test_collection_count.info.yml b/core/modules/jsonapi/tests/modules/jsonapi_test_collection_count/jsonapi_test_collection_count.info.yml
   

   This is the test that kinda simulated how in contrib things are handled. Mostly jsonapi_extras though, which i also maintain.

6. +++ b/core/modules/jsonapi/tests/modules/jsonapi_test_resource_type_building/src/EventSubscriber/ResourceTypeBuildEventSubscriber.php
   @@ -26,6 +27,17 @@ public static function getSubscribedEvents() {
   diff --git a/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php
   
   +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -391,12 +391,6 @@ public function testRead() {
   -    $collection_output = Json::decode($this->drupalGet('/jsonapi/node/article'));
   
   @@ -515,6 +509,48 @@ public function testRead() {
   +    $get_data = function () {
   ...
   +      $this->assertSession()->statusCodeEquals(200);
   

   A get_data function that also asserts status code? Seems inconsistent naming. I think just doing a decode as in other parts of this testfile is better.

Thanks for the work :)

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php
@@ -515,6 +515,45 @@ public function testRead() {
+    $get_data = function (): array {
+      return Json::decode($this->drupalGet('/jsonapi/node/article'));
+    };

I would really prefer not using this kinda function. Perhaps we can make a separate issue where we introduce a few helper methods that can do smart things like this.

Hmm, technically this is changing to a public API if it is included in the event.

Think it will help if the summary is updated according to the current solution. I've asked around if another maintainer of jsonapi will have a look also.

1. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -249,14 +258,34 @@ public function isFieldEnabled($field_name) {
   +  /**
   +   * Returns the name of a `meta` member that holds a total count of items.
   +   *
   +   * @return string|null
   +   *   The name of a `meta` member that holds a total count of items or `null`
   +   *   if the member should not be present.
   +   */
   +  public function getTotalCountMetaName(): ?string {
   +    // If the `totalCountMetaName` is `null` and `includeCount()` returns
   +    // `true`, this means we deal with the overridden `includeCount()` and
   +    // should return the former name for the meta member containing the
   +    // total count.
   +    if ($this->totalCountMetaName === NULL && $this->includeCount()) {
   +      return 'count';
   +    }
   

   Let's add isCollectionSizeMemberEnabled() and deprecate the includeCount() method. Then we can add an assertion that $this->totalCountMetaName !== NULL in here. That will allow us to do return $this->totalCountMetaName ?? 'count'. Nullable types are cool, but IMO they're easy to mishandle. Better to always return a string.

2. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeBuildEvent.php
   @@ -139,4 +150,26 @@ public function disableField(ResourceTypeField $field) {
   +  /**
   +   * Sets the name of a `meta` member that holds a total count of items.
   +   *
   +   * @param string|null $name
   +   *   The name of a `meta` member that holds a total count of items.
   +   *   Excludes the member in case of `null`.
   +   */
   +  public function setTotalCountMetaName(?string $name): void {
   +    $this->totalCountMetaName = $name;
   +  }
   

   I think this method should document how and why this member's value can be inaccurate.

   Maybe I'm being obstinate... but let me offer this compromise: can we replace all the "total count name" keywords with the phrase "collection size member name". E.g. s/setTotalCountMetaName/setCollectionSizeMemberName and s/$total_count_meta_name/$collection_size_member_name.

   "Total count" and "count" have always implied a precise, discrete, and true number of resources in the collection which we cannot guarantee for weird reasons. "Size" is still that way, but a little closer to the idea of "maximum" and "limit".

   Since this is a new API, we can do this: setCollectionSizeMemberName(string $name = 'collectionSize') at the same time as we do this: getCollectionSizeMemberName() { return $this->collectionCountMetaName ?? 'count'; }. Thus anyone still overriding ResourceType::includeCount() to be TRUE will still see count in their HTTP API. Users who upgrade (like jsonapi_extras) can call setCollectionSizeMemberName('count') to preserve BC. Finally, new users can call setCollectionSizeMemberName() without an argument and will get 'collectionSize'.

Thanks for the ongoing effort you are putting into this @BR0kEN :)

1. What do you think about the solution without isCollectionSizeMemberEnabled()? The idea is to have no repetitive logic and only use getCollectionSizeMemberName() for determining whether to include a collection size member. Returns NULL - exclude, a string - include (sort of a boolean without an extra method so less to deal with). Later on we'll delete the includeCount() and its logic become return $this->collectionSizeMemberName;.

   To be honest I rather have a separate method to provide the check if it is enabled. This will make comparison explicit and the code more readable.

2. > if ($collection_size_member_name = $resource_type->getCollectionSizeMemberName())

   We also need to be carefull for loosely typed comparisons, and that is way easier and less error prone if we have that method.

3. Is it safe to delete these methods? I find them confusing as the value is provided only when the includeCount() returns true (never in standard jsonapi configuration).

   We do need to do the deprecation dance there I think. I don't see much usage in contrib, but perhaps when building custom resources this might come up. For example here in jsonapi_search_api.

4. Another question: in case the methods stay, can we run the count query regardless of the meta member inclusion and always provide the collection size? Perhaps it should become the Drupal\jsonapi\JsonApiResource\Data::__construct() argument.

   Why would we run the count query if the data is not needed, that seems wastefull to do the work for something we can know we do or do not need.

5. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -515,6 +509,61 @@ public function testRead() {
   +  public function testCollectionSizeMember(): void {
   

   Perhaps this should enclude a test for the case of providing an empty string as name. That would be invalid and we should test that case.

6. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -515,6 +509,61 @@ public function testRead() {
   +  public function testCollectionSizeMember(): void {
   

   Perhaps this should enclude a test for the case of providing an empty string as name. That would be rather invalid.

1+2:

I would actually think it would do the following:

return $this->getCollectionSizeMemberName() !== NULL && !empty(trim($this->getCollectionSizeMemberName());

In an if this would mean:

if ($this->isCollectionSizeMemberEnabled()){

Which i think is way better than:

if ($collection_size_member_name = $resource_type->getCollectionSizeMemberName()){

Mostly because of the loose type comparison and I rather be expicit. Technically the specification is even more specific in what would be allowed there. But lets not go there :)

Opened a merge request to 9.3.x with patch from #50.

Still needs work though. :)

I've revisited this issue and concluded my comment in #54 is a bit overly defensive. I've only added a small test for the empty string and rebased for you.

Changes looking good, setting RTBC. Adding tag for CR

This still needs a change record, setting to Needs Work for that,

Not sure if it's OK to RTBC again, i've added a proper changerecord.

Also; added credits for @gabesullic and @JordanDukart for research and work on this issue.

hiding patches because there's an MR here

#38.2 seems to be still unaddressed.

Should we at least document the potential security concerns this could introduce on sites, given that the count bypasses Entity::access? There are many cases where revealing that something exists is considered as an access bypass.

Maybe I'm being obstinate... but let me offer this compromise: can we replace all the "total count name" keywords with the phrase "collection size member name". E.g. s/setTotalCountMetaName/setCollectionSizeMemberName and s/$total_count_meta_name/$collection_size_member_name.

"Total count" and "count" have always implied a precise, discrete, and true number of resources in the collection which we cannot guarantee for weird reasons. "Size" is still that way, but a little closer to the idea of "maximum" and "limit".

Since this is a new API, we can do this: setCollectionSizeMemberName(string $name = 'collectionSize') at the same time as we do this: getCollectionSizeMemberName() { return $this->collectionCountMetaName ?? 'count'; }. Thus anyone still overriding ResourceType::includeCount() to be TRUE will still see count in their HTTP API. Users who upgrade (like jsonapi_extras) can call setCollectionSizeMemberName('count') to preserve BC. Finally, new users can call setCollectionSizeMemberName() without an argument and will get 'collectionSize'.

@lauriii, I think that comment was addressed. The changes in the naming of the variables was applied as suggested by @gabesullice.

Should we at least document the potential security concerns this could introduce on sites, given that the count bypasses Entity::access? There are many cases where revealing that something exists is considered as an access bypass.

This is no new information that is supplied. When you currently query jsonapi the access check will be done AFTER collecting the entities for a page on the index endpoint. This means if you paginate by 10 you could have only 8 actual results (or actually 0 unfortunately). So you know that there are 2 entitites that are filtered out by the accesscheck. So this means this issue does not expose more information than it did before, since you can just count the amount of entities that are not shown in the index endpoints and get the same information.

Setting back to RTBC since I think the issues raised by @laurii have been adressed and there is no new security implications.

@lauriii, I think that comment was addressed. The changes in the naming of the variables was applied as suggested by @gabesullice.

The first paragraph asked for additional documentation which was the part I thought wasn't addressed:

I think this method should document how and why this member's value can be inaccurate.

This is no new information that is supplied. When you currently query jsonapi the access check will be done AFTER collecting the entities for a page on the index endpoint. This means if you paginate by 10 you could have only 8 actual results (or actually 0 unfortunately). So you know that there are 2 entitites that are filtered out by the accesscheck. So this means this issue does not expose more information than it did before, since you can just count the amount of entities that are not shown in the index endpoints and get the same information.

Where is this information available? I wasn't aware that JSON:API exposed that information anywhere.

Hmm, i just had a look at the documentation and found:

Filters and Access Control

There is mention of access checks being done after the fact and that has those implications. But i cant seem to find an explicit mention in the docs right now regarding the lists that could have gaps.

I've also gone through security considerations but haven't really found an explicit mention.

I'd be happy to work on improving the documentation, but perhaps that could be in a child/followup?

I've switched this to 10.0.x because it needs to be applied there first, can we get a 9.5.x version too,

@bbrala, thanks for adding the changed record. It doesn't look like that has been reviewed. Can someone do that?

Rerolled patch against 10.0.x
Attached rerolled diff

Thanks for the reroll! Seems the reroll didn't go as planned though unfortunately.

Rerolled patch against 10.0.x
Attached rerolled diff

Great, thanks! Unfortunately there is a styleissue that needs to be fixed in ResourseType.php

We also need a review on the CR I wrote.

Rerolled patch against 10.0.x, Addressed #73
Attached interdiff

Rerolled patch against #74
Attached interdiff

Rerolled patch against #76
Attached interdiff

I've rerolled to 10.0.x, there were no conflics which is lovely. Only made a small change to the deprecation message. I think this change was minimal so I still stand by my RTBC.

We still need a review on the changerecord though as mentioned in #69 by @quietone

Review for the CR only:

1. The new method Drupal\jsonapi\ResourceType\ResourceTypeBuildEvent::getCollectionSizeMemberName() is added to the public API, only not in the CR.
2. "See Drupal\jsonapi_test_resource_type_building\EventSubscriber\ResourceTypeBuildEventSubscriber for an example of how to alias a resource type name."
   Can we add a link.
3. The new method Drupal\jsonapi\ResourceType\ResourceType::getCollectionSizeMemberName() is added to the public API, only not in the CR.

For the MR: I do not think we can remove public methods without deprecating them first.

For the MR: I do not think we can remove public methods without deprecating them first.

This class, like most of all of json:api modules', is marked internal so this is not strictly a BC break.

This class, like most of all of json:api modules', is marked internal so this is not strictly a BC break.

Good point. Than they are not part of the public API and they can be removed without deprecating them first.

#81.1: From the changerecord: "The JSON:API ResourceTypeBuildEvent now has a new method: setCollectionSizeMemberName(string $name = NULL). Subscribers to that event can now enable the collection count and set the member name used as key in the meta data of the lists."

#81.2: We could add a link, but then it needs to be merged first. I'll promise to add that :)

#81.3: I've added the method to the CR.

#81.MR: When doing a codesearch is seems to see no usage that will break. The code will check the resources for an implementation and use that if it exist. So it does seem fine.

@bbrala: Thank you for your reply.

All my points have been addressed and the CR is RTBC for me.

I've added some questions to MR.

I think we might need another CR to detail all of the code changes. The current CR focuses on the new stuff which is great but I think we should have CR that covers the API changes and what someone who has used the old API is supposed to do.

Ok, went through the feedback and think i addressed all of it. More BC code, a new BC test, and a new change record.

:)

phpstan & phpcs are not yet happy.

Oops, thanks.

Hmm, that is really unfortunate. So i cannot use a contructor promoted property because phpstan doesn't want that? :(

Removed the property promotion since that means a snakecase class property, that doesn't fly in drupal afaik :(

Also added an ignore for phpstan for a anonimous testclass.

I've rebased the MR onto 10.1.x-dev since it was no longer mergable.

Cleaned up the rebase, there were smoe release commits in there for some reason. Don't know why. Fixed by doing an interactive rebase on itself and dropping the extra commits. Diff from 10.1.x now looks clean <3

There is a merge conflict for core/phpstan-baseline.neon

Fixed phpstan error and rebased while im at it.

MR 1308 had a CI failure.

Did not test or review.

Lets try again, updated to 11.x and fixed tests <3

Can the issue summary be updated with a clear proposed solution?

Deprecations appear right and trying to test locally
Applied the MR
Created some articles
Went to /jsonapi/node/article
But not noticing any difference with or without the MR. Any guidance on how to test would be appreciated.

Ping me or I'll keep an eye on this to come back around and prioritize a faster review so it doesn't sit.

Are you sure the meta key name has to be `total_count`?
According to the JSONAPI example https://jsonapi.org/examples/#pagination there is a reference to `totalPages`.

The spec says:

Note: Putting a property like "totalPages" in "meta" can be a convenient way to indicate to clients the total number of pages in a collection (as opposed to the "last" link, which simply gives the URI of the last page). However, all "meta" values are implementation-specific, so you can call this member whatever you like ("total", "count", etc.) or not use it at all.