miragejs / ember-cli-mirage

An Ember Addon to easily add Mirage JS to your Ember app.
http://ember-cli-mirage.com
MIT License
863 stars 439 forks source link

Models #69

Closed samselikoff closed 8 years ago

samselikoff commented 9 years ago

Follow progress in #82

[notes to clarify my thoughts and for posterity, but anyone feel free to jump in]

I think we need to add collections to mirage - essentially, a model layer - because of associations. Here's why.

If you're just using fixtures, fine, you can manage related IDs yourself and return whatever you want. But this becomes a hassle, couples your tests to external files, etc. Lots of people want something simpler.

So, we have factories. But, factories are supposed to define the minimum attributes necessary to make your model valid, as well as attributes for other common scenarios.

So you go to define a contact factory and an address factory, and you say "for a contact to be valid, it also needs an address". You specify this in your factory:

// factories/contact.js
export default Mirage.Factory.extend({
  name: 'Sam',
  address: Mirage.association()
});

The thing is, when you go to .create() a contact, you need to create the associated address. This means building the attrs from the factory, as well as relating the two models via some id within Mirage's database. Essentially, we need to know where the foreign key lives, so in your route handlers you can fetch the related models.

Okay, so let's say a contact hasOne address. We could specify this in the factory...

export default Mirage.Factory.extend({
  name: 'Sam',
- address: Mirage.association()
+ address: Mirage.hasOne()

but then, users would need to ensure all their relationship metadata is in their factories. What if this doesn't match up with their business requirements regarding what makes their models valid? For example, what if a contact didn't need an address to be valid, and so the factory didn't have the association in its definition? In this case, we still want the user to be able to do

var contact = server.create('contact');
var address = server.create('address', {contact: contact});

in their tests. But if the association information isn't in the factory, how will Mirage know to store the foreign key on the addresses collection (i.e., to add a contact_id field to each address in the database), rather than on the contacts collection? It won't.

So, factories aren't a good place to encode relationship information.

Another possibility is in the upcoming serializer layer. We need serializers because, we want users to be able to say from a route handler, "respond with this contact", and then for that contact to go through a serializer, where they decide which attrs to expose, whether to include a root key, what relationships to include etc. (similar to ActiveModelSerializers).

So, we could require users to add the relationship metadata to the serializers. But, this is an unnecessary coupling as well. What if you don't want to include related addresses in your default ContactSerializer. You shouldn't have to, just to let Mirage know about your relationships. Users will also want to be able to specify multiple serializers for different scenarios - for example, to show a small subset of attrs for a summary overview route, and then all attrs + related models for a detail route. Deciphering the relationship metadata from these various serializers seems intractable.

In factory_girl, you specify an associated factory by referencing its name. The thing is, factory_girl is building ActiveRecord objects. The association information is encoded in your ActiveRecord model definitions - has_many, belongs_to, etc. In Mirage, we don't have a model layer. So, I think the solution is, create one.

It's going to be very simple, where you essentially declare what your Mirage collections are, and you specify where the foreign keys are:

// mirage/collections.js
export default function() {
  this.collection('contacts');
  this.collection('addresses', {belongsTo: 'contacts'});
}

Not sure what the API will be, but anyway, something like this.

With this information, we can give users a consistent answer about where they expect to find related models in the db object in their route handlers, as well as make serializers + factories much simpler to work with.

samselikoff commented 9 years ago

is this going to lead to full-blown models? validations? etc.

samselikoff commented 9 years ago

I need models to (1) identify which serializer(s) to use when models are returned from a route handler, (2) extract relationship information (where the foreign key is), and possibly more.

I need an ORM for the client. Any ideas?

samselikoff commented 9 years ago

I'm going to write a lightweight model layer.

/*
  Create
*/
var user = schema.user.new({name: 'Link'}); // user is an unsaved model
user.save()

// or directly to database
schema.user.create({name: 'Link'});

/*
  Read 
*/
schema.user.all();
schema.user.find(1);
schema.user.find([1, 2, 3]);
schema.user.where({admin: true});

user.name; // Link
user.attrs;  // {name: 'Link', age: 100, good: true}

/*
  Update
*/
// .all, .find and .where return Relations, which can be updated.
schema.user.all().update({admin: false});

// individual models can be updated as well
user.name = 'Zelda';
user.save();

// or directly to db
user.update('name', 'Zelda');
user.update({name: 'Young Zelda', admin: true});

/*
  Delete
*/
schema.user.find(1).destroy();
schema.user.find([1, 2, 3]).destroy();

user.destroy();

For example (this would be the default for the this.get('/contacts') shorthand):

this.get('/contacts', function(schema, request) {
  return schema.contact.all();
});

This way any contact returned can go through a ContactSerializer, or its relationships can be used in factories etc.

thoughts on API?

willrax commented 9 years ago

This looks awesome to me. This part in particular looks great:

// .all, .find and .where return Relations, which can be updated. schema.user.all().update({admin: false});

Would this replace this type of code in tests:

tag = server.create('tag');
project = server.create('project', { tagIds: [1] });

Basically removing the assumption of the id.

samselikoff commented 9 years ago

Yep exactly, that's where this all came from. Originally it was, "lets add relationship support to factories", and now I'm writing an ORM. If I do it right, though, it should make things simpler for end users, not more complex.

End goal is for users to never have to worry about linking up ids with their factories. After it works with factories in tests, I want to make it so you can setup a default scenario for development using your factories. At that point we'll probably encourage people to get rid of their fixtures (just bc ids are so annoying to manage).

samselikoff commented 9 years ago

Issues:

Is this even a problem?

cibernox commented 9 years ago

I do like this idea, but maybe we can start with something simpler: Traits/Hooks

I've found myself having to set up quite a lot of records in order to have a usable state in my tests.

P.e,

    student = server.create('student');
    quizQuestions = [
      server.create('quiz_question', { description: 'Favorite color', correct_answer: 'Red' }),
      server.create('quiz_question', { description: 'Favorite movie', correct_answer: 'Lion king' }),
      server.create('quiz_question', { description: 'Favorite song', correct_answer: 'Funky town' })
    ];
    quiz = server.create('quiz', { question_ids: quizQuestions.map(q => q.id) });
    quizSubmissionQuestions = quizQuestions.map(function(qq) {
      return server.create('quiz_submission_question')
    });
    quizSubmission = server.create('quiz_submission', {
      quiz_id: quiz.id,
      student_id: student.id
    });

I'd like to have something similar to traits and transient attributes to be able to be able to write something like this:

quiz = server.create('quiz', { questionsCount: 3 });
quizSubmission = server.create('quiz_submission', ['withStudent'], { quiz_id: quiz.id });

The simplest approach I can think about without making any special syntax would be just to provide an init hook where you can customize whatever you want:

// app/mirage/factories/quiz.js
import Mirage from 'ember-cli-mirage';

export default Mirage.Factory.extend({
  // Some attributes
  time_limit: 30,
  random_order: true,

  init(attributes) {
    if (attributes.hasOwnProperty('questionsCount')) {
      attributes.question_ids = [];
      for (let i = 0; i < questionsCount; i++) {
        attributes.question_ids .push(this.server.create('quiz_question').id);
      }
      delete attributes.questionsCount;
    }
    super(attributes)
  }
});

I'm not sure about the idea of the traits, but I do think that mirage should provide some mechanism, even if it's just a init hook to allow you to highjack record initialization and take decissions based on the arguments.

samselikoff commented 9 years ago

not a bad idea. Of course you still have to manage ids which is one of the main points of the model layer, but I understand that it's a stop-gap measure for the short term.

the thing is schema and belongsTo works, I just need to finish hasMany and then this wont be far away. hesitant to implement this bc it will mean ppl implementing a lot of custom code, which should really be part of the lib.

i may accept a PR though? I'll keep thinking about this

samselikoff commented 9 years ago

and btw, traits will definitely come shortly after relationships, so you can do the kinds of things youre describing

cibernox commented 9 years ago

I don't know if I'll find enough spare time for a PR that big anytime soon.

I remember you mentioned that you didn't wanted to use much ember-specific stuff because this addon is really generally useful for everybody. One of those steps would move away from the ember object model (p.e. Mirage.Fixture.extend({})) towards ES6 classes and regular extend sentences (class User extends Mirage.Model {}).

If that is accomplished you'd have a constructor() method and super avaliable for free. Seems like a free win.

samselikoff commented 9 years ago

I absolutely want to do this but, I think we still need a .extend method if the lib was used outside of an ES6 project. right?

cibernox commented 9 years ago

Yes, although I see it as a natural tradeof: User egonomics vs implementer ergonomics.

You might make things more complicated for people that it's not using a transpiler in exchange of doing the code more maintenable and aligned with the standards.

I think that most people that cares enough about good practices/decoupling use mirage to mock a backend during development and tests will be already using a transpiler. Probably it's already very painful to use the library as is without a ES6 module transpiler.

Users not using ES6 can still use regular prototype inheritance anyway if they really want to:

  function User(attributes) {
    Model.call(this, attributes);
  }

  User.prototype = Object.create(Model.prototype, { 
    constructor: { 
      value: User, 
      enumerable: false, 
      writable: true, 
      configurable: true 
    } 
  }); 
  User.__proto__ = Model; 

  module.export = User;
g-cassie commented 9 years ago

Just had a read through this and had a couple thoughts.

I don't think it's necessary to support auto-creating hasMany relationships in factories. hasMany relationships should always be optional. I think that it is totally acceptable to have to write this:

var basket = server.create('basket');
server.create('fruit', 3, {basket: basket});

If you are doing this a lot you could support a postCreate hook in the factory class.

// factories/basket-with-fruit.js
export default Mirage.Factory.extend({
  // ...
  postBuild(server, newObj){
    server.create('fruit', 3, {basket: newObj});
  }
})

On the traits thing mentioned above - I think you can keep the api simpler by encouraging this customization using subclassed factories instead of adding a traits argument. I might not be understanding fully what the traits are supposed to do.

I love the idea of replacing fixtures with factories.

cibernox commented 9 years ago

@g-cassie Just to clarify, factories are not a replacement to fixtures. Right now both exist and have different use cases. Fixtures for development, factories for tests.

In development I have a very large and complex set of fixtures (7K+ LOC) to replicate a lot of different scenarios. It helps me develop at my own pace regardless of how fast or slow is the backend being developed (in fact my colleagues look the fixtures as fixtures I've created to validate the API).

g-cassie commented 9 years ago

@cibernox Yeah "replacement" was the wrong word. What I understand is that you will be able to use factories to define fixtures so that you can keep them a lot more concise and get rid of the headaches associated with managing relationships in your fixture files. Is that correct?

samselikoff commented 9 years ago

Indeed fixtures will always be supported, but once models land I think factories will be a much simpler way to create object graphs for development. I want to have a route that can essentially show the fixture output for a set of factory build commands.

Yes Gordon that's exactly right, I basically think when seeding your database you shouldn't have to think about ids at all. On Fri, May 15, 2015 at 9:51 AM Miguel Camba notifications@github.com wrote:

@g-cassie https://github.com/g-cassie Just to clarify, factories are not a replacement to fixtures. Right now both exist and have different use cases. Fixtures for development, factories for tests.

In development I have a very large and complex set of fixtures (7K+ LOC) to replicate a lot of different scenarios. It helps me develop at my own pace regardless of how fast or slow is the backend being developed (in fact my colleagues look the fixtures as fixtures I've created to validate the API).

— Reply to this email directly or view it on GitHub https://github.com/samselikoff/ember-cli-mirage/issues/69#issuecomment-102403562 .

samselikoff commented 9 years ago

@g-cassie the hasMany relationships won't be created automatically, only if you add them to your factories.

e.g.

// app/mirage/factories/user.js
import Mirage from 'ember-cli-mirage';

export default Mirage.Factory.extend({
  name: i => `User ${i}`,
  addresses: Mirage.hasMany({count: 5})
});

Traits will allow us to have different versions here

// app/mirage/factories/user.js
import Mirage from 'ember-cli-mirage';

export default Mirage.Factory.extend({
  name: i => `User ${i}`,

  withAddresses: {
    addresses: Mirage.hasMany({count: 5}),
    another: 'attr'
  }
});

server.create('user'); // just the user
server.create('user', 'withAddresses'); //user and addresses

There will also be an api for changing the count directly from the server.create method, similar to transient attrs in factory girl

g-cassie commented 9 years ago

@samselikoff Ok, that makes sense. My point is mainly that it might not be worth adding complexity in order to support that kind of functionality. Given that mirage is mocking up the backend I think it would be fair (maybe even helpful) for it to have similar design constraints to a typical relational database (i.e. all relationships are stored as foreignKeys - hasMany relationships are computed). Granted I am really not familiar with the implementation of mirage so this might not make any sense at all.

samselikoff commented 9 years ago

No you're exactly right, this is exactly how it's being implemented. The trait in the factory is just a succinct way to generate object graphs. Do you work with rails at all? The API is inspired by factory_girl. You can check out their associations guide to see how they do it, but under the hood it uses ActiveRecord models (which rely on foreign keys to link associations).

g-cassie commented 9 years ago

Ok that makes sense. I work in Django which has FactoryBoy. Looking at the factory_girl docs the api is actually quite a bit different which is probably why I'm getting a little confused here. Overall I think they have the same functionality. Thanks for entertaining my questions!

samselikoff commented 9 years ago

Of course! That's why I wanted this issue in the open + I appreciate you taking time to read it and provide feedback.

Out of curiosity, how would you create an object graph in FactoryBoy?

g-cassie commented 9 years ago

Here's a sketch of a typical usage of factory boy

# import FactoryBoy library
import factory
# import your Django model definitions
from myapp.models import Fruit, Basket

# define a couple factories
class BasketFactory(Factory):
    FACTORY_FOR = Basket
    title = "Fruit Basket"

class FruitFactory(Factory):
    FACTORY_FOR = Fruit
    title = "Orange"
    basket = factory.SubFactory(BasketFactory)

# some_test.py
basket = BasketFactory(title="Special Title")
fruit1 = FruitFactory(basket=basket)
fruit2 = FruitFactory(basket__title="Some other basket")
fruit1.basket != fruit2.basket  # evaluates to True

fruit3 = FruitFactory(basket=fruit2.basket)
fruit3.basket == fruit2.basket  # evaluates to True

# creating child objects in a one-to-many can be hacked, but best practice is this
new_basket = BasketFactory()
fruits = FruitFactory.create_batch(3, basket=new_basket)

# this is something helpful that might be unique to FactoryBoy?
class CompanyFactory(Factory):
    country = "United Sates"

class PersonFactory(Factory):
    country = factory.Iterator(["France", "Italy", "Spain"])
    company = factory.SubFactory(CompanyFactory(country=factory.SelfAttribute('..country'))

person = PersonFactory()
person.company.country == person.country  # evaluates to True
buschtoens commented 9 years ago

What's the status of this feature? Is it usable, but not yet documented? The current release contains the ORM code.

samselikoff commented 9 years ago

Yes, merged but not ready (deliberately not documented). I know it's needed - I hit some snags in the serializer layer.

I'm trying to blog about things here to keep people up to date.

buschtoens commented 9 years ago

Of course I didn't think about giving the blog a read. :sweat_smile:

Thanks. :smiley:

lukeclewlow commented 9 years ago

Happy days, was just about to rewrite my serialisation and relationship layer but this saves me having to do it! Whoop, I'll pull it in and give it a try!