Change _normalizeTypeKey to defer normalization to the container #2821

mixonic · 2015-02-25T20:57:40Z

Let's talk about Ember-Data model resolution works in this PR.

First, remember that isolated containers have no normalization. When testing with isolated containers (unit tests), the user-entered model name becomes the factory name requested from the container as well as the typeKey.

For example, in a unit test modelFor works like this:

modelFor("SomeItem")
Looks for "model:SomeItem" in the container
This is normalized to "model:SomeItem" (unit tests have no
normalization)
The factory is returned. In Ember-CLI, an error (?). In globals Ember,
App.SomeItem.
The typeKey is set to the normalized value: SomeItem

In an acceptance or app environment the container has a different resolver. In these cases normalization changes things. For example, with the Ember-CLI resolver:

modelFor("SomeItem")
Looks for "model:SomeItem" in the container
This is normalized to "model:some-item" (Ember-CLI is dasherizing)
The factory is returned. In Ember-CLI, that from app/models/some-item.
The typeKey is set to the normalized value: some-item

There are several points of confusion IMO. I'd like to suggest two followup issues:

Most developers do not realize that unit testing uses a different resolver/normalization than apps and acceptance tests. Acceptance tests are more "forgiving". If you camelCase in an Ember-CLI unit test you will get model-not-found errors. IMO (and I've PR this before, got it merged, and it was ripped out), Ember-Data should normalize model names before passing them to the container. This doesn't mean the typeKey should not be based on the container normalization (should not matter, should be an internal anyway), but that an additional normalization step would be introduced at the beginning of modelFor. This normalization would ensure that Ember-Data, when there is no normalization provided by the container, always normalizes its own requests to camelCase or some other consistent format instead of blindly trying to trust user input.
RESTAdapter defers to the typeKey for model keys. If you change your resolver, you will change the requests sent to your server and the way data is parsed.
- serializing a hash presumes the typeKey is the payload key. bmac suggested using typeForRoot here (which is definitely just a happens to work fix, it camelizes) or introducing a rootForType. rootForType with a default of camelization (based on other typeKey normalization in that file) is correct and I believe necessary.
- Embedded records mixin also seems to presume the type can be read from a server response during extract. This should maybe go through typeForRoot as well.

See also:

mixonic · 2015-02-25T21:06:45Z

packages/ember-data/tests/integration/serializers/rest_serializer_test.js

-
-  deepEqual(json["evilMinionType"], "yellowMinion");
-});
-


I believe this test is incorrect.

You should never manually set a typeKey.

The YellowMinion was registered into the container with camelCase.

This is a unit test, there is no normalization of model names

Looking into polymorphics, I am confused by the type usage. Sometimes it seems like we trust the typeKey (serialization) other times we camelize it (relationType).

bmac · 2015-02-26T02:12:33Z

I'm also in favor of adding a rootForType with a default of camelization for backwards compatibility. I now believe the typeForRoot that I used in #2766 was a mistake.

Overall I like this pr. I wanted to bring up a that one of @tomdale's reasons for suggesting Ember Data issues a deprecation warning when it detects caps (that may not have been well communicated in the comment in #2766) was the fear that users would have to change their type argument the store.find call when migrating from globals to the Ember CLI resolver.

mixonic · 2015-02-26T02:25:32Z

@bmac I believe a user can move app code between globals and Ember-CLI smoothly today. The default ember application container normalization should be sufficient.

Moving unit tests is the mess: There is no normalization, but the model names in the container have changed. Thus you end up being forced into dasherized format in unit tests.

I'll add rootForType to this PR.

mixonic · 2015-02-26T04:50:08Z

Slight change of plans: I did not implement a rootForType, as it is would only be used in serializeIntoHash. Instead I just camelize in serializeIntoHash.

Need to add a test for new serializeIntoHash behavior. It should assert that the current behavior in Ember-CLI (that a payload namespace is camelCased) occurs even when dasherized names are used in the container.

Did some cleanup of embedded records mixin. The tests make a lot of assumptions about camelCasing. Need to add some for serialization.

mixonic · 2015-02-28T04:08:58Z

More tests
Introduce typeForPayload in the JSONSerializer. I added it to the JSONSerializer because embedded records mixin is tested against that serializer, and this method is required for polymorphic serialization. typeForPayload allows us to not leak the typeKey. An Ember-CLI app (with dasherized keys) and a globals app (with camelCase keys) will send the same polymorphic type string to the server via typeForPayload (camelCase by default for backwards compat).
Importantly, I believe this PR changes nothing about how polymorphic type are sent to the server. It also changes nothing about how mixin keys are read from response payloads. So it should be backwards compatible.

@bmac @igorT @stefanpenner I'd appreciate a look. I think this is good to go.

fivetanley · 2015-02-28T04:55:03Z

@mixonic I think @tomdale and @wycats thought this kind of logic should always delegate to a Resolver. Am I way off track here tomhuda?

