getoutreach / epf

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

Creating model with multiple `belongsTo` #98

Closed heartsentwined closed 10 years ago

heartsentwined commented 10 years ago

Code speaks better than I do -

Basic model, with two belongsTo relationships:

class App.Foo extends Ep.Model
class App.Bar extends Ep.Model
class App.Baz extends Ep.Model

App.Foo.reopen
  bar: Ep.belongsTo App.Bar
  baz: Ep.belongsTo App.Baz

App.Bar.reopen
  foos: Ep.hasMany App.Foo

App.Baz.reopen
  foos: Ep.hasMany App.Foo

Test:

s = App.__container__.lookup('session:main');
foo = s.create('foo');
bar = s.create('bar');
baz = s.create('baz');

foo.set('bar', bar);
foo.set('baz', baz);

console.log(foo.get('bar')); // okay, no problem
console.log(foo.get('baz')); // okay, no problem
s.flush();
POST /bars
(payload): {"bar":{"client_id":"bar63","client_rev":1}}
(response): {"bar":{"client_id":"bar63","client_rev":1,"rev":1,"id":1}}

POST /bazs
(payload): {"baz":{"client_id":"baz64","client_rev":1}}
(response): {"baz":{"client_id":"baz64","client_rev":1,"rev":1,"id":1}}

POST /foos
(payload): {"foo":{"client_id":"foo62","client_rev":1,"bar_id":1,"baz_id":0}} // baz_id !

Regarding those client_rev and rev attrs. I first observed this behavior when I echoed back only the client_id, it seemed from #89 that omitting them will "just work". But it doesn't, so I tried adding them back in, as I thought epf was replacing the models with the server payload. However apparently that isn't the problem - all four permutations (client_rev and rev echoed back, or not) produce the same observation.

I also tried dropping an empty foo_ids: [] into the bars and bazs responses, but it doesn't help either. Behavior is also the same if I use the mass setter foo.setProperties({bar: bar, baz: baz}).

Is this an epf bug, or am I doing something wrong?

heartsentwined commented 10 years ago

Update: so the collation order matters? If I swapped out bar with car, now baz_id will get set properly, but car_id will become missing.

ghempton commented 10 years ago

This should totally work, I'm very surprised it doesn't. Is there any way you could translate this code into a test?

heartsentwined commented 10 years ago

Update: this is actually a regression. git bisect gives either 6d5cac8 or f2040bd as the first breaking commit. (6d5cac8 errored elsewhere, so I can't pinpoint).

heartsentwined commented 10 years ago

But either of those breaking kinda makes sense, both commits are supposed to be "lazier" / "performance-gain" improvements - now their side-effects come up?

ghempton commented 10 years ago

Yeah I am confused as well as to why they have caused this.

heartsentwined commented 10 years ago

ah well, bisect isn't totally reliable after all; it could be elsewhere

ghempton commented 10 years ago

Is this in a child session?

heartsentwined commented 10 years ago

I'm not sure how to write out the test though, I can understand epf logic pretty much up to the model layer; once the adapter/serializer takes over, I can only resort to "non-standard" debugging. I can write a test case if those console.log() statements indicate error, but I don't know where to track down the actual missing id from the request payload.

heartsentwined commented 10 years ago

Re: child session

Makes no difference. The actual use case is in a child session, but I have reproduced this with the main session.

s = App.__container__.lookup('session:main');
ghempton commented 10 years ago

I think the test could be pretty high level, just check to see that the id is set in the payload sent down to the server. An example would be here: https://github.com/GroupTalent/epf/blob/master/test/rest/rest.acceptance.em#L55

heartsentwined commented 10 years ago

Thanks for the code sample, I'll put together a test case. What does adapter.r['POST:/users'] do though?

ghempton commented 10 years ago

adapter.r is basically just a hash that can map particular requests to simulated responses (this is a customized adapter just for test purposes). Ultimately, I think I would like to move over to sinon.js for this

heartsentwined commented 10 years ago

ok

heartsentwined commented 10 years ago

@ghempton any update?

heartsentwined commented 10 years ago

Sorry and never mind, turns out it was a 3rd party lib that's breaking epf, we tried it on a clean ember + epf environment, and it worked.

EDIT: no, it doesn't work - see comment below

ghempton commented 10 years ago

Well that is good, but I still want to get to the bottom of the test failures. I just added an npm shrinkwrap, let me know if that works. Thanks for sticking with it :)

