UnboundLocalError in renderers.py

[daveslab]

I recently upgraded my project to Django 1.10 and to DRF JSON API 2.1.0, and I am now seeing an error every time I try to make a simple GET call on one of my resources. The error remains if I use Django 1.10 or Django 1.9. Here is a piece of the error page:

UnboundLocalError at /api/abonnes_fichier/
local variable 'relation_instance' referenced before assignment
Request Method: GET
Request URL:    http://localhost:9000/api/abonnes_fichier/
Django Version: 1.9.9
Exception Type: UnboundLocalError
Exception Value:    
local variable 'relation_instance' referenced before assignment
Exception Location: .../site-packages/rest_framework_json_api/renderers.py in extract_relationships, line 202

The error comes from the function extract_relationships in the class JSONRenderer. I'm still a newbie to Django and DRF so I'm having a hard time debugging the function. I think the problematic commit might be b96d79d.

[daveslab]

I believe the problem has to do with including reverse relationships. My resource at abonnes_fichier had its serializer thus:

class AbonneFichierSerializer(serializers.ModelSerializer):
    frequences_recu_fiscal = FrequenceRecuFiscalAbonneFichierSerializer(many=True)
    included_serializers = {
        'fichier': FichierSerializer,
        'abonne_details': AbonneDetailsSerializer,
        'abonne': AbonneSerializer,
        'frequences_recu_fiscal': FrequenceRecuFiscalAbonneFichierSerializer
    }

    class Meta:
        model = AbonneFichier

If I comment out the two lines that mention frequences_recu_fiscal then it works just fine. Note that this field comes from another model which has a ForeignKey to AbonneFichier. Here is the other model:

class FrequenceRecuFiscalAbonneFichier(Base):
    id = models.AutoField(primary_key=True)
    abonne_fichier = models.ForeignKey(AbonneFichier, 
        on_delete=models.CASCADE, 
        related_name='frequences_recu_fiscal')
    type_recu_fiscal = models.ForeignKey(TypeRecuFiscal, on_delete=models.PROTECT)
    frequence_recu_fiscal = models.ForeignKey(FrequenceRecuFiscal, on_delete=models.PROTECT)

I also noticed that in the code of the function extract_relationships that when iterating through all the fields, if a given field does not match any of these conditions

if isinstance(field, relations.HyperlinkedIdentityField):
#...
if isinstance(field, ResourceRelatedField):
#...
if isinstance(field, (relations.PrimaryKeyRelatedField, relations.HyperlinkedRelatedField)):
#...
if isinstance(field, relations.ManyRelatedField):
#...

Then on the following condition:

if isinstance(field, ListSerializer) and relation_instance is not None:

relation_instance will not have been defined already and thus the UnboundLocalError is raised.

[jerel]

@daveslab thanks for the detailed report. I believe that and relation_instance is not None on line 202 is not necessary as resource_instance (which must be what the conditional was meaning to target) is already checked on line 78. Could you remove it on your local copy and see if that resolves your issue?

[jerel]

@daveslab I'm surprised that a test didn't catch this regression. If you want to add a test to verify the fix this would be a good place for it.

[daveslab]

@jerel Thanks for responding so quickly. As you asked, if I change line 202 to be

if isinstance(field, ListSerializer):

Then it works just fine. How do I add a test? (To be honest, like I said, I'm a serious n00b and don't even know how to use git).

[daveslab]

@jerel Ahh, however, I just noticed another problem (not sure if I should open another issue), but now that the call works when I change line 202, it doesn't return the right type. It should return (as it does using django-rest-framework-json-api 2.0.1):

