Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE ds-payload-hooks] Add hooks to map type in payload to modelName #4318

Merged
merged 1 commit into from
May 23, 2016
Merged

[FEATURE ds-payload-hooks] Add hooks to map type in payload to modelName #4318

merged 1 commit into from
May 23, 2016

Conversation

pangratz
Copy link
Member

The modelNameFromPayloadKey and payloadKeyFromModelName hooks on the
serializer allow to customize the mapping between the key of a JSON and
the corresponding name of the model. Consider the following payload:

{
  "blog/post": {
    "id": 1
  }
}

To map the "blog/post" key to the post model, the
modelNameFromPayloadKey hook is used:

serializer.modelNameFromPayloadKey("blog/post")

There are some issues with the current code base, as the
modelNameFromPayloadKey and payloadKeyFromModelName hooks are also
used to map the "value" of a type from / to the payload. Consider the
following payload of a JSON-API document:

{
  "data": {
    "id": 1,
    "type": "API:V1::User"
  }
}

To map the namespaced type to the model name ember-data is using,
currently the modelNameFromPayloadKey is used:

serializer.modelNameFromPayloadKey("API::V1::User")

Now this gets complicated if your API responds with the following
payload (using custom key and custom types for polymorphic records for
example):

{
  "blog/post": {
    "id": 1,
    "user": 2,
    "userType": "API:V1::Administrator"
  }
}

Now the modelNameFromPayloadKey is invoked for both:

serializer.modelNameFromPayloadKey("blog/post")
serializer.modelNameFromPayloadKey("API::V1::Administrator")

This means that the logic within the hook would get complicated.


As the name suggests, the method should only be used to map the key, not
the value. This commit adds modelNameFromPayloadType and
payloadTypeFromModelName hooks and uses them during normalization and
serialization.

This commit maintains the old behavior using, if it is used and logs a
deprecation warning to move to the new hooks.


This PR should help tackling the issues raised in #3801 and #4208.

@bmac
Copy link
Member

bmac commented Apr 10, 2016

Overall looks good. Do you mind wrapping this in a feature flag?

let modelName = this.modelNameFromPayloadType(relationshipDataHash.type);
let deprecatedModelNameLookup = this.modelNameFromPayloadKey(relationshipDataHash.type);

if (modelName !== deprecatedModelNameLookup && this.modelNameFromPayloadType === JSONAPISerializer.prototype.modelNameFromPayloadType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like if a user intentionally wanted modelNameFromPayloadType and modelNameFromPayloadKey to return different values and they needed to define a custom modelNameFromPayloadType we would always hit the deprecated case.

What about if we always called modelNameFromPayloadType but had its default implementation just call modelNameFromPayloadKey?

@pangratz pangratz changed the title Add modelNameFromPayloadType and payloadTypeFromModelName hooks [FEATURE ds-payload-hooks] Add hooks to map type in payload to modelName Apr 13, 2016
The `modelNameFromPayloadKey` and `payloadKeyFromModelName` hooks on the
serializer allow to customize the mapping between the key of a JSON and
the corresponding name of the model. Consider the following payload:

```json
{
  "blog/post": {
    "id": 1
  }
}
```

To map the "blog/post" key to the `post` model, the
`modelNameFromPayloadKey` hook is used:

```js
serializer.modelNameFromPayloadKey("blog/post")
```

---

There are some issues with the current code base, as the
`modelNameFromPayloadKey` and `payloadKeyFromModelName` hooks are also
used to map the "value" of a type from / to the payload. Consider the
following payload of a JSON-API document:

```json
{
  "data": {
    "id": 1,
    "type": "API:V1::User"
  }
}
```

To map the namespaced type to the model name ember-data is using,
currently the `modelNameFromPayloadKey` is used:

```js
serializer.modelNameFromPayloadKey("API::V1::User")
```

Now this gets complicated if your API responds with the following
payload (using custom key and custom types for polymorphic records for
example):

```json
{
  "blog/post": {
    "id": 1,
    "user": 2,
    "userType": "API:V1::Administrator"
  }
}
```

Now the `modelNameFromPayloadKey` is invoked for both:

```js
serializer.modelNameFromPayloadKey("blog/post")
serializer.modelNameFromPayloadKey("API::V1::Administrator")
```

This means that the logic within the hook would get complicated.

---

As the name suggests, the method should only be used to map the key, not
the value. This commit adds `modelNameFromPayloadType` and
`payloadTypeFromModelName` hooks and uses them during normalization and
serialization.

This commit maintains the old behavior using, if it is used and logs a
deprecation warning to move to the new hooks.
@fivetanley
Copy link
Member

We should also make an ember-watson transform after this gets merged.

@pangratz
Copy link
Member Author

@bmac I've updated the code and put everything behind the ds-payload-type-hooks feature flag.

You're comment made me re-think the approach and now the following "rules" apply:

  • modelNameFromPayloadKey and payloadKeyFromModelName are deprecated on json and json-api serializer, since the their payload doesn't have the type in any key; there is a deprecation warning to use the new modelNameFromPayloadType and payloadTypeFromModelName hooks instead
  • rest serializer supports all 4 hooks (payloadKey and payloadType) since they make sense for that payload
    • if the value for the new hook differs with the old one, then a deprecation is logged iff the new hook is not implemented but the old one is
    • if they are the same then everything is ok

@bmac bmac merged commit 0e15f2f into emberjs:master May 23, 2016
@pangratz pangratz deleted the model-name-from-payload-type branch June 25, 2016 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants