[Regression] Broken with latest jsonapi 2.0-rc3 [#3020237]

Problem/Motivation

The issue was covered by #3020112: [Regression] API change in jsonapi module., but once that was resolved, it uncovered this particular one.

The issue is once jsonapi_extras is installed unless you explicitly SAVE the resource type fields are not shown through the API.

In short the default behavior has regressed and any time that a NullResourceType is used, the particular resource in the API will not have fields present in the payload.

This is caused by code from jsonapi_extras (the early return):

  /**
   * ...
   */
  protected function overrideFieldMapping(JsonapiResourceConfig $resource_config) {
    if ($resource_config instanceof NullJsonapiResourceConfig) {
      return [];
    }
    // ...
  }

And how NULL configs handle fields...

Proposed resolution

Any direction on how to resolve this, and feedback on whether the current patch direction is ok-ish.

Except the patch, there is a semi-work-around - go in the UI and re-save all entity configs manually and export that as configurations, so we are not using the NULL configs anywhere. At that point everything will be marked as overwritten and maintenance efforts will greatly increase. This is first not a scalable option and it is not a valid one, as we can not expose web forms and similar dynamic things through JSON:API anymore.

I will propose to extend the NULL type with a valid original ID (even if it's NULL) so we can fetch field mappings for it if needed.

This is jsonapi_extras issue due to the fact it's resolved once jsonapi_extras is disabled as seein in #5.

Remaining tasks

Direction approval.
Resolve the API write issue in tests and/or code (TBD).
Patch (we have API-read scenario working now)
RTBC
Commit

User interface changes

None.

API changes

None expected...

Data model changes

None.

Release notes snippet

None.

Comment	File	Size	Author
#24	3020237--regression--24.patch	10.68 KB	e0ipso
#24	8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 10 pass
#15	interdiff-3020237-11-15.txt	1.04 KB	ndobromirov
#15	3020237-15.patch	2.77 KB	ndobromirov
#15	8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 8 pass, 1 fail
#11	3020237-11.patch	1.73 KB	ndobromirov
#11	8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 7 pass, 3 fail
#8	regression_jsonapi-3020237-8.patch	2.21 KB	rhristov
#8	8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 7 pass, 3 fail
#6	3020237-6.patch	1.55 KB	ndobromirov
#6	8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 7 pass, 3 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

13 December 2018 at 08:37

ndobromirov created an issue. See original summary.

Comment #2

ndobromirov CreditAttribution: ndobromirov at FFW commented 13 December 2018 at 08:38

Issue summary:

View changes

Comment #3

ndobromirov CreditAttribution: ndobromirov at FFW commented 13 December 2018 at 08:38

Issue summary:

View changes

Comment #4

ndobromirov CreditAttribution: ndobromirov at FFW commented 13 December 2018 at 08:51

What was the reasoning behind NULL configs?
With all the caches involved, in the process now is the speed-up from NULL configs still meaningful?
Why not just instantiate fully valid resource configurations based on system state instead of NULL instances and just have a flag that they are default.
Add a DefaultResourceConfig(entity, bundle), so upon creation, this will be handled transparently from:

  protected function getResourceConfig($resource_config_id) {
    try {
      $resource_configs = $this->getResourceConfigs();
      return isset($resource_configs[$resource_config_id]) ?
        $resource_configs[$resource_config_id] :
        new NullJsonapiResourceConfig([], '');
    }
    catch (PluginException $e) {
      return new NullJsonapiResourceConfig([], '');
    }
  }

Just throwing ideas... :(

Comment #5

ndobromirov CreditAttribution: ndobromirov at FFW commented 13 December 2018 at 12:24

Disabling the jsonapi_extras is resolving the issue, so it's not jsonapi related.

Comment #6

ndobromirov CreditAttribution: ndobromirov at FFW commented 13 December 2018 at 12:59

Status:

Active

» Needs review

File	Size
3020237-6.patch	1.55 KB
8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 7 pass, 3 fail

Here is an atempt at a fix...

Comment #7

13 December 2018 at 13:01

Status:

Needs review

» Needs work

The last submitted patch, 6: 3020237-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Comment #8

rhristov CreditAttribution: rhristov at Bulcode commented 13 December 2018 at 13:15

File	Size
regression_jsonapi-3020237-8.patch	2.21 KB
8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 7 pass, 3 fail

Applying a new patch that is fixing the issue: it fallbacks to the default JSONAPI resource type and field item normalizer if the configuration is not overridden.

Comment #9

rhristov CreditAttribution: rhristov at Bulcode commented 13 December 2018 at 13:15

Status:

Needs work

» Needs review

Comment #10

13 December 2018 at 13:28

Status:

Needs review

» Needs work

The last submitted patch, 8: regression_jsonapi-3020237-8.patch, failed testing. View results

Comment #11

ndobromirov CreditAttribution: ndobromirov at FFW commented 13 December 2018 at 13:32

Status:

Needs work

» Needs review

File	Size
3020237-11.patch	1.73 KB
8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 7 pass, 3 fail

Handle the exception case as well in the same manner...

Comment #12

13 December 2018 at 13:42

Status:

Needs review

» Needs work

The last submitted patch, 11: 3020237-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Comment #13

ndobromirov CreditAttribution: ndobromirov at FFW commented 13 December 2018 at 15:05

I am sure that at least some of the tests are now broken and this is jsonapi 2.x related somehow. Terms tid is now exposed as drupal_internal__tid.

Comment #14

ndobromirov CreditAttribution: ndobromirov at FFW commented 13 December 2018 at 15:05

Issue tags:

+API-First Initiative

Comment #15

ndobromirov CreditAttribution: ndobromirov at FFW commented 13 December 2018 at 15:38

Status:

Needs work

» Needs review

File	Size
3020237-15.patch	2.77 KB
8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 8 pass, 1 fail
interdiff-3020237-11-15.txt	1.04 KB

2 files were hidden/shown/deleted

File	Size
3020237-6.patch	1.55 KB
8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 7 pass, 3 fail
3020237-11.patch	1.73 KB
8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 7 pass, 3 fail

Updating tests should resolve one of the failures as it seems like a change in the structure there.

Comment #16

13 December 2018 at 15:39

Status:

Needs review

» Needs work

The last submitted patch, 15: 3020237-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Comment #17

ndobromirov CreditAttribution: ndobromirov at FFW commented 19 December 2018 at 13:34

The only failing test is write through the api.
As this is not a use case for us, we can use the patch as is.

Any support to make this green will be appreciated as I am not 100% sure how to test the writes manually :)!

Comment #18

vtcore CreditAttribution: vtcore commented 14 December 2018 at 13:37

Just updated jsonapi to from 2.0-rc2 to 2.0-rc3 and exactly the same thing happened. For not overwritten resources the response is missing the attributes property.
Another thing is that the nid and vid properties are now drupal_internal__nid and drupal_internal__vid while in the overwrites UI of JSON:API Extras they are still named nid and vid.

Comment #19

ndobromirov CreditAttribution: ndobromirov at FFW commented 14 December 2018 at 13:42

Title:

[Regression] Broken with latest jsonapi 2.x-dev

» [Regression] Broken with latest jsonapi 2.0-rc3

Comment #20

daniel.nitsche CreditAttribution: daniel.nitsche commented 15 December 2018 at 12:52

I haven't had time to test this thoroughly, but the patch in #15 seems to bring back attributes and relationships where they were previously missing.

@vtcore -- should a separate issue be logged for the nid/vid issue?

Comment #21

ndobromirov CreditAttribution: ndobromirov at FFW commented 17 December 2018 at 08:27

Comment #22

cspitzlay

German

🇩🇪🇪🇺

CreditAttribution: cspitzlay at bio.logis Genetic Information Management GmbH commented 19 December 2018 at 12:43

My 2 cents:

I had similar issues: supposedly unkown filter fields (and missing fields in the output when not filtering).
I tried #15 and it solved the issue in my test cases.

Comment #23

ndobromirov CreditAttribution: ndobromirov at FFW commented 19 December 2018 at 13:44

Issue summary:

View changes

Updated issue summary with latest findings...

Comment #24

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 22 December 2018 at 22:49

Status:

Needs work

» Needs review

File	Size
3020237--regression--24.patch	10.68 KB
8.x-3.x: PHP 7.2 & MySQL 5.5, D8.6.4 10 pass

Comment #25

22 December 2018 at 23:12

e0ipso authored 29cb1c0 on 8.x-3.x

Issue #3020237 by ndobromirov, rhristov, e0ipso, vtcore, daniel.nitsche...

Comment #26

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 22 December 2018 at 23:12

Status:

Needs review

» Fixed

Comment #27

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 24 December 2018 at 17:07

I think #3020365: Fatal error with JSON:API 2.0-RC3 at /jsonapi/user/user: Call to a member function getPropertyType() on null was a duplicate?

Comment #28

e0ipso

Catalan

Can Picafort

CreditAttribution: e0ipso as a volunteer and at Lullabot commented 26 December 2018 at 06:07

Good call @Wim Leers. Thanks for this!

Comment #29

9 January 2019 at 06:09

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Comment #30

audriusb CreditAttribution: audriusb commented 29 January 2019 at 18:07

I am still having an issue in v3.3. file field is always empty but it comes back populated when I disable jsonapi_extras. trying with jsonapi 2.1

[Regression] Broken with latest jsonapi 2.0-rc3

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Related issues

Referenced by