Choose strategy for looking up async hasMany relationships about ember-django-adapter HOT 15 CLOSED

I have a potential solution for this that uses django-filter and query params. I need to develop it a little more before I make a PR for discussion.

from ember-django-adapter.

@benkonrath I have built a solution on the DRF side in my adapter here https://github.com/g-cassie/ember-drf/blob/master/ember_drf/filters.py

Are you building something similar?

from ember-django-adapter.

nice filter @g-cassie .. you could also do it right on the viewset:

def get_queryset(self):
    queryset = MyModel.objects.all()
    ids = self.request.QUERY_PARAMS.getlist('ids[]')
    if ids:
        queryset = queryset.filter(id__in=ids)
    return queryset

but the filter is probably better as it can be easily applied to the entire project.

either way, i like this better than adding the extra endpoint for async hasMany relationships.

from ember-django-adapter.

From personal experience, I came to the conclusion is best to avoid nested endpoints and have only flat (it's much easier to maintain when you have complicated data models)

I recall that in the original adapter, was kind of a pain having to maintain endpoints for nested relations in order to support async lookups (I opened issue 55 to point this out back then). So the philosophy was to assume something about the backend.

This philosophy can still be maintained, but this time it would be less demanding on the API. @g-cassie filter looks like an elegant solution and easy to add to a current API.

Looking this from a user of the adapter perspective, what do you think of this?: if the coalesce option is set to true, we print a warning that the DRF doesn't not support it by default, but can be easily modified to do so, and point to some docs to do that. then it's safe to assume the backend will support it and we can use it.

from ember-django-adapter.

I think there should be a unified front on this. Taken together, @g-cassie's emberdrf package and the adapter connect all the dots to make ember/drf development a breeze.

from ember-django-adapter.

Agree that we should unify. I think we should stick with separate repos for python/js code to make package manager config easier. I think what I have in my repo is more a collection of tools that you can use to get going with Ember Data. One of those is the aforementioned filter. If you use them all together you end up with a full adapter solution but there is nothing saying you have to. I can rework the readme to reflect this concept and direct people to this project if they want to use an Ember side adapter and just cherry pick a few tools out of my project.

On another note, I should highlight that there is an issue with trailing slashes on urls and the coalesceFindIds option. I opened emberjs/data#2468 and just received a green light. Unfortunately I since reworked my own project to not use trailing slashes so this isn't a huge priority for me. I will try to fix up my PR for merge by the end of the week but if anyone needs to move faster feel free to jump in.

from ember-django-adapter.

My idea is to use query params and a django-filter to solve this. It's probably best to just give and example to demonstrate my proposed setup.

ED models:

App.ProductGroup = DS.Model.extend({
  comments: DS.hasMany('comments', {async: true})
});

App.ProductAssociation = DS.Model.extend({
   post: DS.belongsTo('post', {async: true})
 });

Django models:

class Post(models.Model):
    pass

class Comment(models.Model):
    post = models.ForeignKey('Post', related_name='comments')

DRF viewsets:

PostViewSet(ModelViewSet)
    pass

CommentViewSet(ModelViewSet)
    filter_fields = ('post',)

We could setup the adapter to make requests to /api/comments/?post=1 when getting the comments for post 1. I've already used this strategy with the old adapter by overriding extractDjangoPayload for hasMany relationships. I have something sort of working but I need to spend some time to make it generic and add tests.

The nice thing about this is the solution is easy and it's not nested which I also think is more difficult to work with. We can always have both setups as an option; coalesceFindRequests would require the solution the @g-cassie developed and we could make a new option for the queryParam / django-filter based solution. I'm just brainstorming here really but I'm curious what you guys think.

I'll more time to work on this again towards the end of this week.

from ember-django-adapter.

@benkonrath in the short run I think I prefer @g-cassie's approach the best. It is the strategy used natively by ED and it is super easy to implement in DRF without any additional libraries.

The problem with it is that, from a pure API standpoint, it is kind of messy. That's why Toran's original solution was so nice—it looked great in API documentation, easy to understand, easy to consume elsewhere besides Ember.

I don't think re-implementing Toran's solution is a real priority for now, but since it sounds like you've made a lot of progress with the filter solution, I'm more than happy to get the necessary adapter components merged in to make it work. If it's a "one or the other" type thing between the filter solution and the Ember ids array solution, we'll have to add a config option for the user. The default option is something we can discuss when we get to that bridge.

from ember-django-adapter.

@dustinfarris Yep, I'm on board for using @g-cassie's solution. I still like the filter solution and I think that I can write some time documentation to make it easy to consume. But I also can recognize that I'm a little biased here.

It's just practical to use a solution that is already working correctly, is documented and supports ED's default behaviour. @g-cassie Thanks for working hard on the ember-drf project and for sharing your solution here.

The only real thing that needs to be done now is to remove the check for coalesceFindRequests and add document how to setup DRF for coalesceFindRequests.

https://github.com/dustinfarris/ember-django-adapter/blob/version-1.0/addon/adapters/drf.js#L22

from ember-django-adapter.

@dustinfarris @benkonrath Glad my project can be used as part of the Ember Data adapter story.

Just want to bring up emberjs/data#2468 which I think will still be an outstanding issue for you guys. tl;dr; coalesceFindRequests does not play nice with trailing slashes on urls. I am totally swamped right now so don't have the time to put together a test for my PR and bring it across the finish line but happy for someone else to piggyback off my PR and get it done.

from ember-django-adapter.

Ah, thanks for the heads-up. Since we plan on supporting coalesceFindRequests out of the gate, it looks like we'll have to recommend that users turn off trailing slashes in their DRF projects until your ED pull request is merged, e.g.:

from rest_framework.routers import DefaultRouter

router = DefaultRouter(trailing_slash=False)

from ember-django-adapter.

@dustinfarris I propose we bump this to the 1.1 milestone that I just created for a February release. It's going to be more work to solve than we thought and I think it would be nice to get something released for people to start using.

It would be even better to do a 0.9 release on 9 January and then the 1.0 on 6 February. The current 1.0 milestone would be renamed to 0.9 and the current 1.1 milestone would be renamed to 1.0. The 0.9 indicates that we're quite finished but it's still something for people to use. What do you think?

from ember-django-adapter.

Agreed. I've actually been second-guessing the "1.0" versioning myself. I think I would prefer our "1.0" to occur shortly after the final release of Ember Data 1.0.

Let's just bump the minor version to 0.5 (the next step), and then we can do micro-releases as we get the features in order and as Ember Data solidifies in this final stage.

I'm happy to release 0.5 immediately, even without #38, so that we can move the development to master. Agree?

from ember-django-adapter.

Yeah, sounds good. What do you think about merging #42 before the 0.5 release?

from ember-django-adapter.

Merged

from ember-django-adapter.