Retry form rendering when rendering with serializer fails. by asedeno · Pull Request #3164 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation32 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

As per IRC, here's the new pull request.
First commit is a small test that tickles the issue.
Second commit adds a retry mechanism to use a generated serializer if the one attached to data does not work.

The issue here is most easily seen in a POST request that returns multiple objects, in which case the BrowsableAPIRenderer raises a TypeError on the serializer (a ListSerializer) not being iterable. This exception prevents an otherwise fine response to fail on account of a failure to render a form.

x-ref: #3138, #2918.

tomchristie

# trace.
existing_serializer = None
kwargs = {}
retried = True

Choose a reason for hiding this comment

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

The change footprint for this fix is pretty large. Worth considering if there's any more minimal ways to address this.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Still do really - having that all inside a while True makes is super hard to determine the behavior, or see where it's breaking out.

A good plan would be to try to target this more specifically - I'd far prefer to see some code repeated in the exception block, than introduce a more complicated flow in order to not duplicate anything.

Choose a reason for hiding this comment

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

I was considering that, though I didn't like just how much code I was going to duplicate. How would you feel about me breaking out the common code into an inlined function?

Choose a reason for hiding this comment

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

Let's find out.

Choose a reason for hiding this comment

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

Which is shorthand for, "couldn't really say without seeing it first".

However, having said that... ...it might be best to first make this change in the most minimal way possible, and only then consider refactoring if it got accepted.

Choose a reason for hiding this comment

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

Step by little step.

It'd be worth bring a brief description into the summary here.
Also cross references should be in the description or thread only, not in the title.

asedeno changed the title~~Follow up from PR 3138, issue 2918 - test and second fix attempt~~ Retry form rendering in BrowsableAPIRenderer::get_rendered_html_form when rendering with serializer fails

asedeno changed the title~~Retry form rendering in BrowsableAPIRenderer::get_rendered_html_form when rendering with serializer fails~~ Retry form rendering in BrowsableAPIRenderer::get_rendered_html_form() when rendering with serializer fails

I updated the loop to an inline function. I believe this is a good compromise between code duplication and understanding behavior. Ignoring whitespace, renderers.py only gains seven lines, which is pretty minimal.

Are there any plans to merge this? It's just the thing I need to get the BrowsableAPI to work when posting multiple objects.

Thanks for the note - yes it's in the backlog, and we'd merge an acceptable pull request here.

Review: The pull request as it currently stands deals with two separate concerns. There's a refactoring, and there's also the re-try. It'd be easier to comprehensively review if these were treated as separate pull requests. Best thing to move this along would be for someone to take on the task of splitting this into two separately reviewable units, so that we can keep each step as absolutely minimal and thoroughly reviewed as possible.

Would splitting it into two separate commits in this pull request suffice? The two changes don't really make sense separately.

No change on my original comment.

I'd want to see:

A refactoring PR that does not change the behavior, but is valid as it stands.
A behavioral PR on top of that.

Right now I don't have a lot of overhead for issue backlog - dealing with remains of 3.3/kickstarter functionality in last few days of funded time. Hopefully be able to get cracking with it again if we have a subsequent funding drive.

I do not understand why you want to see this as two PRs rather than two commits in one PR.
The refactor is meaningless without the behavior change. What does anybody gain from multiple Pull Requests for one issue?

Specifically, this is one logical change:

A commit which breaks the tests, providing an example of what is broken and a way to test that it is fixed.
A commit that splits off the behavior we want to modify to fix the broken test, but does not change functionality.
A commit to change the functionality in a way that fixes the test.

I can't make a PR for (1) by itself because that will break the tests by design.
Making a PR for (2) is asking you to review and merge a commit that appears to have no point, then starting over.
Making a PR for (3) requires that (2) have already been merged.

This is one change. You can take it or leave it. I am done jumping through hoops trying to make your software better for now.

I'm still waiting for resolution on this, and I'm not alone.

I've added a test that demonstrates the bug, I've made a non-functional change that helps with the fix (at your request), and I've fixed the bug. It's about as simple as a unit of work can get.

@asedeno Noted. We've got 114 open issues, and since the end of the kickstarter currently have no funded work on the project, so there's a backlog right now.