heartsentwined commented 10 years ago

This?

{
  "name": "epf",
  "version": "0.1.4",
  "dependencies": {}
}

That won't lock down any package version at all, you need to do

$ npm install
$ npm shrinkwrap

and the output npm-shrinkwrap.json will become a giant file containing exact version and info of each dependency.

ghempton commented 10 years ago

Oh I should have looked at the output, that is exactly what I did, maybe shrinkwrap doesn't capture dev dependencies?

ghempton commented 10 years ago

Yeah, sure enough. Needed the --dev flag. Just pushed.

heartsentwined commented 10 years ago

still same result. so apparently not related to package versions?

heartsentwined commented 10 years ago

I'm terribly sorry for closing and reopening this, but we made a mistake. This is after all reproducible on a clean ember + epf - ignore my previous comment, we got tangled in a myriad of libs. So this is still an epf bug; I'll put together a fiddle.

ghempton commented 10 years ago

no worries. I will dig through the fiddle. Also, what version of node are you using?

heartsentwined commented 10 years ago

Edit: a test repo perhaps? coz I need to access (dummy) server API, and I'm not familiar enough with epf fixtures.

heartsentwined commented 10 years ago

I'm on rails, not node

heartsentwined commented 10 years ago

The code is basically the same as above, just demonstrated in a "real time" environment

ghempton commented 10 years ago

I was talking about what version of node you are using for the epf tests? I would really like to get to the bottom of your test issue. I think a failing test would be the best way for me to fix this.

heartsentwined commented 10 years ago

v0.8.9

ghempton commented 10 years ago

Ah I think that could actually be the problem, I use the latest stable version which is 0.10

heartsentwined commented 10 years ago

I'll try upgrading that

ghempton commented 10 years ago

Btw, what do you mean by "demonstrated in a real-time environment"?

heartsentwined commented 10 years ago

well nothing, just like a fiddle, making the code "executable", like you can go to the console and see the error messages for yourself

heartsentwined commented 10 years ago

perfect, upgrading node fixes the test errors

ghempton commented 10 years ago

Great, so I wonder what needs to change on the test to make it fail

heartsentwined commented 10 years ago

we now have a test repo, which just contains the code I posted in the beginning. do you still want it?

ghempton commented 10 years ago

A failing test would be ideal, but I can take a look at the repo

heartsentwined commented 10 years ago

give us a while and we'll put that on github

heartsentwined commented 10 years ago

over at heartsentwined/epf-test

heartsentwined commented 10 years ago

Only three places worth a look, the rest is boilerplate:

ghempton commented 10 years ago

Thanks, I will take a look at this later tonight

ghempton commented 10 years ago

Looking over the test code, it seems like FoosController is not returning the foreign keys for its children? Something like this is what epf expects:

class FoosController < ApplicationController
  def create
    render json: { foo: { id: 1, client_id: params[:foo][:client_id], baz_id: 1, bar_id: 1 } }, status: 201, location: foos_path
  end
  def show
  end
end
heartsentwined commented 10 years ago

That doesn't matter, because the payload itself is already corrupt.

If you insist, I would make it

class FoosController < ApplicationController
  def create
    render json: { foo: { id: 1, client_id: params[:foo][:client_id], baz_id: params[:foo][:baz_id], bar_id: params[:foo][:bar_id] } }, status: 201, location: foos_path
  end
  def show
  end
end

so as to reflect the (possibly) missing bar and baz ids.

In any case, the error happens before the foos response; it is the payload sent to this foo end point that is missing the correct baz_id.

ghempton commented 10 years ago

Is this still an issue? Things have changed a lot since this issue was opened. I am going to close for now, please reopen if there are still problems.