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

WIP for has many releationships with django-filter. #58

Closed
wants to merge 1 commit into from

Conversation

benkonrath
Copy link
Collaborator

This shouldn't be merged.

if (relationship.kind === 'hasMany' && adapter.useHasManyRelatedFilterUrl) {
// FIXME This is the wrong approach. Inverse maybe?
var parentTypeKey = relationship.parentType.typeKey;
if (parentTypeKey === 'trainerProductGroup') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hard-coded for one of my projects but it needs to be generalized for it to be useful.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the relationship.kind is 'hasMany', then we could safely assume that if hash['related_name'] begins with "/" or "http://" that it is a link.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like,

if (relationship.kind === 'hasMany' && hash[payloadRelKey].match(/^\//)) {
  // Add to links ...
}

@benkonrath benkonrath self-assigned this Jan 16, 2015
@benkonrath
Copy link
Collaborator Author

I'd like to implement the hyper link fields feature. It's independent of the the django-filter solution so I can always work on that later. What do you think?

@dustinfarris
Copy link
Owner

Can you share a little of how your serializers are set up? I'm having some difficulty wrapping my head around this.

@benkonrath
Copy link
Collaborator Author

Let me give you a simplified example.

Product Group

class ProductGroup(models.Model):
    name = models.CharField(max_length=100)
    products = models.ManyToManyField('Product', related_name='product_groups')

class ProductGroupSerializer(serializers.ModelSerializer):
    class Meta:
        model = ProductGroup
        fields = ('id', 'name', 'products')
GET: /api/product_groups/23
{id: 23, name: 'Fun Products', products: [12, 44, 234]}

Product

class Product(models.Model):
    name = models.CharField(max_length=100)

class ProductSerializer(serializers.ModelSerializer):
    class Meta:
        model = ProductGroup
        fields = ('id', 'name', 'product_group')
GET: /api/products/12
{id: 12, name: 'Silly string', product_group: 23}

I think this is a pretty standard setup so far. The interesting bit is the ProductView with the DjangoFilterBackend.

class ProductViewSet(viewsets.ModelViewSet):
    serializer_class = ProductSerializer
    queryset = Product.objects.all()
    filter_backends = (filters.DjangoFilterBackend,)
    filter_fields = ('product_group',)

Now we can make a request for the Products in ProductGroup 23 with this URL:

GET: /api/products/?product_group=23
[
  {id: 12, name: 'Silly string', product_group: 23},
  {id: 44, name: 'Whoopee cushion', product_group: 23},
  {id: 234, name: 'Stink bomb', product_group: 23},
]

The code in this PR just tries to automatically build the /api/products/?product_group=23 url. But I think this is too complex to always get right (as you can see by my comment and the fact that I hard-coded some stuff).

It would be better to get DRF to generate the correct URL with a custom HyperLinkedField and just add general support for the HyperLinkedField to EDA. For example, the ProductGroup json would look like this if I wanted to use the django-filter pattern:

GET: /api/product_groups/23
{id: 23, name: 'Fun Products', products: '/api/products/?product_group=23'}

So, the next task on my TODO list for EDA is to add general support for HyperLinkedFields (without the logic to build the url with the query param). Does this sound reasonable to you?

@dustinfarris
Copy link
Owner

I am going to finish the work on emberjs/data#2468 so we can support coalesceFindRequests. Hope to do so by the end of the week.

As things are now, (if you remove the block in the adapter) I've noticed that with API_ADD_TRAILING_SLASHES set to false and with django-filter enabled on DRF, things "just work." It caught me off guard that this would work so easily.

This doesn't really relate to the hyperlinked fields discussion, but I think hasMany will get a lot easier once that pull request is finished.

@benkonrath
Copy link
Collaborator Author

I'm surprised by that as well. How does ED know to build the query param url correctly? Maybe ED is loading all of the records, not just the related records. Did you check the console api requests to see if it's loading the related or all of the data?

@dustinfarris
Copy link
Owner

I was wrong on this. You are right, ED does build the query param URL correctly, but without a filter on the DRF side, DRF returns everything. So it works, but is not efficient to say the least.

#68 clears this up by documenting how to enable coalesceFindRequests and add a custom filter to DRF.

@dustinfarris
Copy link
Owner

What do we have to do to finish this?

@benkonrath
Copy link
Collaborator Author

I actually have this almost finished in another branch. I'm back to doing Ember stuff again so I'll prioritize getting this done.

@benkonrath
Copy link
Collaborator Author

I did a bit of work on this today on another branch (with tests) and it looks like it's not going to need a custom Hyperlinked serializer field. The problem is that the hasMany relationship is created with multiple URLs instead of one URL that will limit only return the related items. For example:

Post

{
    "id": 11, 
    "title": "post title 11", 
    "body": "post body 11", 
    "comments": [
        "http://localhost:8000/api/comments/9/", 
        "http://localhost:8000/api/comments/10/", 
        "http://localhost:8000/api/comments/11/"
    ]
}

It's possible to support the belongsTo relationship in the default DRF configuration because it uses one URL. For example:

Comment

{
    "id": 9, 
    "body": "comment body 1 for post 11", 
    "post": "http://localhost:8000/api/posts/11/"
}

The next task would be to create the custom Hyperlinked serializer field that generates one URL using the DjangoFilterBackend or using embedded URLs (e.g. /api/posts/11/comments/). Is this something that you think is useful? If not, I'll just drop this and use coalesceFindRequests.

@benkonrath
Copy link
Collaborator Author

Closing this PR in favour of #95.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants