getoutreach / epf

A framework for keeping your Ember.js apps in sync.
http://epf.io
MIT License
369 stars 33 forks source link

Question: Embedded Documents #140

Open dev-inigmas opened 10 years ago

dev-inigmas commented 10 years ago

What is the correct (and simplest) means of notifying EPF about Embedded Documents? Earlier, I thought that I had figured it out; but now, I'm running into the follow issue:

SO.Foo = Ep.Model.extend
  ### Properties ###
  name:        Ep.attr 'string'
  description: Ep.attr 'string'

  ### Relationships ###
  bars: Ep.hasMany 'bar'

  ### Computed ###
  summary: ( ->
    @get('bars').mapProperty('name').join(', ')
  ).property('bars.@each.name')

SO.FooSerializer = SO.ApplicationSerializer.extend
  properties:
    bars: embedded: 'always'

SO.Bar = Ep.Model.extend
  ### Properties ###
  name: Ep.attr 'string'

  ## Parent Document
  foo: Ep.belongsTo 'foo', inverse: 'bars'

SO.FooIndexRoute = Ember.Route.extend
  model: -> @session.query 'foo'

When going to the "foo.index" route, I get the following:

Error while processing route: foo.index Cannot read property 'isEqual' of undefined TypeError: Cannot read property 'isEqual' of undefined
    at Ep.ModelArray.Ember.ArrayProxy.extend.removeObject (http://bebop.local:9494/dist/deps.js:73508:34)
    at apply (http://bebop.local:9494/dist/deps.js:31391:27)
    at superWrapper (http://bebop.local:9494/dist/deps.js:30969:15)
    at null.<anonymous> (http://bebop.local:9494/dist/deps.js:74282:50)
    at Ep.Model.reopen.suspendRelationshipObservers (http://bebop.local:9494/dist/deps.js:73210:37)
    at Ep.InverseManager.Ember.Object.extend._removeFromInverse (http://bebop.local:9494/dist/deps.js:74280:23)
    at Ep.InverseManager.Ember.Object.extend.unregisterRelationship (http://bebop.local:9494/dist/deps.js:74261:26)
    at null.<anonymous> (http://bebop.local:9494/dist/deps.js:74234:30)
    at Mixin.create.forEach (http://bebop.local:9494/dist/deps.js:39783:20)
    at null.<anonymous> (http://bebop.local:9494/dist/deps.js:74233:37) 

Anyone know what's going on?

Fyi - things seem to load fine when I don't specify an inverse. But then, EPF tries to POST/PUT the embedded document(s) separately.

Thanks.

ghempton commented 10 years ago

Are you using recent build from master?

ghempton commented 10 years ago

You also shouldn't need to specify an inverse in this case

ghempton commented 10 years ago

Also, what does SO.ApplicationSerializer look like?

dev-inigmas commented 10 years ago

I just checked out EPF and built it from the master branch yesterday.

I agree that I shouldn't have to specify an explicit inverse in this case. On another model, doing that seemed to prevent EPF from sending invalid POSTs/PUTs.

Here's the code:

SO.ApplicationSerializer = Ep.ActiveModelSerializer.extend()
ghempton commented 10 years ago

Can you paste an example payload?

On Thu, Jul 10, 2014 at 5:23 PM, David Patrick notifications@github.com wrote:

I just checked out EPF and built it from the master branch yesterday.

I agree that I shouldn't have to specify an explicit inverse in this case. On another model, doing that seemed to prevent EPF from sending invalid POSTs/PUTs.

Here's the code:

SO.ApplicationSerializer = Ep.ActiveModelSerializer.extend()

— Reply to this email directly or view it on GitHub https://github.com/getoutreach/epf/issues/140#issuecomment-48681404.

Gordon L. Hempton http://codebrief.com 360.460.8098

dev-inigmas commented 10 years ago

Which payload? A GET response? The POST/PUT requests?

ghempton commented 10 years ago

GET response, or both?

On Thu, Jul 10, 2014 at 5:24 PM, David Patrick notifications@github.com wrote:

Which payload? A GET response? The POST/PUT requests?

— Reply to this email directly or view it on GitHub https://github.com/getoutreach/epf/issues/140#issuecomment-48681509.

Gordon L. Hempton http://codebrief.com 360.460.8098

dev-inigmas commented 10 years ago

Seems like the explicit inverse doesn't break "findById" requests.... I'll get the responses now.

dev-inigmas commented 10 years ago

Here's a response for a single item:

{
  "race": {
    "doc_meta": {
      "created":1405024824835,
      "updated":1405029317207,
      "created_by":"53bd768f970000004057a9f4"
    },
    "id":"53befa38970000ff7257aa18",
    "name":"qwe",
    "description":"qwe",
    "abilities": [{
      "ability":"qwe",
      "description":"qwe"
    }],
    "module_id":"53bef0b0970000771f57aa17"
  }
}
ghempton commented 10 years ago

Hmm, I think this might be caused by the nested ability not having and id. Can you try passing a fixed id property just to check if this is the case? There isn't a real reason why EPF would require an id in an embedded model, but it could be a bug.

On Thu, Jul 10, 2014 at 5:31 PM, David Patrick notifications@github.com wrote:

Here's a response for a single item:

{ "race": { "doc_meta": { "created":1405024824835, "updated":1405029317207, "created_by":"53bd768f970000004057a9f4" }, "id":"53befa38970000ff7257aa18", "name":"qwe", "description":"qwe", "abilities": [{ "ability":"qwe", "description":"qwe" }], "module_id":"53bef0b0970000771f57aa17" } }

— Reply to this email directly or view it on GitHub https://github.com/getoutreach/epf/issues/140#issuecomment-48681948.

Gordon L. Hempton http://codebrief.com 360.460.8098

dev-inigmas commented 10 years ago

Here is a very truncated response from the server during a @session.query 'race':

{
  "races": [{
    "doc_meta": {
      "created":1377814485629,
      "updated":1394501556095,
      "created_by":"50c923c6e4b0add373778525"
    },
    "id":"50ea5298e4b0c6e36078e0aa",
    "name":"Android",
    "description":"Androids...",
    "abilities": [{
      "ability":"Recharge",
      "description":"During..."
    }, {
      "ability":"Unnatural",
      "description":"Arcane powers..."
    }],
    "module_id":"50c9277de4b0add373778527"
  }]
}
dev-inigmas commented 10 years ago

Putting a static "id" of 1 in there made the exceptions go away. But the abilities also went away.

ghempton commented 10 years ago

Ah so I think the issue is that the abilities have a belongsTo on them. When this is the case it epf currently expects the ability to have the parent id specified in their payload, e.g. race_id: '50ea5298e4b0c6e36078e0aa'. This definitely seems like an area that could be improved.

On Thu, Jul 10, 2014 at 5:56 PM, David Patrick notifications@github.com wrote:

Putting a static "id" of 1 in there made the exceptions go away. But the abilities also went away.

— Reply to this email directly or view it on GitHub https://github.com/getoutreach/epf/issues/140#issuecomment-48683295.

Gordon L. Hempton http://codebrief.com 360.460.8098

ghempton commented 10 years ago

Have you tried removing the belongsTo entirely from the ability?a

On Thu, Jul 10, 2014 at 5:56 PM, David Patrick notifications@github.com wrote:

Putting a static "id" of 1 in there made the exceptions go away. But the abilities also went away.

— Reply to this email directly or view it on GitHub https://github.com/getoutreach/epf/issues/140#issuecomment-48683295.

Gordon L. Hempton http://codebrief.com 360.460.8098

dev-inigmas commented 10 years ago

Yeah; then I get extra POST/PUT from EPF.

dev-inigmas commented 10 years ago

At some point, I saw a comment in the EPF that said something about embedded documents requiring inverses... I could try to find it again.

dev-inigmas commented 10 years ago

Particularly, line 49 of epf/lib/rest/embedded_manager.js

ghempton commented 10 years ago

I might have made that unnecessary, but I think the safest solution for the short term would be to include the parent ID in the embedded model. Hopefully this can be cleaned up soon.

dev-inigmas commented 10 years ago

I probably could do that.... But I have whole lot of Model types that make use of this pattern. Changing them all would be very time-consuming. Is there a slightly less safe tweak I could try out in the JavaScript, before resorting to that approach?

ghempton commented 10 years ago

You could create a custom HasMany serializer and a custom Model serializer base class.

Inside the HasMany serializer you could override the deserializeEmbedded to pass in some extra data through the options parameter to automatically set the parent in the model serializer.

On Thu, Jul 10, 2014 at 6:11 PM, David Patrick notifications@github.com wrote:

I probably could do that.... But I have whole lot of Model types that make use of this pattern. Changing them all would be very time-consuming. Is there a slightly less safe tweak I could try out in the JavaScript, before resorting to that approach?

— Reply to this email directly or view it on GitHub https://github.com/getoutreach/epf/issues/140#issuecomment-48684142.

Gordon L. Hempton http://codebrief.com 360.460.8098

dev-inigmas commented 10 years ago

Sounds good..... How do I register the custom HasMany serializer with EPF again? I know how to do it for a model. A HasMany override seems like it might be more explicit though.

ghempton commented 10 years ago

Unlike ember data, there is no distinction between a transform and a serializer (they are both just objects that have a serialize and a deserialize method) and they are looked up the same way. Looks like you aren't using EAK or ember-cli, so in your case you would just follow the right naming convention:

App.HasManySerializer = Ep.HasManySerializer.extend({...})

Cheers

dev-inigmas commented 10 years ago

(edited to provide configurable implementation and example Model definitions)

So, this seems to work:

SO.ApplicationSerializer = Ep.ActiveModelSerializer.extend
  extractProperty: (model, hash, name, type, opts) ->
    if opts?.embedded && hash.id? && hash[name]?.length > 0
      hash[name].forEach (m) =>
        m[@_prop(name)]=hash.id
    @_super(model, hash, name, type, opts)

  _prop: (name) ->
    (@configFor(name)?.inverse || @get('typeKey'))+'_id'

And here are my Models:

# Represents a sentient creature type; like Humans, Elves, or Androids
SO.Race = Ep.Model.extend
  ### Properties ###
  name:        Ep.attr 'string'
  description: Ep.attr 'string'

  ### Relationships ###
  abilities: Ep.hasMany 'race_ability'

# An effect the Race has on a character
SO.RaceAbility = Ep.Model.extend
  ### Properties ###
  ability:     Ep.attr 'string'
  description: Ep.attr 'string'
  script:      Ep.attr 'string'

  ## Parent Document
  race: Ep.belongsTo 'race', inverse: 'abilities'

SO.RaceSerializer = SO.ApplicationSerializer.extend
  properties:
    abilities:
      embedded: 'always'
      inverse:  'race'

I wasn't sure what extra data the HasManySerializer had access to that it would be able to pass in a suitable option to the ModelSerializer... So I ended up not overriding it.

See any downsides to this approach?

ghempton commented 10 years ago

Hmm at a glance it all looks like it could work out. Keep me posted as to how this plays out.