Document why JSON API only supports @DataType-level normalizers

#2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX moves away from using timestamps to some variant of ISO8601/RFC3339 which would make the output way more consistent. I strongly believe we want to do the same thing in jsonapi.

I strongly believe we want to do the same thing in jsonapi.

Indeed.

Better yet: we'll automatically benefit from #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX in JSON API! :)

Also: related work for the datetime field is going on in #2824717: Add a format constraint to DateTimeItem to provide REST error message and #2867206: API-First DX of "date" and "date range" fields: add 'datetime_iso8601' @DataType (de)normalizer using RFC3339 timestamps with timezones.

I agree with the conclusions above. After #2858023: Integer is still serialized as String was merged, we defer field item normalization to whatever aplicable normalizer is registered.

Next steps can be:
- Apply #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX.
- Test if the timestamps get the new format.
- Check that the new format has consistent timezone handling.

@pcambra is that something you'd be willing to assign to yourself?

D8.4.0 is around the corner. I tested the latest dev with jsonapi-8.x-1.1. On a fresh install, REST module outputs timestamp as https://www.drupal.org/node/2859657 advertises, but jsonapi outputs timestamp as before.

Not sure why what's happening and setting the issue status to active and bug report.

I just tested D8.4.0-beta1 with jsonapi-8.x-1.1 on a fresh install (simplytest.me). The problem remains, below is the response:

GET /jsonapi/node/article

{
  "data": [
    {
      "type": "node--article",
      "id": "bb364e01-de8d-48ad-af60-05a5d0f43472",
      "attributes": {
        "nid": 1,
        "uuid": "bb364e01-de8d-48ad-af60-05a5d0f43472",
        "vid": 1,
        "langcode": "en",
        "revision_timestamp": 1503494183,
        "revision_log": null,
        "status": true,
        "title": "Testing Product A",
        "created": 1503494177,
        "changed": 1503494183,
        "promote": true,
        "sticky": false,
        "default_langcode": true,
        "revision_translation_affected": true,
        "path": {
          "alias": null,
          "pid": null,
          "langcode": "en"
        },
        "body": null,
        "comment": {
          "status": 2,
          "cid": 0,
          "last_comment_timestamp": 1503494183,
          "last_comment_name": null,
          "last_comment_uid": 1,
          "comment_count": 0
        }
      },
      "relationships": {
        "type": {
          "data": {
            "type": "node_type--node_type",
            "id": "31bd6d2c-9384-4b9e-9bfc-621657b30f9a"
          },
          "links": {
            "self": "https:\/\/d2i6s.ply.st\/jsonapi\/node\/article\/bb364e01-de8d-48ad-af60-05a5d0f43472\/relationships\/type",
            "related": "https:\/\/d2i6s.ply.st\/jsonapi\/node\/article\/bb364e01-de8d-48ad-af60-05a5d0f43472\/type"
          }
        },
        "revision_uid": {
          "data": {
            "type": "user--user",
            "id": "4d8e4cbd-a3f7-499d-9f89-13500dbe480b"
          },
          "links": {
            "self": "https:\/\/d2i6s.ply.st\/jsonapi\/node\/article\/bb364e01-de8d-48ad-af60-05a5d0f43472\/relationships\/revision_uid",
            "related": "https:\/\/d2i6s.ply.st\/jsonapi\/node\/article\/bb364e01-de8d-48ad-af60-05a5d0f43472\/revision_uid"
          }
        },
        "uid": {
          "data": {
            "type": "user--user",
            "id": "4d8e4cbd-a3f7-499d-9f89-13500dbe480b"
          },
          "links": {
            "self": "https:\/\/d2i6s.ply.st\/jsonapi\/node\/article\/bb364e01-de8d-48ad-af60-05a5d0f43472\/relationships\/uid",
            "related": "https:\/\/d2i6s.ply.st\/jsonapi\/node\/article\/bb364e01-de8d-48ad-af60-05a5d0f43472\/uid"
          }
        },
        "field_image": {
          "data": null
        },
        "field_tags": {
          "data": [
            
          ]
        }
      },
      "links": {
        "self": "https:\/\/d2i6s.ply.st\/jsonapi\/node\/article\/bb364e01-de8d-48ad-af60-05a5d0f43472"
      }
    }
  ],
  "links": {
    "self": "https:\/\/d2i6s.ply.st\/jsonapi\/node\/article"
  }
}

Updated title & summary.

Been working on finding the root cause why JSON API does not pick this up. Stay tuned…

The root cause is actually quite simple.

The JSON API module is adding lots of its own normalizer services. It assigns all of them priorities that are higher than the normalizer services of the serialization module. (With the exception of the HTTP exception normalizer services that JSON API provides, those have lower priorities than many of serialization module's normalizer services.)

The consequence is that when \Symfony\Component\Serializer\Serializer::getNormalizer() runs to get a normalizer for \Drupal\Core\Field\Plugin\Field\FieldType\CreatedItem (which extends TimestampItem), this is what happens:

i.e. \Drupal\jsonapi\Normalizer\FieldItemNormalizer (normalizer 4 in the list of normalizers to evaluate) is evaluated before \Drupal\serialization\Normalizer\TimestampItemNormalizer (normalizer 12 in the list of normalizers to evaluate). For both, supportsNormalization($data, $format) returns TRUE. But the first one that matches, wins. Therefore the higher priority for the generic JSON API field item normalizer causes it to win.

However, that means this doesn't apply just to serialization's TimestampItemNormalizer. It applies to all field type-specific normalizers provided by the serialization module. Currently, that's not too many yet:
most notably TimestampItemNormalizer and PrimitiveDataNormalizer. But with #2852860: REST: top priorities for Drupal 8.4.x, we're filling in serialization gaps, and that list will grow.

This is also why JSON API still has \Drupal\jsonapi\Normalizer\ScalarNormalizer — that should no longer be necessary thanks to \Drupal\serialization\Normalizer\PrimitiveDataNormalizer (added in February 2017, present in Drupal 8.3).

Conclusion: the jsonapi module is not actually building upon the serialization module's normalizers, it's replacing all of them. This is especially problematic when taking contributed modules into account: they'd need to provide both "regular" and "JSON API" normalizers.

And in fact precisely this has already been reported in a support request: #2892193: Custom normalizers are not respected. Marking that as a duplicate of this.

This is exactly the thing I thought (and feared) was the case, and it was one of the next things I was going to review in #2842148: The path for JSON API to "super stability". This is a deep architectural decision. I know that JSON API has its own particular strategy for normalizers, with value objects etc. So perhaps it's impossible to reconcile (i.e. let jsonapi reuse serialization's normalizers). But this is exactly the sort of thing we need to have a clear plan for, going forward.
Again: this is nobody's fault. The #1 priority was to get the JSON API module to a usable state, which was achieved. But the #2 priority is to make it maintainable, and a "good citizen" in the overall API-First Drupal ecosystem. This issue is probably the most important JSON API issue in that respect. I'm sure it won't be easy (because Symfony's serializer system is far from ideal — see #2575761: Discuss a better system for discovering and selecting normalizers). But it's definitely necessary.

I think this reads more negatively than I intended 😔

Basically: we shipped JSON API, and it works! But now that core's REST/Serialization support finally has most of its foundational problems fixed, core is moving on to fixing lots of field normalizers (what I called "serialization gaps" in #2852860: REST: top priorities for Drupal 8.4.x), and so now the shortcuts/work-arounds that JSON API kind of had to take … are now in need of being reconsidered. That's all :) This is a great problem to have! 🎉

Rewrote IS.

Found another related issue, this one is blocked on this being fixed: #2901429: JSON API Extras @ResourceFieldEnhancer plugins should also be applied to filter values.

And #2901253: Add normalization support for metatag module also reports that contrib modules' normalizers are not being respected. This is clearly far from theoretical at this point — many people have already been running into problems with this so far. Made that a duplicate too.

