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

Use linkage objects for relationships #425

Closed
wants to merge 3 commits into from

Conversation

ctolsen
Copy link

@ctolsen ctolsen commented Apr 3, 2015

Updating relationships now uses linkage objects, as the latest JSON API requires and as discussed in #404.

I ended up with a bit of yak shaving along the way, and updated the input for the tests in test_jsonapi.py as well. I don't think I broke anything else.

Regarding test_updating.TestAssociationProxy.test_extra_info, I'm not entirely sure what this is supposed to do. Shouldn't intermediate data basically be handled as being its own resource? I can't find any mention of intermediate data in the spec, unless I missed something, which is entirely possible.

In addition, I ended up having to eliminate the dictionary passing that was done and pass a list instead. I couldn't figure out what the dictionary was supposed to do and it created some trouble. If they are meant to update the related objects somehow, can't that be done by instead filling a list and manipulating the objects directly?

Anyway, that was a good start. Happy to take a look at some of the other m2m/association proxy stuff, but I need to know what actually needs doing first 😄

@jfinkels
Copy link
Owner

jfinkels commented Apr 6, 2015

The dictionary thing was a work-in-progress solution to setting extra information on an association table. I hadn't quite gotten to implementing that part yet.

I wanted to support the behavior in test_extra_info because I think it is currently implemented in some form in Flask-Restless, and I want to maintain as much behavior as possible. See, for example, https://github.com/jfinkels/flask-restless/blob/master/tests/test_views.py#L581

@jfinkels
Copy link
Owner

jfinkels commented Apr 6, 2015

As for more association proxy issues, check out, for example, test_fetching.py, which has a failing test in the TestAssociationProxy class, because I wasn't sure exactly how to define the model. Other modules like test_updating.py have similar issues.

@ctolsen
Copy link
Author

ctolsen commented Apr 7, 2015

Regarding extra info: I understand if it's legacy, but does this mean you would show the association object flatly in the related object? For instance, say you have a user, an author, and an association object where the user can follow the author. As best I can tell, you'd show it like this:

// truncated user object
'linkage': [{
  'type': 'author'
  // local attribute
  'name': 'Ernest Hemingway'
  // on the proxy
  'user_follows': 'true'
}

Is this the case? If it is, then there are a few problems – how an object looks would depend on its context, and it would open up for potentially conflicting attribute names.

You could, perhaps, remove this from Flask-Restless entirely, and handle it with hybrid properties in SQLAlchemy, leaving it up to implementation. It would not break compatibility on the API level at least. Just a suggestion – I feel like handling this increases complexity in a way that shouldn't be necessary. I'd rather handle those things on the user object in this case anyway.

@jfinkels
Copy link
Owner

jfinkels commented Apr 7, 2015 via email

@jfinkels
Copy link
Owner

jfinkels commented Apr 8, 2015

I can fix that later though. I have squashed and pulled in your changes at 25020f9. Thanks.

@jfinkels jfinkels closed this Apr 8, 2015
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

Successfully merging this pull request may close these issues.

2 participants