While trying to finally close #2932625: Fix query count after query builder refactor, I started to look into the underlying functionality. I did not realize that we were doing anything to count queries. In fact, I think I can remember early discussions where we explicitly decided that providing counts was an anti-pattern and would encourage broken client implementations (but I could also just be dreaming that).

As far as I can tell, we're using using ResourceType::includeCount() to determine if we should run a count entity query, then adding that count to the EntityCollection which would only get serialized if includeCount() returned TRUE.

However, the current implementation of includeCount only every returns FALSE!:

public function includeCount() {
  // By default, do not return counts in collection queries.
  return FALSE;
}

This means we're supporting a lot of code paths that can never be executed unless JSON API Extras has overridden the resource type. I'm okay with that, if it's for a really good reason™️.

However, in this case, I think we're really doing a disservice to our consumers.

The only reason I can think of for including a page count is to try to guess the number of requests to make on the client side, rather than relying on the next links already in the document. It encourages clients to manually construct links and it can potentially really trip up developers when any kind of access control is involved.

This unnecessarily forces us to maintain BC for our URL structure, which "in theory", shouldn't be necessary (for discussion elsewhere ;) ). Worst of all, It promises something we can't deliver on. It's bad DX.

I believe we ought to remove this feature before we put JSON API in core and that we should consider how we can best provide resources for developers to grok best-practices for pagination and making use of all the link information we provide, rather than leading them astray.

To be clear, I don't want this to be construed as a blocker to core inclusion. I just think it's we should have the discussion sooner rather than later and the longer we wait, the less freedom we have.

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

gabesullice
Primary language English
commented

Coming from #2932625-17: Fix query count after query builder refactor. If we do decide that we should continue to support this, we need to add explicit test coverage.

gabesullice’s picture

gabesullice
Primary language English
commented
Issue summary: View changes

I believe we ought to remove this feature before we put JSON API in core

FTR, I don't think this is a blocker to core inclusion and is absolutely not critical. It's just a discussion :)

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I am not opposed to removing this feature and move it completely to JSON API Extras. However adding the necessary extensibility to allow that may introduce more code paths than the ones that we remove. On top of that we will have to worry about other contribs using those extensibility outlets.

We are not doing much to allow this feature inside of jsonapi, right?

unless JSON API Extras has overridden the resource type.

I can expect someone decorating the resource type repository service to toggle that on without needing JSON API extras. However there is no documentation on how to do that or the fact that you can do it. Maybe worth mentioning in the new Pagination section of the docs?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +blocker
Related issues: +#2950744: Move relevant test coverage from jsonapi_extras to jsonapi

This is a blocker for #2950744: Move relevant test coverage from jsonapi_extras to jsonapi, which blocks #2931785: The path for JSON API to core.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

@Wim Leers I don't think we should be blocking such an important issue #2931785: The path for JSON API to core on this. My take is, we should continue supporting this until we reach agreement to drop it. I wouldn't want to put everything on hold for this discussion.

FWIW I don't think we should drop the feature, I think it's very important for JSON API to have it. However I'm open to having a discussion.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Sure … but then the question is: how do we test it?

As @gabesullice explained at #2932625-17: Fix query count after query builder refactor:

[…] If we find that it really is a feature we want to support, we can add tests for explicit support. Right now, most of the code related to counts is in an inaccessible code path without JSON API Extras.

And therefore #2950744: Move relevant test coverage from jsonapi_extras to jsonapi is blocked on this.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

most of the code related to counts is in an inaccessible code path without JSON API Extras.

That is not entirely correct. See #4:

I can expect someone decorating the resource type repository service to toggle that on without needing JSON API extras.

Sure … but then the question is: how do we test it?

I will take that one. As stated in #4 it only involves decorating the resource type repository. Since it's doable with the codebase as-is, I believe we can un-postpone it.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I can expect someone decorating the resource type repository service to toggle that on without needing JSON API extras.

Right, I agree, and came to the same conclusion in #2950744: Move relevant test coverage from jsonapi_extras to jsonapi:

The only way to test this in jsonapi is by duplicating some of jsonapi_extras into jsonapi, as a test-only module, so that we can then move the relevant test coverage from \Drupal\Tests\jsonapi_extras\Functional\JsonExtrasApiFunctionalTest::testRead() into jsonapi:

    // 2. Make sure the count is included in collections. This also tests the
    // overridden paths.
    $output = Json::decode($this->drupalGet('/api/articles'));
    $this->assertSame($num_articles, (int) $output['meta']['count']);
    $this->assertSession()->statusCodeEquals(200);