I think you are mixing two different problems, one in JSON API and one in Metatag. Please see https://www.drupal.org/node/2901253#comment-12239668.

Thanks for your thoughts in #14. I'll try to find the time to go through all the points there. I can already spot several inaccuracies that support the conclusions in the This is exactly the thing I thought (and feared) was the case paragraph.

After thinking about the points here I think they're reduced to 2:

1. JSON API needs to pick up on normalizers (from core or contrib) at a basic level. For instance, you wouldn't expect that Drupal\serialization\Normalizer\EntityNormalizer to be used instead of Drupal\jsonapi\Normalizer\ContentEntityNormalizer. The line is drawn somewhere. You are arguing that it's at the FieldItemList level. I think it should happen at the DataType level. If the serialization normalizer was for Drupal\Core\TypedData\Plugin\DataType\Timestamp JSON API would pick it up in here.
2. I do not think that we need a better way to discover normalizers (as you mention in #2575761: Discuss a better system for discovering and selecting normalizers), instead I think that we need to acknowledge the limitations inherent to the problem space. We cannot be compatible at the same time with an opinionated format (denoted by the $format parameter being passed around) and so generic that it can be used in all the opinionated formats (already defined and future). If I ask you what normalizers should take precedence for api_json, the ones in the jsonapi module or the ones in the metatag module, your response will probably span more than one line of text.

FWIW I'm not married to any implementation detail.

I'd love to see this discussion about where to draw the line moving forward.

The one about the expectations about a format is more difficult since different "formats" will have hugely different requirements / assumptions / it depends statements (there are many very bizarre data formats).

I think it should happen at the DataType level.

The serialization module only has normalizers at the @FieldType level. So you're arguing that we should change all of core's normalizers to work at a lower level (which could totally be what's necessary!), which means we need big BC breaks :(
But yes, this is definitely the thing we may need to look into, and may need to change. It's pretty likely we'll need to change both modules: serialization and jsonapi.

I do not think that we need a better way to discover normalizers

If not for discovering, then definitely for selecting, which #2575761: Discuss a better system for discovering and selecting normalizers also covers.

We cannot be compatible at the same time with an opinionated format (denoted by the $format parameter being passed around) and so generic that it can be used in all the opinionated formats (already defined and future).

But earlier you said that you think the line should be drawn at the "DataType" level. And now you're saying it's impossible. You're contradicting yourself?

I think you hit the nail on the head though: normalizers at the "DataType" level are the most atomic bits of data. It makes sense for JSON API to inherit those (and in fact for all formats to inherit those). But hal_json and api_json have very different wrapper/envelope/body structures. There I do agree with you that they can't be generic — and I think that's what you're really getting at? :)

IOW:

1. have format-specific high-level normalizers (at entity, field item list, field item and field item property levels)
2. have generic (formatless) "DataType" normalizers (at property value level)

(I'm not married to any implementation detail either. REST/Serialization as they are today are just the way they are for historical reasons, not for particularly thoughtful reasons AFAIK — otherwise those would've been documented. This issue is just what uncovered the false claim made by JSON API that it builds upon/supports generic normalizers, which #14 proves it does not.)

Again, just to clarify, I think it is inaccurate to say

This issue is just what uncovered the false claim made by JSON API that it builds upon/supports generic normalizers

JSON API does build upon/supports generic normalizers, just not all of them. It reuses the DataType level normalizers (see the code http://cgit.drupalcode.org/jsonapi/tree/src/Normalizer/FieldItemNormaliz...).

It is important to note that without a tight control over the normalizers the JSON API module cannot ensure compliance with the specification. That is potentially true for any format. I understand the nervousness of not seeing your (high level generic) normalizer being applied, but the same nervousness would come if some module broke your valid JSON API because this module is not stopping them.

About:

But earlier you said that you think the line should be drawn at the "DataType" level. And now you're saying it's impossible. You're contradicting yourself?

I'm not sure if it's a contradiction, but what meant is that you cannot pretend you can inject generic high level data structures in ALL formats and pretend it's good for ALL of them. Many formats won't be OK with that. So it's a pattern we should not endorse. However on the *particular case* of JSON API we can re-use the generic ones for DataType. I hope it's more clear now.

As I said, it's a complex issue because it's a complex problem space. An many people underestimate this problem.

See #2895532-93: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map. That's adding a MapNormalizer, for the Map @DataType, which means it'll also work for JSON API :)

Note we've been hard at work solving normalization problems for core's serialization.module (and rest.module), most of our time in that area has gone to #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts, because it provides infrastructure for all other "serialization/normalization gap" issues that are remaining (see #2905563: REST: top priorities for Drupal 8.5.x).

JSON API does build upon/supports generic normalizers, just not all of them. It reuses the DataType level normalizers

Right, but … several key normalizers were not built with this in mind:

• Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer (added in September 2015)
• Drupal\serialization\Normalizer\TimestampItemNormalizer (added in May 2017)
• contrib modules' normalizers, see #2901253: Add normalization support for metatag module for example
• #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field in one of its previous patch iterations
• #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs in its latest patch iteration
• #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata in one of its previous patch iterations
• #2577923: MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property in its latest patch iteration

For the very first time, I'm relieved that those last 4 issues are taking forever to get finished! 🙃

So we'll need to make the necessary changes there and carefully avoid BC breaks. More importantly, we'll need to establish best practices, and essentially strongly recommend against creating @FieldType-level normalizers. Which I honestly didn't realize until this very issue! 😢

It is important to note that without a tight control over the normalizers the JSON API module cannot ensure compliance with the specification. That is potentially true for any format.

Absolutely!

I understand the nervousness of not seeing your (high level generic) normalizer being applied, but the same nervousness would come if some module broke your valid JSON API because this module is not stopping them.

Absolutely! And I'm not nervous about this problem space — it's to be expected that oversights exist in a complex software ecosystem like this (with Typed Data, field types, data types, entity types, multiple formats, and it all interacting somehow 😵). I am nervous about adding JSON API to Drupal core without this being solved.

About:

But earlier you said that you think the line should be drawn at the "DataType" level. And now you're saying it's impossible. You're contradicting yourself?

I'm not sure if it's a contradiction, but what meant is that you cannot pretend you can inject generic high level data structures in ALL formats and pretend it's good for ALL of them. Many formats won't be OK with that. So it's a pattern we should not endorse. However on the *particular case* of JSON API we can re-use the generic ones for DataType. I hope it's more clear now.

As I said, it's a complex issue because it's a complex problem space. An many people underestimate this problem.

Completely agreed! 💯

P.S.: Seeing my comments now, they seem aggressive, they should not have been. Apologies for my tone. I probably did that out of sheer fear of seeing JSON API module added to core without this being resolved first, which would be a huge pile of work. But that still doesn't justify my tone. Sorry! :(

New related issue: #2915539: Omit "_core" key from normalized config entities, which includes the default config hash.

+1 for that better name! Refining it slightly more.

I'll also update the IS tomorrow. Need to run now.

Explicitly pointed #2867206 to this issue and recommended them to write a normalizer for the @DataType-plugin level, which in their case means writing a normalizer for \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601!

See #2867206-8: API-First DX of "date" and "date range" fields: add 'datetime_iso8601' @DataType (de)normalizer using RFC3339 timestamps with timezones and #2867206-9: API-First DX of "date" and "date range" fields: add 'datetime_iso8601' @DataType (de)normalizer using RFC3339 timestamps with timezones.

That's great to hear @Wim Leers!

This issue is listed in #2842148: The path for JSON API to "super stability" as This is probably the most critical bug since we started tagging stable releases..

I see this as a works as designed kind of situation, but I'm opened to listen to proposals. But the issue summary seems to misrepresent the state of the conversation (thanks for adding the tag Wim!). A sample:

This problem will only grow bigger over time: as core adds more sensible normalizers

Core seems to have shifted the approach, as explained in #30.

Another example is:

make the JSON API module respect/reuse normalizers outside the JSON API module.

We already determined that this is not possible while ensuring JSON API compliance, as confirmed in #26 (see Absolutely!)

At this point I don't think that the problem is what we initially thought it was, and the proposed solution is not what we need after all. Hence I'm proposing to close the issue. I will leave it up to Wim to decide to close or see if he has additional concerns and/or solutions.

But the issue summary seems to misrepresent the state of the conversation

Yes, because when I wrote the IS, I didn't realize JSON API was specifically built to only reuse @DataType normalizers! Thanks to your comments, I gained that understanding. Which is why I wrote all the things I wrote in #26.

Specifically:

The general problem: strongly discouraging @FieldType-level normalizers

So we'll need to make the necessary changes there and carefully avoid BC breaks. More importantly, we'll need to establish best practices, and essentially strongly recommend against creating @FieldType-level normalizers. Which I honestly didn't realize until this very issue! 😢

Created #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem for this.

The concrete problems: core's existing @FieldType-level normalizers

Right, but … several key normalizers were not built with this in mind:

• Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer (added in September 2015)
• Drupal\serialization\Normalizer\TimestampItemNormalizer (added in May 2017)
• contrib modules' normalizers, see #2901253: Add normalization support for metatag module for example
• #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field in one of its previous patch iterations
• #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs in its latest patch iteration
• #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata in one of its previous patch iterations
• #2577923: MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property in its latest patch iteration

I'm addressing each of them in turn:

1. 👍 We can keep this as-is, because each normalization (default aka serialization, hal and jsonapi) has its own way of dealing with this.
2. 👎 We need to deprecate TimestampItemNormalizer and introduce TimestampNormalizer instead. JSON API will then be able to reuse this.
3. 👍 #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field is adding a new computed property, not a new normalizer. Therefore it'll work automatically with JSON API.
4. 👍 #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs is adding a new computed property, not a new normalizer. Therefore it'll work automatically with JSON API.
5. 👍 #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata is adding a new computed property, not a new normalizer. Therefore it'll work automatically with JSON API.
6. 👍 #2577923: MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property is adding a new computed property, not a new normalizer. Therefore it'll work automatically with JSON API.

So the damage is fairly limited: we only need to fix TimestampItemNormalizer. That will also fix #2870609: [PP-1] Core Date field is serialized as String instead of timestamp/long. Created #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API for this.

Assigning to myself to update the IS.

#2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API now has a patch that should mostly work. See #2926508-4: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API + #2926508-5: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API.

Status update: #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API has gotten a green patch in the mean time!

A last update before the weekend. #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem now has a complete issue summary, and a clear plan. Next week, I'll add a patch with the test described in the plan.

Of course, we don't want to wait for #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API to land and sites to update to Drupal 8.5 to the expected/intended normalization for timestamp field type and data type. So we should in the mean time duplicate the logic in JSON API, and then remove it once JSON API requires Drupal 8.5.

Per #14, here's an issue to remove JSON API's ScalarNormalizer: #2929935: Remove JSON API's ScalarNormalizer: it's no longer necessary thanks to #2751325's PrimitiveDataNormalizer.

Per #14 + #33 + #36, here's an issue to add TimestampNormalizer to JSON API until #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API lands and can require Drupal 8.5 (the version in which #2926508 will most likely land): #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands.

I think this issue is now effectively a documentation issue. We need to document JSON API creator @e0ipso's comment #21 in a more discoverable place. I'd suggest adding jsonapi.api.php, which is what several core modules also do: that's the way to have documentation be in the git repo.

Added #2887372: Add "subfield name" to storage settings for Double Field, for better normalization in core REST, JSON API and GraphQL as another example.

I love this patch. It feels like closure to one of the most misunderstood necessary compromises in this module.

It feels like closure to one of the most misunderstood necessary compromises in this module.

Exactly! ❤️

And now that we have this jsonapi.api.php file, expect many more additions in days and weeks to come. It'll become crucial for this module's future, because it'll help future contributors understand the architecture :)

FYI: #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem now has a patch and a CR.