//...
      "type": "AbonneFichier",
      "id": "20007",
      "attributes": {
      //...
      },
      "relationships": {
        "frequences_recu_fiscal": {
          "data": [
            {
              "type": "FrequenceRecuFiscalAbonneFichier",
              "id": "18470"
            },
           //...
          ]
        },
//...

But what it returns is:

//...
      "type": "AbonneFichier",
      "id": "20007",
      "attributes": {
      //...
      },
      "relationships": {
        "frequences_recu_fiscal": {
          "data": [
            {
              "type": "AbonneFichier",
              "id": "18470"
            },
           //...
          ]
        },
//...

[daveslab]

Hi, I'm sorry to bother, but I could have someone's thoughts on this? I'm happy to make the slight change necessary to fix the original bug I reported in this issue, but this second problem seems more difficult to debug.

[scottfisk]

@daveslab Ill see if I can track it down. I think I managed to get the tests to break from this issue.

EDIT: NM it was unrelated test breakage.

[scottfisk]

@daveslab Can you try changing

frequences_recu_fiscal = FrequenceRecuFiscalAbonneFichierSerializer(many=True)

to

from rest_framework_json_api import relations
...
frequences_recu_fiscal = relations.ResourceRelatedField(source='frequences_recu_fiscal', many=True)

Im not sure embedding a serializer like you are doing is officially supported without a corresponding JSONAPIMeta include key, have not been able to confirm this yet though.

EDIT: See #256 for how to default include vs parameterly include

[daveslab]

@scottfisk Thanks so much for replying Scott. I made the changes you suggested and now it works if I delete the second part of the line 202 as was suggested to me above. Now my serializer looks like this (I've since had to add another field,statuts):

class AbonneFichierSerializer(serializers.ModelSerializer):
    frequences_recu_fiscal = ResourceRelatedField(many=True, read_only=True)
    statuts = ResourceRelatedField(many=True, read_only=True)
    included_serializers = {
        'fichier': FichierSerializer,
        'abonne_details': AbonneDetailsSerializer,
        'abonne': AbonneSerializer,
        'frequences_recu_fiscal': FrequenceRecuFiscalAbonneFichierSerializer,
        'statuts': StatutSerializer,
    }

    class Meta:
        model = AbonneFichier

Originally I was following this post on SO by the creator of DRF on how to include related models.

At the end of the day, it looks like deleting that stuff on line 202 in renderers.py fixes the bug. Is there any chance that this could be included in the next point release? I can Google how to do a pull request on Github (never done it) and then submit it myself if you like.

[jarekwg]

+1 to this.
Reverse foreign keys return the wrong type as of 0aedffb
It's a little worrying this regression didn't cause any tests to fail.

For reference, here is a simplified version of my use case:

from rest_framework import serializers
class HandSerializer(serializers.ModelSerializer):
    included_serializers = {
        'finger_set': 'path.to.serializer.FingerSerializer',
    }
    class Meta:
        model = Hand
        fields = (
            'finger_set',
        )
class FingerSerializer(serializers.ModelSerializer):
    class Meta:
        model = Finger
        fields = ()

And the endpoint for hand/16757 returns

{
    "data": {
        "type": "Hand",
        "id": "16757",
        "attributes": {
        },
        "relationships": {
            "finger_set": {
                "data": [
                    {
                        "type": "Hand", // <-- This should be Finger!
                        "id": "65730"
                    },
                    {
                        "type": "Hand", // <-- This should be Finger!
                        "id": "65729"
                    }
                ],
                "meta": {
                    "count": 2
                }
            }
        }
    }
}

[scottfisk]

I couldn't get a failing test going for this, maybe this simple test case will help me.

[jarekwg]

Hopefully!
Let me know if you need more info.

[scottfisk]

@jarekwg Well I found out that the tests use from rest_framework_json_api import serializers and when I switch that to from rest_framework import serializers I start seeing incorrect types on reverse relationships and manytomany fields. This leads me to believe something here is allowing the types to work correctly.

I might have to defer to @jerel on what the difference between these 2 imports are and if things are expected to work importing and using drf rest_framework.serializers.

[jerel]

rest_framework.serializers isn't expected to work with json-api functionality. Any serializer imports should be from rest_framework_json_api (and all of them are available)

[jarekwg]

@scottfisk @jerel
Thanks for tracking this down and getting back to me so quickly.

I'd still label this as a bug currently. If drf serializers are not to be used with jsonapi viewsets, then an exception should be raised much earlier on, rather than this subtle mistake in the output.

However, I really hope there's a better solution than just not supporting drf serializers. Part of the beauty of this package is that it can be used as a partial drop-in replacement to drf and everything just works.

Here's my company's use case:
We have a v1 API using drf. Recently we've been transitioning to v2 using jsonapi. This has been an incremental change. So taking my example from earlier, imagine the Hand model is in one app and Finger is in another. We make a v2 api endpoint for Hand's app, building fresh serializers and viewsets for it (using restframework_json_api.serializers, say), however Finger's app is not yet updated. So we've been able to just have HandSerializers included_serializers use FingerSerializer from the v1 api and everything would still work.

I'm not sure exactly how jsonapi's ModelSerializer differs so much from drf's that would prevent this 'backwards compatibility', but I think it would be worth trying to support both.

From doing some of my own digging I see that relation_type = utils.get_related_resource_type(field) (line 105) always finds the wrong type for the field, regardless of whether drf or jsonapi serializers are used. It's just that in the jsonapi case line 162 overrides it. So I have a suspicion there's a bug with utils.get_related_resource_type, which only came to light as a result of the recent renderer changes. I'll investigate this further and get back to you.

Anyway, I see two courses of action here:

• Raise exception when jsonapi viewset attempts to use serializer not defined by jsonapi. I feel the problem is too subtle to just mention lack of drf serializer support in the docs.
• Extend tests to consider both jsonapi and drf serializers (in their simplest form), and fix whatever is going wrong with the current implementation (be it with utils.get_related_resource_type or elsewhere).

If I submit a PR for the second option will you accept it? :)

[jerel]

I would be grateful for a PR for option two.

Using JSON-API serializers and default serializers in the same app should
currently be fine but I can see how mixing them together could cause new
bugs to surface.

On Sep 7, 2016 6:14 PM, "Jarek Glowacki" [email protected] wrote:

@scottfisk https://github.com/scottfisk @jerel
https://github.com/jerel
Thanks for tracking this down and getting back to me so quickly.

I'd still label this as a bug currently. If drf serializers are not to be
used with jsonapi viewsets, then an exception should be raised much earlier
on, rather than this subtle mistake in the output.

However, I really hope there's a better solution than just not supporting
drf serializers. Part of the beauty of this package is that it can be used
as a partial drop-in replacement to drf and everything just works.

Here's my company's use case:
We have a v1 API using drf. Recently we've been transitioning to v2 using
jsonapi. This has been an incremental change. So taking my example from
earlier, imagine the Hand model is in one app and Finger is in another.
We make a v2 api endpoint for Hand's app, building fresh serializers and
viewsets for it (using restframework_json_api.serializers, say), however
Finger's app is not yet updated. So we've been able to just have
HandSerializers included_serializers use FingerSerializer from the v1 api
and everything would still work.

I'm not sure exactly how jsonapi's ModelSerializer differs so much from
drf's that would prevent this 'backwards compatibility', but I think it
would be worth trying to support both.

From doing some of my own digging I see that relation_type =
utils.get_related_resource_type(field) (line 105
0aedffb#diff-e2e58e3e5c345fc3b14c160b3a00a64cR105)
always finds the wrong type for the field, regardless of whether drf or
jsonapi serializers are used. It's just that in the jsonapi case line 162
0aedffb#diff-e2e58e3e5c345fc3b14c160b3a00a64cR162
overrides it. So I have a suspicion there's a bug with
utils.get_related_resource_type, which only came to light as a result of
the recent renderer changes. I'll investigate this further and get back to
you.

Anyway, I see two courses of action here:

• Raise exception when jsonapi viewset attempts to use serializer not
  defined by jsonapi. I feel the problem is too subtle to just mention lack
  of drf serializer support in the docs.
• Extend tests to consider both jsonapi and drf serializers (in their
  simplest form), and fix whatever is going wrong with the current
  implementation (be it with utils.get_related_resource_type or
  elsewhere).

If I submit a PR for the second option will you accept it? :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#270 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATskmw_7oHxvwcXh6GMFSX2q4OIOo5qks5qn0VRgaJpZM4JuMTH
.

[scottfisk]

what jerel said, option 2 works for me.

It seems like the current situation is more of they aren't really tested and therefore not officially supported. As you noticed most use cases using the drf serializers directly worked prior to 2.1.0 so its certainly possible to support them, not sure what edge cases would fail yet.

Maybe we should open another issue for the type issues? I don't want the discussion to get lost and still plan to fix the UnboundLocalError in the original report when I can get a breaking test for it.

[jarekwg]

Done, sorry for derailing. :)

[jerel]

Closed by #278