I agree that this is the most pragmatic approach, if we still feel that we should keep this feature. I just wanted to get an update in this issue if in the past month (#4 was Feb 7, #5 was Mar 7), thoughts had shifted. If everybody agrees we don't need this feature, then it'd be less work!

I will take that one.

Do you mean you want to work on that test coverage? (In #2950744: Move relevant test coverage from jsonapi_extras to jsonapi.)

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I, for one, think we need this feature. I use it fairly often. Specially around consuming a paginated list of elements in a performant way.

gabesullice’s picture

gabesullice
Primary language English
commented

Specially around consuming a paginated list of elements in a performant way.

@e0ipso, can you explain? I'm assuming that you're creating subrequests by "building" URLs for subsequent pages, is that the case?

If so, I'd love to chat about how to do that differently without losing any performance :) I'm almost certain that it's possible (but maybe you'll prove me wrong).

Then, if we can make it work, would you consider dropping this feature? I truly do believe it's an anti-pattern and that it's not a necessary feature, it just a matter of demonstrating a "better" approach.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

We can meet. Will that proposal involve a browser? Because non browser scenarios is a must atm.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Based on #11, I think it's better we all hold off working on testing the count stuff in jsonapi until after @gabesullice and @e0ipso have had that meeting :)

gabesullice’s picture

gabesullice
Primary language English
commented

In our discussion, we agreed that it's necessary to understand pagination information in advance so that one can send parallel requests.

My understanding was that: solely relying on a "next" link means one can't rely on the next link if you need to send parallel requests. I.e. if you don't know how many pages to send, you risk "overshooting" the last page. Our call ended before we could really settle on anything.

We just had a chat, which I'll summarize below.

e0ipso [9:59 AM] @gabesullice did we agree on leaving the count as-is in JSON API, given that the Subrequests approach is not actionable atm?

gabesullice [10:51 AM] @e0ipso, what if we made a compromise? What if we drop a total count of resources and instead add a "last" link to the top-level links member? We can easily calculate this. It's not a "lie" because we'll always return X number of pages, it's just that some pages may be smaller than the limit. Then we're not putting anything under "meta" either and if you need a "number" for your designs, just do the reverse calculation (number of pages * limit) then you' have to consciously understand that it's not accurate

e0ipso [10:53 AM] @gabesullice if we are calculating the count we should return it. it is a useful aggregation.
the counting is a two-fold feature:
- Pagination
- Count

gabesullice [10:58 AM] Ah man, that was my best idea! I thought you were gonna love it :disappointed:

I don't really agree that "if we are calculating the count we should return it". I thought a major principle of the module was that we should hide implementation details.

The aggregation thing is obviously on your mind :slightly_smiling_face: but I think we should find a way to "do it right" rather than shoving possibly inaccurate data in the meta section.

e0ipso [10:58 AM]
ah yes
we should come up with better (and additional) aggregations

gabesullice [10:59 AM]
It's clear that you understood that count was a gray area, that's why it's only enabled in Extras to begin with :stuck_out_tongue:

e0ipso [10:59 AM]
but not having this would be a shortcoming of the module. A feature regression IMO. (edited)
@gabesullice it's because it requires an extra query to compute it. So it's better to have opt-in

gabesullice [11:08 AM]

>not having this would be a shortcoming of the module

I don't see that we'll ever agree on that :disappointed:

I see it as fundamentally misleading. Not exposing that value is simply admitting a reality: one can't check access on potentially hundreds of thousands of items to reveal a count of resources in a performant way.

Putting a false count in is just pretending that the problem doesn't exist.

>the counting is a two-fold feature
Putting a total number of pages in the response still gives you pagination + count. If you know the page limit and the number of pages, you can get a count that is just as accurate as the one we provide (which is not accurate to begin with).
Ultimately, I think it's your decision. So that's all I'll say about it. I hope I've made good arguments at least.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Putting a false count in is just pretending that the problem doesn't exist.

Most sites will never experience this problem.

Ultimately, I think it's your decision. So that's all I'll say about it. I hope I've made good arguments at least.

No, no. I won't take a unilateral decision. This decision is also yours to take.

I don't see that we'll ever agree on that :disappointed:

I hope you are wrong.

Ultimately what I care is about the features. Honestly, if it didn't cost us a separate query every time the count would not be optional. If there is a way to provide the total count and a full pagination that is different to what we already have, then I don't care about details. If the proposal is to just drop the features, then I don't agree.

Until consensus is reached I will assume things stay the same. I will provide test coverage for this (at some point). If later on we decide to drop this (via BC break), then we can remove the test coverage as well.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Since the proposal in #11 couldn't provide the features we expected, I'm unblocking #2950744: Move relevant test coverage from jsonapi_extras to jsonapi based on #15.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I have two questions:

  1. Most sites will never experience this problem.

    Could you elaborate on this?

  2. What are the concerns about having a last link? i.e. why does one think it's a viable compromise/solution, and the other thinks it's not?
gabesullice’s picture

gabesullice
Primary language English
commented

No, no. I won't take a unilateral decision. This decision is also yours to take.

To be clear, I just was deferring to you so as not to be an obstructionist, not to force you to take a decision, I think that came off more poorly than I realized.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

To be clear, I just was deferring to you so as not to be an obstructionist, not to force you to take a decision, I think that came off more poorly than I realized.

Please, don't worry too much about that. I assume respect and good intentions when interpreting your words. I hope you see that of me. And that you assume that from me too :-*

#17.1: Most sites don't have contrived access rules that cannot possibly be expressed in a JSON API filter.
#17.2: I doesn't allow a super common use case in web design: Showing 15 items of %COUNT%. Whereas adding a count and a last link does.

More importantly I don't see why this creates such a big hassle. It is a super common use case, it has a very little impact in the codebase and it's optional. I don't see the big win in removing it, even after the previous comments. In addition to all that we've been supporting this since the first dev releases, removing support for this seems like an unnecessary regression (even after we marked all the things as internal at some point).

In other news we have test coverage for both counting and enabling counts in https://www.drupal.org/project/jsonapi/issues/2950744#comment-12519567.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Most sites don't have contrived access rules that cannot possibly be expressed in a JSON API filter.

Sites use content_moderation cannot easily express "is published" in a JSON API filter. So I think this will change rather significantly and quickly!

I doesn't allow a super common use case […]

But the total count is going to be unreliable. Also, I think this is increasingly rare actually … Gmail hasn't showed the total count for years. Twitter doesn't show it. Facebook doesn't show it. (I personally do like seeing that total count though! But only if it's accurate.)

