mathematic-inc / ts-japi

A highly-modular (typescript-friendly)-framework agnostic library for serializing data to the JSON:API specification
Apache License 2.0
202 stars 15 forks source link

[BUG] Related fields are included in attributes by default #77

Open egmacke opened 5 months ago

egmacke commented 5 months ago

Describe the bug*

When serializing a complex object with defined relationships, all related fields are included in the attributes by default as well as in relationships.

To avoid this, you currently have to explicitly define a projection that excludes the relationship fields which adds an unnecessary overhead.

To Reproduce*

See replit: https://replit.com/@egmacke/ts-japi-issue-76

Result is

{
  "jsonapi": {
    "version": "1.0"
  },
  "data": {
    "type": "User",
    "id": "1",
    "attributes": {
      "name": "Foo",
      "articles": [
        {
          "id": "1",
          "title": "Article 1",
          "body": "Article 1 body"
        },
        {
          "id": "2",
          "title": "Article 2",
          "body": "Article 2 body"
        }
      ]
    },
    "relationships": {
      "articles": {
        "data": [
          {
            "type": "Article",
            "id": "1"
          },
          {
            "type": "Article",
            "id": "2"
          }
        ]
      }
    }
  }
}

I would expect the result to be (which can be achieved using the serializer option {projection: {articles: 0}})

{
  "jsonapi": {
    "version": "1.0"
  },
  "data": {
    "type": "User",
    "id": "1",
    "attributes": {
      "name": "Foo",
    },
    "relationships": {
      "articles": {
        "data": [
          {
            "type": "Article",
            "id": "1"
          },
          {
            "type": "Article",
            "id": "2"
          }
        ]
      }
    }
  }
}

Expected behavior*

When a relator is defined for a given field, it should be automatically excluded from the serialized attributes.

egmacke commented 5 months ago

@jun-sheaf I'm happy to work on a solution for this unless you think there's a reason not to?

jrandolf commented 5 months ago

This could be a breaking change I guess? The question is whether the opposite of the default is what's expected from users. I think we can maybe have a global function that changes the default. We can expose it as a named export in the main module.

egmacke commented 5 months ago

@jun-sheaf in order to reduce impact on library users that expect the current behaviour, I'd suggest maybe having a new serializer option - something like excludeRelationshipsFromAttributes which if unset defaults to false, but if unset, it logs a deprecation warning that the default value for this will be changed in future versions (next major version perhaps)?

jrandolf commented 5 months ago

Let's add the option, but without the deprecation notice.

Edit: Actually, scratch that. Do what you feel as the active maintainer.

egmacke commented 5 months ago

Having investigated this further, I've actually found a simpler way to solve this issue.

Rather than making changes to the library, there's a way using core javascript/typescript which solves it.

For any fields that should only be serialized as relationships, defining the property on the data model as true private (using the # notation - ref) then providing a public accessor function (either a method or a getter) prevents the library from seeing the property during serialization.

The property can then be accessed in the relator definition via the method or getter.

I'll create a PR to update the documentation to recommend this approach instead of making code changes.

rodrigopsasaki commented 1 week ago

I just ran into this myself and if I can share my two cents on the matter, I don't think that using the # notation is the best way forward and here is why:

  1. It is not an obvious behavior, libraries meant for wide usage benefit from applying the Principal of Least Astonishment in its design, especially in very common cases
  2. Going the # route prevents us from using interfaces and types, it forces us to use classes for defining the model. Personally I don't think this is a strong enough reason to give up this flexibility