I understand there is a backlog. That would be more compelling if I were asking you to fix a bug, not handing you a fix for it myself. I put the work in for you, free of charge, and I will make any functional changes you might want to this PR, but I believe it is ready and has been for months.

Milestoning to move it up the queue.

So does a milestone mean anything for this PR, or is it only a can to keep kicking down the road?

Checks pass, there are no conflicts, this has been essentially ready for months.

This is very far from being a simple, obviously correct or easily reviewable change.

It's not immediately apparent to me what exactly the behavior is before or after, and I've not any spare time to review in depth.

Having try...except TypeError reads completely opaquely, so even as the author I wouldn't understand what's happening there or why. If this was a simpler and more obvious fix, then yes it'd be in - as it stands, it's not terribly clear and hence, given the review bottleneck we've got at the moment it's stalled. Perhaps there's a simpler more obvious fix for this - again, not sure without some proper thinking about it.

Is the milestone meaningful? yes - it means this isn't absolutely at the bottom of the queue, and if and when we get paid time back it's likely to get resolved.

The try...except TypeError is there to catch
TypeError: 'ListSerializer' object is not iterable
If I could catch a more specific exception, I would. I can add more comments to the code if you like.

This is nothing more than:

try it with the serializer we have, like we do right now.
If we had a serializer and it fails with a TypeError, try again without the serializer like we would if we didn't have one.

In all the cases where DRF currently works, no TypeError is raised, so the try...except doesn't do anything. In the case where a TypeError is raised, DRF currently crashes and burns. This tries something else before crashing and burning, and the test I added shows that it does enough to make it succeed.

Again, this exception is raised after DRF has a perfectly valid response to the user's request, while rendering the user-friendly browsable api and trying to pre-fill a form. Returning a response and a form that is not filled in is much friendlier than returning a TypeError when we have a perfectly good response to the user's request.

Noted. One comment for consideration. If the form rendering horribleness that we currently have can at least be nicely encapsulated then this could be a go-er.

Pulling the inner function out causes a lot more code churn, adds a lot of function parameters to replicate the enclosing scope, and I think actually makes this harder to reason about. This encapsulates just the piece of get_rendered_html_form we want to retry so we can call it without a serializer.

This is one of the reasons python has scoping rules that allow for nested scoping. (Thank you, PEP 227.)

I don't have any huge problem with an inner scoped function.
I still have a problem with relying on outset scoped variables inside the inner scoped function, tho.
(i.e. has_serializer, others?)

The whole point of statically nested scoping (PEP 227) is to have access to the enclosing scope.

I'll refer you to viewsets.py where an inner view function makes use of actions from the enclosing scope. Also, just about every decorator ever.

whole point
It keeps the function out of the global scope, which if it's not useful elsewhere is a nice bit of encapsulation.

I'll refer you to
Is a different case - we need to take that approach there because we're returning a function closure.

Those are side issues tho - In either case I still not convinced it's the right approach to keep it nested here as we're only increasing the complexity, without mitigating that by encapsulating any of it away. (Since we're in the same scope) Wrap it up more strictly and at least we're keeping the horribleness contained somewhat.

I think you are wrong. That said, I'm looking at a different approach that will maybe make you happier:

Collect a list of serializers to try with. If there is an existing serializer, that one will be first on the list. Next on the list is the serializer we would set up if there was not an existing serializer.

Then, loop over them in a try block and return the form using the first one that succeeds. The only question is, if they all fail, and there were multiple attempts, which exception would to like to see raised?

I've gone with the last one because that is the one I can raise with the right stack trace.

The first one should work, if it doesn't because of Issue #2918, then the second one should work. If no collected serializer worked, raise the exception generated by the last one.

Is this implementation more to your liking? There's no inner function, and the loop is much more manageable.

Waking up this PR once again as I am still waiting for feedback or for it to be merged.

Okay, finally picked up as I now have funded time on the project.
Fully reviewed now, and have an implementation based on this at #4181.
Thanks for the work towards this!

tomchristie changed the title~~Retry form rendering in BrowsableAPIRenderer::get_rendered_html_form() when rendering with serializer fails~~ Retry form rendering when rendering with serializer fails.

This was referenced

Mar 9, 2017