DRF3 support #29
DRF3 support #29
Conversation
| else: | ||
| serializer = self.get_serializer(data=request.data, many=True) | ||
| serializer.is_valid(raise_exception=True) | ||
| self.perform_create(serializer) |
There was a problem hiding this comment.
@kevin-brown from #24
perform_createis provided by the generics API, and it's most likely the primary override point for users who previously used the old save hooks. I'd recommend creating a new hook,perform_bulk_createand embedding logic in there to callperform_createif it exists, or fall back to the oldpre_saveandpost_saveloops if it doesn't.
Did you had any ideas how to do that since perform_create simply calls serializer.save()?
Since serializer is then responsible for generating multiple objects, I am not sure what we can loop over here.
There was a problem hiding this comment.
And ListSerializer.create just calls OriginalSerializer.create on each of the objects.
I'd just create a perform_bulk_create hook (that just calls serializer.save) and leave it at that.
The other option is to split away from ListSerializer.create and handle creation on your own by re-creating the loop that it usually uses and calling your own hooks instead.
| # this allows to maintain clean code for each DRF version | ||
| # without doing any magic | ||
| # a little more code but a lit clearer what is going on | ||
| if str(rest_framework.__version__).startswith('2'): |
There was a problem hiding this comment.
how so? we might have to wait a while to get to DRF 20+
There was a problem hiding this comment.
in case that is changed to some other datastructure but not necessary now
…3 support which I based my implementation on [ci skip]
| # does not actually do anything with the returned object | ||
| for file, line, function, code in traceback.extract_stack(): | ||
| if all((file.endswith('rest_framework/views.py'), | ||
| function == 'options')): |
There was a problem hiding this comment.
Maybe it would be better here just to check self.request.method?
There was a problem hiding this comment.
I was thinking if people use a custom router, they might be using metadata() or options() some other place (like offline doc generation). Is that a possibility?
I agree this is not the cleanest way of doing it. I would of used self.action buts thats only available in viewsets but this is necessary in views only when metadata is generated.
There was a problem hiding this comment.
@tomchristie maybe you can advise?
There was a problem hiding this comment.
I'm sorry I don't understand why AssertionError is being caught there so from a brief look I've no idea. Happy to look in more detail during the week.
There was a problem hiding this comment.
@tomchristie here is the context:
In the super implementation of the get_object(), it asserts that lookup_url_kwarg is present within self.kwargs. That makes sense since get_object() is usually called in a URL for a single resource (e.g. /api/resource/<pk>/) so lookup_url_kwarg will always be present.
The challenge here is that we are trying to add bulk operations with support for PUT requests to a list resource which obviously will be lacking a lookup_url_kwarg. More particularly the exception occurs when making OPTIONS request to the api which uses metadata class to get api docs which internally calls get_object.
Here is what is my end goal - detect when metadata is generated in which case ignore the assertion error but in all other cases rerase the exception since that most likely indicates that something is indeed wrong and exception should not be ignored.
What would be the best way of achieving that?
@kevin-brown suggested that we can simply check if self.request.method == 'OPTIONS' and then reraise exception but I am not sure if that will catch all the use-cases.
There was a problem hiding this comment.
What would be the best way of achieving that?
I'd probably suggest fully overriding get_object so that it actually has the correct behavior that you need for bulk updates.
…n are found in db
Fixes #23
Some ideas taken from #24.
The major difference in the approach here vs one in #24 is that instead of only supporting DRF3, this PR supports both 2 and 3. This is especially important since many of existing APIs have not migrated to DRF3 yet (including some of my own so sorry for being a little selfish here). How we accomplish that is by having separate packages for DRF2 and DRF3. This way we dont have to do crazy things to support both. I understand this adds code duplication however in a long run I think this is nicer and cleaner approach. In addition this is allows to only new methods introduced in DRF3 without mixing them with older DRF2 methods such as
pre_save, etc.Eventually when this library will only support DRF3, we can then simply move the DRF3 mixins from its own package to the root of the library and no other changes are going to be required.
TODO: