Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases

Postponed on #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.

#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only has been pretty much completed. Time to get started here.

Great! 💪🏽

Let me know if you want to brainstorm on what this issue should cover.

First: a test coverage test that ensures we have complete test coverage. This is pretty much copied from #2868035: Test that all core content+config entity types have functional REST test coverage.

+++ b/tests/src/Functional/TestCoverageTest.php
@@ -0,0 +1,117 @@
+namespace Drupal\Tests\rest\Functional\EntityResource;

Of course this didn't fail as expected /facepalm

That failed for the wrong reasons on 8.4, but for the right reasons on 8.6. This interdiff should ensure they behave consistently.

8.6 failed like this:

1) Drupal\Tests\jsonapi\Functional\TestCoverageTest::testEntityTypeRestTestCoverage
☼
      _________________________
     /           Hi!           \
    |  It's llame to not have   |
    |  complete JSON API tests! |
    |                           |
    |     Progress: 7/43.      |
    | _________________________/
    |/
//  o
l'>
ll
llama
|| ||
'' ''

Failed asserting that Array &0 (
    0 => 'aggregator_item: Item (Drupal\aggregator\Entity\Item) (expected tests: \Drupal\Tests\aggregator\Functional\Jsonapi\ItemTest)'
    1 => 'aggregator_feed: Feed (Drupal\aggregator\Entity\Feed) (expected tests: \Drupal\Tests\aggregator\Functional\Jsonapi\FeedTest)'
    2 => 'block_content_type: BlockContentType (Drupal\block_content\Entity\BlockContentType) (expected tests: \Drupal\Tests\block_content\Functional\Jsonapi\BlockContentTypeTest)'
    3 => 'block_content: BlockContent (Drupal\block_content\Entity\BlockContent) (expected tests: \Drupal\Tests\block_content\Functional\Jsonapi\BlockContentTest)'
    4 => 'comment_type: CommentType (Drupal\comment\Entity\CommentType) (expected tests: \Drupal\Tests\comment\Functional\Jsonapi\CommentTypeTest)'
    5 => 'contact_message: Message (Drupal\contact\Entity\Message) (expected tests: \Drupal\Tests\contact\Functional\Jsonapi\MessageTest)'
    6 => 'contact_form: ContactForm (Drupal\contact\Entity\ContactForm) (expected tests: \Drupal\Tests\contact\Functional\Jsonapi\ContactFormTest)'
    7 => 'editor: Editor (Drupal\editor\Entity\Editor) (expected tests: \Drupal\Tests\editor\Functional\Jsonapi\EditorTest)'
    8 => 'field_storage_config: FieldStorageConfig (Drupal\field\Entity\FieldStorageConfig) (expected tests: \Drupal\Tests\field\Functional\Jsonapi\FieldStorageConfigTest)'
    9 => 'field_config: FieldConfig (Drupal\field\Entity\FieldConfig) (expected tests: \Drupal\Tests\field\Functional\Jsonapi\FieldConfigTest)'
    10 => 'file: File (Drupal\file\Entity\File) (expected tests: \Drupal\Tests\file\Functional\Jsonapi\FileTest)'
    11 => 'filter_format: FilterFormat (Drupal\filter\Entity\FilterFormat) (expected tests: \Drupal\Tests\filter\Functional\Jsonapi\FilterFormatTest)'
    12 => 'image_style: ImageStyle (Drupal\image\Entity\ImageStyle) (expected tests: \Drupal\Tests\image\Functional\Jsonapi\ImageStyleTest)'
    13 => 'language_content_settings: ContentLanguageSettings (Drupal\language\Entity\ContentLanguageSettings) (expected tests: \Drupal\Tests\language\Functional\Jsonapi\ContentLanguageSettingsTest)'
    14 => 'configurable_language: ConfigurableLanguage (Drupal\language\Entity\ConfigurableLanguage) (expected tests: \Drupal\Tests\language\Functional\Jsonapi\ConfigurableLanguageTest)'
    15 => 'media_type: MediaType (Drupal\media\Entity\MediaType) (expected tests: \Drupal\Tests\media\Functional\Jsonapi\MediaTypeTest)'
    16 => 'media: Media (Drupal\media\Entity\Media) (expected tests: \Drupal\Tests\media\Functional\Jsonapi\MediaTest)'
    17 => 'node_type: NodeType (Drupal\node\Entity\NodeType) (expected tests: \Drupal\Tests\node\Functional\Jsonapi\NodeTypeTest)'
    18 => 'rdf_mapping: RdfMapping (Drupal\rdf\Entity\RdfMapping) (expected tests: \Drupal\Tests\rdf\Functional\Jsonapi\RdfMappingTest)'
    19 => 'responsive_image_style: ResponsiveImageStyle (Drupal\responsive_image\Entity\ResponsiveImageStyle) (expected tests: \Drupal\Tests\responsive_image\Functional\Jsonapi\ResponsiveImageStyleTest)'
    20 => 'rest_resource_config: RestResourceConfig (Drupal\rest\Entity\RestResourceConfig) (expected tests: \Drupal\Tests\rest\Functional\Jsonapi\RestResourceConfigTest)'
    21 => 'search_page: SearchPage (Drupal\search\Entity\SearchPage) (expected tests: \Drupal\Tests\search\Functional\Jsonapi\SearchPageTest)'
    22 => 'shortcut_set: ShortcutSet (Drupal\shortcut\Entity\ShortcutSet) (expected tests: \Drupal\Tests\shortcut\Functional\Jsonapi\ShortcutSetTest)'
    23 => 'shortcut: Shortcut (Drupal\shortcut\Entity\Shortcut) (expected tests: \Drupal\Tests\shortcut\Functional\Jsonapi\ShortcutTest)'
    24 => 'menu: Menu (Drupal\system\Entity\Menu) (expected tests: \Drupal\Tests\system\Functional\Jsonapi\MenuTest)'
    25 => 'action: Action (Drupal\system\Entity\Action) (expected tests: \Drupal\Tests\system\Functional\Jsonapi\ActionTest)'
    26 => 'tour: Tour (Drupal\tour\Entity\Tour) (expected tests: \Drupal\Tests\tour\Functional\Jsonapi\TourTest)'
    27 => 'workflow: Workflow (Drupal\workflows\Entity\Workflow) (expected tests: \Drupal\Tests\workflows\Functional\Jsonapi\WorkflowTest)'
    28 => 'menu_link_content: MenuLinkContent (Drupal\menu_link_content\Entity\MenuLinkContent) (expected tests: \Drupal\Tests\menu_link_content\Functional\Jsonapi\MenuLinkContentTest)'
    29 => 'view: View (Drupal\views\Entity\View) (expected tests: \Drupal\Tests\views\Functional\Jsonapi\ViewTest)'
    30 => 'base_field_override: BaseFieldOverride (Drupal\Core\Field\Entity\BaseFieldOverride) (expected tests: \Drupal\Tests\Core\Functional\Jsonapi\BaseFieldOverrideTest)'
    31 => 'entity_view_mode: EntityViewMode (Drupal\Core\Entity\Entity\EntityViewMode) (expected tests: \Drupal\Tests\Core\Functional\Jsonapi\EntityViewModeTest)'
    32 => 'entity_view_display: EntityViewDisplay (Drupal\Core\Entity\Entity\EntityViewDisplay) (expected tests: \Drupal\Tests\Core\Functional\Jsonapi\EntityViewDisplayTest)'
    33 => 'entity_form_mode: EntityFormMode (Drupal\Core\Entity\Entity\EntityFormMode) (expected tests: \Drupal\Tests\Core\Functional\Jsonapi\EntityFormModeTest)'
    34 => 'entity_form_display: EntityFormDisplay (Drupal\Core\Entity\Entity\EntityFormDisplay) (expected tests: \Drupal\Tests\Core\Functional\Jsonapi\EntityFormDisplayTest)'
    35 => 'date_format: DateFormat (Drupal\Core\Datetime\Entity\DateFormat) (expected tests: \Drupal\Tests\Core\Functional\Jsonapi\DateFormatTest)'
) is identical to Array &0 ().