I don't see the big win in removing it, even after the previous comments.

The big win lies in the fact that this the result is unreliable, and that once this is in core, we really can't remove it, so we'll be required to support something unreliable. Bug reports will be filed, and some people will want it to work one way, others will want it to work the other way. It'll be painful to support.

RE: in other news: I saw, GREAT! Thank you :) I reviewed/responded/rerolled there: #2950744-6: Move relevant test coverage from jsonapi_extras to jsonapi.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

The big win lies in the fact that this the result is unreliable

It seems to boil down to this. And I understand the frustration, but I totally disagree with the approach.

I think it's best to be accurate most of the time and document the inaccuracies, rather than throw our arms on the air and regress the feature. I wonder how the Views module is doing it, since it offers aggregation capabilities as well. I don't think we should open an issue to remove aggregation capabilities if they are not 100% accurate 100% of the time.

My take is that this is a tool that has limitations and should be documented and used as such.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I think it's best to be accurate most of the time and document the inaccuracies, rather than throw our arms on the air and regress the feature.

This is indeed where we fundamentally disagree :)

I would not ship this feature. Because documenting the inaccuracies does not work in practice in the real world IMHO: many people will either not read it because they don't want to, or can't find it.

I've also shipped every possible experimental feature in the past with my contrib modules, but I now firmly believe in only shipping features that are rock solid — see https://wimleers.com/article/simplicity-maintainability-cdn-module-drupal-8 if you're interested :)

gabesullice’s picture

gabesullice
Primary language English
commented
Version: 8.x-1.x-dev » 8.x-2.x-dev
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

This is indeed where we fundamentally disagree :)

I guess it is. I don't have any other arguments, other than this is certainly a common request in many projects. Saying, your requirements are wrong or your designs don't meet the real world expectations may not be inaccurate, but that doesn't make it less real. This is a need people have, even if it can't work perfectly.

As a compromise to unblock this I'm not opposed to remove all traces from counting in JSON API, as long as we also provide an accompanying patch for JSON API Extras to keep counts working.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Project: JSON:API » Drupal core
Version: 8.x-2.x-dev » 8.8.x-dev
Component: Code » jsonapi.module
br0ken’s picture

br0ken
Primary language Ukrainian
commented

Excuse me for interrupting you :)

Just wanted to say that I use this feature in several projects and would like to have a clear path for implementing it as a custom if it will be removed.

br0ken’s picture

br0ken
Primary language Ukrainian
commented

Also, recently I removed JSON:API Extras from one project and now working on a patch to return a configurable value from that "static" method.

Edit:

See #3104408: Allow to include a total count of items to collection responses.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Active » Closed (outdated)

This issue is superseeded by #3104408: Allow to include a total count of items to collection responses since we are actually implementing a public API to support this. :)