add lazy loading serializer support

[tfranzel]

This is one approach of supporting recursive serializers through lazy loading string classes #23

the reason the loading is located in _get_serializer_from_model_or_instance is that if it is placed in __init__ the same circular import issue occurs, because at that point in time python has not completely parsed the poly serializer classes yet.

• (+) minimal changes
• (+) behaves identical if classes are used instead of strings.
• (-) accessing model_serializer_mapping by hand will not automatically load the class. However, if you use strings you are certainly aware of that and getting a string shouldn't be surprising.

originally I intended to propose a second approach, where model_serializer_mapping is automagically loaded and can be directly accessed through the attribute. As it turns out it was a lot more complicated, required deeper changes and broke some tests. So this PR is a lot more practical, understandable and gets the job done the same way basically. what do you think?

[codecov]

Codecov Report

Merging #24 into master will increase coverage by 0.51%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   93.10%   93.61%   +0.51%     
==========================================
  Files           1        1              
  Lines          87       94       +7     
==========================================
+ Hits           81       88       +7     
  Misses          6        6              

[denisorehovsky]

Thanks for the PR!

[denisorehovsky]

Maybe it would be better to import serializers in init method (somewhere here https://github.com/apirobot/django-rest-polymorphic/blob/master/rest_polymorphic/serializers.py#L39)? Because now we instantiate serializers in init method. Also, in that case, we wouldn't need to create serializer_args and serializer_kwargs fields. We will have something like this:

if isinstance(serializer, str):
 serializer = import_string(serializer)(*args, **kwargs)
elif callable(serializer):
  serializer = serializer(*args, **kwargs)

What do you think?

[denisorehovsky]

Sorry, I haven't seen your PR's description first. I will take a look at this PR later

[denisorehovsky]

Did you try to use importlib.import_module? I'm looking at this code https://github.com/rsinger86/drf-flex-fields/blob/master/rest_flex_fields/serializers.py#L94, and if you can see, they import serializers in init method.

[tfranzel]

django.utils.module_loading.import_string is a convenience wrapper around importlib.import_module. effects are identical and flex-fields basically reimplements import_string in _import_serializer_class.

Unfortunatelly, I was not able to find tests in flex-fields that actually use that feature. I suspect it might break on situations like the example below.

The thing is it can work in __init__ for simpler cases but it breaks in the really interesting ones. Consider this example:

class ZSerializer(PolymorhicSerializer)
	model_serializer_mapping = {
		ModelX: 'XSerializer',
		ModelY: 'YSerializer',
	}

class XSerializer(Serializer):
	# some fields

class YSerializer(Serializer):
	# other fields
	z = ZSerializer()

Once you start using a nested field like z = ZSerializer(), the __init__ approach breaks down. You must init the ZSerializer with () there. That means ZSerializer inits inside the definition of YSerializer. When it inits it tries to load the string for YSerializer, but loading YSerializer cannot succeed as we just went into a cyclic import.

Basically the culprit is the ZSerializer(). As this is a DRF idiom there is no way around it and as a result "transitive self-referential" imports cannot happen in serializer constructors.

Also, rearranging the classes into separate files will not solve the issue.

Not sure if that is something that can happen with how flex-fields are supposed to be used. I have never used it.

Is that logic comprehensible or did I miss something here?

[tfranzel]

@apirobot i know you are pretty pressed for time, but did you get a chance to think about this? thanks!

[denisorehovsky]

@tfranzel Sorry that it took so long to respond. Could you please test this change?

            if isinstance(serializer, str):
                pieces = serializer.split('.')
                class_name = pieces.pop()
                module = importlib.import_module('.'.join(pieces))
                serializer = getattr(module, class_name)

When I added this code and then ran your test, it passed.

[denisorehovsky]

@tfranzel If this change won't work as well, then probably, the only way to get this working is by pushing what you committed.

Also, I don't wanna cause any bugs. So, maybe this feature doesn't worth it?

[denisorehovsky]

I've tried to create serializers like these:

And I didn't receive an error with this change. Am I doing something wrong?

[tfranzel]

@apirobot thank you for taking the time! my test only covers basic functionality and not the feature i outlined in the comment above. i'll construct a test for that as well and test both versions.

[denisorehovsky]

Awesome. Sounds good. Thanks