Skip to content

Relation types only take into account model names which are not always correct. #166

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

Closed
e3b0c442 opened this issue Nov 29, 2015 · 6 comments

Comments

@e3b0c442
Copy link

Example:

#models.py
class Ticket(models.Model)
    #auto pk
    handle = models.CharField(max_length=255)

class Task(models.Model)
    #auto pk
    label = models.CharField(max_length=255)
    ticket = models.ForeignKey(Ticket, blank=True, null=True)

#views.py
class TicketViewSet(viewsets.ReadOnlyModelViewSet):
    queryset = Ticket.objects.all()
    serializer_class = serializers.TicketSerializer
    resource_name = 'service-now-ticket'

class TaskViewSet(viewsets.ModelViewSet):
    queryset = Task.objects.all()
    serializer_class = serializers.TaskSerializer
    resource_name = 'task'

#serializers.py
from rest_framework_json_api import serializers

class TicketSerializer(serializers.ModelSerializer):
    class Meta:
        model = models.Ticket
        fields = (
            'id',
            'handle',
        )

class TaskSerializer(serializers.ModelSerializer):
    ticket = serializers.ResourceRelatedField(read_only=True)
    class Meta:
        model = models.Task
        field = (
            'id',
            'label',
            'ticket',
        )

When invoked, the desired output for Task is:

{
    "data": {
        "type": "task",
        "id": "126",
        "attributes": {
            "label": "This is a task"
        },
        "relationships": {
            "ticket": {
                "data": {
                    "type": "service-now-tickets",
                    "id": "91734"
                }
            }
        }
    }
}

However, the actual output is:

{
    "data": {
        "type": "task",
        "id": "126",
        "attributes": {
            "label": "This is a task"
        },
        "relationships": {
            "ticket": {
                "data": {
                    "type": "tickets",
                    "id": "91734"
                }
            }
        }
    }
}
@e3b0c442
Copy link
Author

It appears that ResourceRelatedField is calling get_resource_type_from_instance() for its type value, which bases the type solely on the model name.

I think an appropriate solution here would be to add an explicit resource_type argument to the ResourceRelatedField constructor that would be used both in output and input validation.

I'll begin work on a PR with a failing test case and fix unless there are any concerns.

@jsenecal
Copy link
Member

While your proposed solution works for your use case, another discussion in another issue/pr (sorry I'm on my mobile) described the possibility of using a custom Meta class on the models to define custom types.

@e3b0c442 e3b0c442 changed the title Relation types only take into account model names which are not already correct. Relation types only take into account model names which are not always correct. Nov 29, 2015
@e3b0c442
Copy link
Author

Aha, I see #152 .

I'm on board with that, and I think it's a more elegant way to handle it because it gets the data where it should be, on the model. It looks like the places that previously relied on _meta.model.name have been changed to point at that, as well.

I'll close this issue in favor of that solution.

@e3b0c442
Copy link
Author

I considered this a bit more this morning, and it came to me that there is another case that #152 does not cover that this type of solution may be appropriate for.

There may be instances where for various reasons multiple serializers/API endpoints refer back to the same model. An override of this type would be useful in those situations as well.

Thoughts?

@jsenecal
Copy link
Member

Proxy models?

@e3b0c442
Copy link
Author

e3b0c442 commented Dec 6, 2015

That is an elegant and simple solution that only someone who has been buried so long in his code that he forgot to come up for air can miss. 😂 Consider this closed.

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