First, remember that isolated containers have no normalization. When testing with isolated containers, the user-entered model key becomes the container object name as well as the type key. For example, in a unit test this modelFor works like this: * `modelFor("SomeItem")` * Looks for `"model:SomeItem"` in the container * This is normalized to `"model:SomeItem"` (unit tests have no normalization) * The object is returned. In Ember-CLI, a error. In globals Ember, `App.SomeItem`. * The typeKey is set to the normalized value: `SomeItem` In an acceptance or app environment the container has a different resolver. In these cases normalization changes things. For example, with the Ember-CLI resolver: * `modelFor("SomeItem")` * Looks for `"model:SomeItem"` in the container * This is normalized to `"model:some-item"` (Ember-CLI is dasherizing) * The object is returned. In Ember-CLI, `app/models/some-item`. * The typeKey is set to the normalized value: `some-item` What must be added to this confusing situation is that RESTAdapter defers to the `typeKey` for model keys. If you change your resolver, you will change the requests sent to your server and the way data is parsed.

mixonic · 2015-03-07T22:07:10Z

This is rebased and passing. Waiting on a +1 or further discussion.

bmac · 2015-03-07T23:54:09Z

packages/ember-data/lib/serializers/rest_serializer.js

+    import DS from "ember-data";
+    export default DS.RESTSerializer.extend({
+      typeForRoot: function(root) {
+        return subRoot.singularize().dasherize();


Does this mean Ember CLI users will still need to use a custom Serializer after this pr is merged?

I want to stay really specific here. I don't think most devs know that a dasherizing typeForRoot can make their ember-cli unit tests work properly (with multi-word model names). The current code will pass a camelized name to the store (and then container, where there is no normalization), and not find the model. That behavior is not changed in this PR.

I would recommend that they use a singularize then dasherize typeForRoot, yeah.

do conventional API's return dasherized responses?

mixonic · 2015-03-13T17:49:33Z

We discussed this a bit at the ember-data meeting.

Let's introduce a property on classes called modelName, which is always dasherized
Let's deprecate getting typeKey
We need both typeForRoot and payloadForType, though perhaps the names need more bikeshedding (modelNameFromPayload, modelNameForPayload I suggest)
Before the use-dashes-everywhere work can land Ember's default container, the isolated container, and the Ember-CLI container must raise a warning or deprecation notice for camelCase lookups.

stefanpenner · 2015-03-13T17:55:25Z

Let's introduce a property on classes called modelName, which is always dasherized

can you also explain what this property will be used for.

Before the use-dashes-everywhere work can land Ember's default container, the isolated container, and the Ember-CLI container must raise a warning or deprecation notice for camelCase lookups.

only the resolver needs these deprecation warnings

mixonic · 2015-03-13T18:06:40Z

can you also explain what this property will be used for.

It is the new public interface for model names. Always dasherized, which matches our conventions across the board (once the deprecations are added to the resolvers)

only the resolver needs these deprecation warnings

Ah, so the normalization is different in the three places I listed. It seems correct that the resovler may be the only things that needs to throw, but the normalization should probably be reviewed.

stefanpenner · 2015-03-13T18:12:24Z

It is the new public interface for model names. Always dasherized, which matches our conventions across the board (once the deprecations are added to the resolvers)

What are the public usages of this api? (i am just curious why a name placed on a factory is ever needed)

stefanpenner · 2015-03-13T18:12:44Z

Ah, so the normalization is different in the three places I listed. It seems correct that the resovler may be the only things that needs to throw, but the normalization should probably be reviewed.

all normalization must go through the resolver

mixonic · 2015-03-13T18:23:31Z

What are the public usages of this api? (i am just curious why a name placed on a factory is ever needed)

when building a url in the adapter. during serialization of a polymorphic model. when determining the root property name of a create payload.

all normalization must go through the resolver

This has changed recently. Looks like you're right. I see at least these:

normalize on the default resolver https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/system/resolver.js#L119
normalize on the ember-cli resolver https://github.com/ember-cli/ember-resolver/blob/master/packages/ember-resolver/lib/core.js#L139

So "the resolver" translates back to my original statement. "Ember's default container, the isolated container, and the Ember-CLI container must raise a warning or deprecation notice for camelCase lookups."

It seems a non sequitur to raise that normalization goes through the resolver. The behavior still needs to change in three situations, correct?

stefanpenner · 2015-03-13T20:04:28Z

So "the resolver" translates back to my original statement. "Ember's default container, the isolated container, and the Ember-CLI container must raise a warning or deprecation notice for camelCase lookups."

i want to clarify further, to make sure we remove all mis-behaving code. The resolver is the only thing that knows about naming conventions, and as such it is the only place we should put these deprecations. With one exception, and that is plural/singlular inflections. If other exceptions exist, I strongly suspect that is demonstrating a bug, that we must resolve.

stefanpenner · 2015-03-13T20:05:43Z

packages/ember-data/lib/serializers/json_serializer.js

+    @return {String} the model's name in your request payloads
+  */
+  typeForPayload: function(key) {
+    return camelize(singularize(key));


this is plucking from the payload correct?

fivetanley · 2015-03-13T20:20:48Z

only the resolver needs these deprecation warnings

The idea here is that you shouldn't be able to register anything in the container that is not dasherized either.

stefanpenner · 2015-03-13T22:20:10Z

The idea here is that you shouldn't be able to register anything in the container that is not dasherized either.

registration itself is some-what of an anti-pattern, most app scenarios should ideally be resolved.

cah-brian-gantzler · 2015-05-29T18:58:26Z

Each version of Ember data that comes out I check to see if perhaps this has been completed :) Any idea on when this may make it into a beta? 1.0 is coming quickly and hoping this does not get deferred to 2.x I currently can not use the mock http facility and would very much like to.

cah-brian-gantzler · 2015-05-29T19:42:51Z

Looking at the code more closely, this particular problem is indeed fixed.(Tried beta.18, could be fixed earlier though) The correct adapter is being called. My test is still failing because the http-mock is not being installed. I have found that the tests work using http:/localhost:4200/tests but does not work with ember test -s. Any ideas?

mixonic · 2015-05-29T19:44:13Z

I believe much of this has been addressed in beta.18 in other PRs by @fivetanley (thank you sir). I'm closing, though I'd like to port my tests from here forward to master now that his code has landed.

fivetanley · 2015-05-29T20:11:41Z

@mixonic much of the code left is in #3074

cah-brian-gantzler · 2015-05-31T14:57:48Z

Are mocks suppose to be installed when running ember test -s?

bmac · 2015-06-01T13:44:54Z

@cah-briangantzler I asked @rwjblue about mocks with ember test -s and he reports that they currently do not work.

mixonic reviewed Feb 25, 2015
View reviewed changes

mixonic force-pushed the normalize-type-key branch from 062cd51 to 6e22154 Compare February 25, 2015 21:08

mixonic force-pushed the normalize-type-key branch 3 times, most recently from 2a42c50 to 8165ff7 Compare February 26, 2015 04:44

mixonic force-pushed the normalize-type-key branch from 8165ff7 to 3f590b5 Compare February 28, 2015 04:00

bmac added this to the 1.0.0-beta.16 milestone Feb 28, 2015

mixonic added 4 commits March 7, 2015 17:02

Cleaning up typeKey usage, testing

561eef6

Introduce typeForPayload

d051cbf

Fix some code style

e038e41

mixonic force-pushed the normalize-type-key branch from 468ad0f to e038e41 Compare March 7, 2015 22:02

bmac reviewed Mar 7, 2015
View reviewed changes

bmac added the team-review label Mar 11, 2015

stefanpenner reviewed Mar 13, 2015
View reviewed changes

bmac removed the team-review label Mar 15, 2015

wecc mentioned this pull request Mar 21, 2015

Implement basic JSON-API adapter #2904

Merged

bmac removed this from the 1.0.0-beta.16 milestone Mar 22, 2015

fivetanley mentioned this pull request Apr 29, 2015

introduce modelNameFromPayloadKey and deprecate typeForRoot #3034

Merged

mixonic closed this May 29, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change _normalizeTypeKey to defer normalization to the container #2821

Change _normalizeTypeKey to defer normalization to the container #2821

mixonic commented Feb 25, 2015

mixonic Feb 25, 2015

bmac commented Feb 26, 2015

mixonic commented Feb 26, 2015

mixonic commented Feb 26, 2015

mixonic commented Feb 28, 2015

fivetanley commented Feb 28, 2015

mixonic commented Mar 7, 2015

bmac Mar 7, 2015

mixonic Mar 8, 2015

stefanpenner Mar 13, 2015

mixonic commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

mixonic commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

mixonic commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

stefanpenner Mar 13, 2015

fivetanley commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

cah-brian-gantzler commented May 29, 2015

cah-brian-gantzler commented May 29, 2015

mixonic commented May 29, 2015

fivetanley commented May 29, 2015

cah-brian-gantzler commented May 31, 2015

bmac commented Jun 1, 2015

Change _normalizeTypeKey to defer normalization to the container #2821

Change _normalizeTypeKey to defer normalization to the container #2821

Conversation

mixonic commented Feb 25, 2015

mixonic Feb 25, 2015

Choose a reason for hiding this comment

bmac commented Feb 26, 2015

mixonic commented Feb 26, 2015

mixonic commented Feb 26, 2015

mixonic commented Feb 28, 2015

fivetanley commented Feb 28, 2015

mixonic commented Mar 7, 2015

bmac Mar 7, 2015

Choose a reason for hiding this comment

mixonic Mar 8, 2015

Choose a reason for hiding this comment

stefanpenner Mar 13, 2015

Choose a reason for hiding this comment

mixonic commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

mixonic commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

mixonic commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

stefanpenner Mar 13, 2015

Choose a reason for hiding this comment

fivetanley commented Mar 13, 2015

stefanpenner commented Mar 13, 2015

cah-brian-gantzler commented May 29, 2015

cah-brian-gantzler commented May 29, 2015

mixonic commented May 29, 2015

fivetanley commented May 29, 2015

cah-brian-gantzler commented May 31, 2015

bmac commented Jun 1, 2015