😀🤘

Let me know if you want to brainstorm on what this issue should cover.

I now see that this issue says Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases. I'd actually propose to limit it to just for every entity type. I think we should have a separate issue for complex use cases. Just adding test coverage for all other entity types will already make this patch plenty big.

Thoughts?

This adds test coverage for MenuLinkContent.

Unless remaining types is pretty trivial, I'd prefer to see complex cases worked on before remaining types. I think "complex cases" will be more likely to uncover higher impact bugs.

I disagree.

Getting remaining types done can be done in a shorter amount of time. And then when we add the generic test coverage for the complex cases to ResourceTestBase, it will run automatically for all entity types. Which means we'll be able to uncover entity type-specific bugs in those complex cases.

IOW: doing "remaining types" first will make "complex cases" more reliable.

I started tackling this alphabetically. So I started with aggregator_item. Which immediately runs into the one edge case in all of Drupal core: that's the only entity type that does NOT support UUIDs… and hence breaks horrendously in JSON API:

Parameter "aggregator_item" for route "jsonapi.aggregator_item--aggregator_item.individual" must match "[^/]++" ("" given) to generate a corresponding URL.

Fixing that is blocked on #2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field.

This adds test coverage for aggregator_item + aggregator_feed.

block_content_type + block_content

#10:

      _________________________
     /           Hi!           \
    |  It's llame to not have   |
    |  complete JSON API tests! |
    |                           |
    |     Progress: 7/41.      |
    | _________________________/
    |/
//  o
l'>
ll
llama
|| ||
'' ''

#19:

      _________________________
     /           Hi!           \
    |  It's llame to not have   |
    |  complete JSON API tests! |
    |                           |
    |     Progress: 12/41.      |
    | _________________________/
    |/
//  o
l'>
ll
llama
|| ||
'' ''

More progress tomorrow!

comment_type + contact_message + contact_form.

Here, contact_message is another special case, because those entities are never stored. That means they can only be POSTed. All other verbs are not allowed. And no routes should be created for them. We have no way to signal this just yet. Core's REST module achieves this via contact_rest_resource_alter() overriding the default REST resource plugin class.
I was contemplating making JSON API respect this automatically for any entity type using the Drupal\Core\Entity\ContentEntityNullStorage storage handler. But because JSON API has a single route for multiple methods, that's less easy to achieve, and merits more debate. So, created a new issue for that: #2944977: Message entities can only be created (POSTed), they cannot be read or modified.
(I already had to make some changes to account for the fact that not every JSON API POST response would contain a Location header.)

editor + filter_format

For editor, I had to comment a piece of code that calculated data for an assertion, and that assertion itself was already commented due to #2942561: Assert denormalizing the JSON API response results in the expected object. Config entity denormalization is unsurprisingly unreliable.

For filter_format, I ran into a show-stopping bug: JSON API is normalizing the config entity's data in a way that results in data loss. We should see:

          'filters' => [
            'filter_html' => [
              'id' => 'filter_html',
              'provider' => 'filter',
              'status' => TRUE,
              'weight' => -10,
              'settings' => [
                'allowed_html' => '<p> <a> <b> <lo>',
                'filter_html_help' => TRUE,
                'filter_html_nofollow' => FALSE,
              ],
            ],
          ],

but instead see:

            'filters' => Array &4 (
                'id' => 'filter_html'
                'provider' => 'filter'
                'settings' => Array &5 (
                    'allowed_html' => '<p> <a> <b> <lo>'
                    'filter_html_help' => true
                    'filter_html_nofollow' => false
                )
                'status' => true
                'weight' => -10
            )

In other words: it's stripping an entire level… This seems to have the same root cause as #2930028-33: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only. #2939800: FieldNormalizerValue::rasterizeValue() respects cardinality === 1, but doesn't verify that this is true improved the situation in that previously discovered case. But in this case, it's still breaking things. The fix is to not just check whether there's >1 value in the array, but whether it's a numerically indexed array or an associatively indexed array. In the latter case, indexes/keys have meaning, and shouldn't be dropped! I have yet to create an issue for this, but wanted the problem to appear in the failed tests.

We want to check (1) the cardinality of the field and (2) whether its data definition is a sequence or a map.

Except there are no data definitions for config entity types.

Which brings me to what I think we actually need to do: special case config entities and just make it output exactly what is stored, no attempts to simplify the data structure can be made.

field_storage_config + field_config + base_field_override

field_storage_config has a dependency, and therefore runs into the exact same problem as block in #2930028-55: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only and similar to filter_format in #24. #2942979: JSON API's normalization of config entities' "dependencies" key-value pair omits essential information when there's only a single dependency is the issue that was created for that. Same exact problem for base_field_override.

(field_config would run into that same problem, if it weren't for the fact it has multiple dependencies and thanks to that is able to escape this fate!)

Confirmed that the problem surfaced in #24 and discussed in #26 + #27, and surfaced again in #28, and that we have an issue for it already: #2942979: JSON API's normalization of config entities' "dependencies" key-value pair omits essential information when there's only a single dependency.

I moved the discussion in #24+#26+#27 over to #2942979-4: JSON API's normalization of config entities' "dependencies" key-value pair omits essential information when there's only a single dependency. Let's continue there.

Attached patch is updating FilterFormatTest to expect the current behavior and with a @todo to fix it there.

language_content_settings + configurable_language + image_style + rdf_mapping + responsive_image_style

configurable_language has one extra test, because the REST test also does: \Drupal\Tests\rest\Functional\EntityResource\ConfigurableLanguage\ConfigurableLanguageResourceTestBase::testGetDefaultConfig()), added by #2915414: Omit "_core" key from normalized config entities, which includes the default config hash. The JSON API port of that issue is in #2915539: Omit "_core" key from normalized config entities, which includes the default config hash. The test coverage is currently asserting HEAD's behavior, which is wrong, but in doing so it paves the path for #2915539!

node_type + rest_resource_config (HAH!) + search_page + menu + action

(Also fixed configurable_language_test from #32, the file was incorrectly named.)

#19 (~20 hours ago):

      _________________________
     /           Hi!           \
    |  It's llame to not have   |
    |  complete JSON API tests! |
    |                           |
    |     Progress: 12/41.      |
    | _________________________/
    |/
//  o
l'>
ll
llama
|| ||
'' ''

#34:

      _________________________
     /           Hi!           \
    |  It's llame to not have   |
    |  complete JSON API tests! |
    |                           |
    |     Progress: 30/41.      |
    | _________________________/
    |/
//  o
l'>
ll
llama
|| ||
'' ''

Hopefully at 100% later today.

entity_view_mode + entity_form_mode + entity_view_display + entity_form_display

shortcut_set + tour + workflow + view + date_format

Updated IS and created #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets for the "complex use cases" aka JSON API-specific features part.

d.o can be so broken in cross-posting situations … not only did it lose my IS update, it also unpublished this node! WTF.

Fixed.

file + shortcut

shortcut does something special with the cache tags to invalidate: it doesn't have its own cache tags, instead it defers to the shortcut_set entity it belongs to, and hence uses that cache tag instead. Due to pre-existing bugs in JSON API wrt caching, there is only one way forward: hardcode a work-around for shortcut in ResourceTestBase — but fortunately the existing work-arounds can be modified so that there is no need for a hardcoded special case for shortcut!

From 94 coding standards violations to 0.

Disabled BlockContentTest on 8.4, it needs 8.5 per #2835845: EntityResource: Provide comprehensive test coverage for BlockContent entity.

Similar story for EntityViewDisplay and EntityFormDisplay: they need 8.5 because they need #2866666: Add proper access handlers for view and form displays. Same for File.

This patch should now be green on 8.4! But we'll need to add test coverage for two more entity types for it to pass on 8.6. That's for tomorrow!

Another crosspost, another time that d.o unpublishes this node! 😡

Fortunately, the patch is green! ✅

For 8.6: media + media_type. They both required extra care to also work on 8.4 and 8.5.

And now with documentation in jsonapi.api.php.

1. ✔️ — significantly rephrased, let me know what you think
2. ✔️

RE: only nit: I'd rather keep it like this, at least for now, because that means less magic. The expected result is there, plain to see, with as little dynamism as possible. That's valuable for complex data structure-based assertions/expectations. Plus, it also leaves the door open to put these expectations in static JSON files rather than as PHP code … which is something we should investigate/evaluate in another follow-up.

#52 passed on 8.4 but failed on 8.6 because #2939729: Do not provide links to related/relationship routes for internal resource types. landed after #51, which causes some changes in certain JSON API documents. Fixed that too.

RTBC and green on both 8.4 and 8.5. One coding standards message, that can be fixed on commit.

Because:

1. this is blocking so many other issues
2. this is just a continuation of #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only, which @e0ipso RTBC'd & committed

… I went ahead and committed this. HURRAY! HUGE milestone! JSON API now has equally strong test coverage as core's REST module!
But still has many @todos in that test coverage to fix — so not quite on par with REST yet.)

Oops… 😅

👍 — thanks!

Looks like #58 was reported again, but this time by a JSON API module user: #2948044: Exceptions no longer have an "errors" parent key.