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

Latest change in 2.7.4 breaks models that have an attribute type #233

Closed
YoranBrondsema opened this issue Aug 23, 2016 · 8 comments
Closed

Comments

@YoranBrondsema
Copy link
Contributor

We have a non-polymorphic model that has an attribute type. This was working fine on 2.7.3 but since 2.7.4 (this commit in particular) the tests fail as it's trying to create a model of type type rather than the generic model with an attribute type.

More concretely, we have a model user that has an attribute type:

DS.Model.extend({
  type: DS.attr('string')
});

We then have factories for several types. So

FactoryGuy.define('user', {
  teacher: {
    type: 'teacher'
  },
  student: {
    type: 'student'
  }
});

In 2.7.3, doing make('teacher') resulted in creating a record of type user with the type attribute equal to teacher.

In 2.7.4, doing make('teacher') results in creating a record of type teacher (which fails because there's no such model).

There's probably something to be said about our choice of the attribute name type. But I think explicitly indicating a factory as polymorphic could provide a way to make a distinction between the two cases and act accordingly.

@danielspaniel
Copy link
Collaborator

danielspaniel commented Aug 23, 2016

I have been waiting for a complaint about this issue for a long time. As you said, the latest commit forced the issue. There is a way to fix it ( I think ) .. but give me a moment to do that.

With rails backed the type attribute is used for holding the class name of polymorphic types, so you can't use the type attribute. Therefore I am guessing you are not using rails as the backend.

I think it is problably better not to use type as an attribute in general ( since json api uses type for the type ) .. but I will see if I can fix this anyway.

@YoranBrondsema
Copy link
Contributor Author

Actually we do use Rails on the back-end :-). And there it's a polymorphic model. However, to keep the front-end model architecture simpler, we decided not to use the polymorphism there.

@danielspaniel
Copy link
Collaborator

Hmm .. wow .. that is interesting.

Not sure actually how to fix this issue, since I can't really force people to say this factory is polymorphic ( can I ? ) .. seems pretty harsh but maybe it's not too bad? Otherwise there is no way to tell if that type attibute is used for the polymorphic type or not .. any thoughts?

@YoranBrondsema
Copy link
Contributor Author

That's what I was thinking about too. Polymorphism is only enabled by your latest commit right?

That would be the most "flexible" in terms of allowing everyone to configure their app to work with ember-data-factory-guy. I agree that our choice of naming causes confusion but we're technically not doing anything "wrong" wrt the JSON-API standard and thus Ember Data.

@danielspaniel
Copy link
Collaborator

Polymorphism has been enabled forever in factory guy. I just always based everything on the assumption that no one would use the type attribute.
So, let me ponder what to do about this. And yeah, I guess your not doing anything wrong, just bringing up a tricky issue.

@danielspaniel
Copy link
Collaborator

How about it I make it so you flag your factory as NOT polymorphic

polymorphic: false

when you are using type as attribute

That was it is backwards compatible. And it is the exception anyway, so ?? what do you think about that.

@YoranBrondsema
Copy link
Contributor Author

That sounds like a good solution to me!

@danielspaniel
Copy link
Collaborator

fixed in v2.7.5 ( but let me know if otherwise )

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

No branches or pull requests

2 participants