Skip to content
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

Retry form rendering when rendering with serializer fails. #3164

Closed
wants to merge 2 commits into from
Closed

Retry form rendering when rendering with serializer fails. #3164

wants to merge 2 commits into from

Conversation

asedeno
Copy link

@asedeno asedeno commented Jul 16, 2015

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.

# trace.
existing_serializer = None
kwargs = {}
retried = True
Copy link
Member

@tomchristie tomchristie Jul 16, 2015

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.

Copy link
Author

@asedeno asedeno Jul 16, 2015

Choose a reason for hiding this comment

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

Most of it is an additional level of indentation. See if you still think the same when ignoring whitespace changes.

https://github.com/tomchristie/django-rest-framework/pull/3164/files?diff=unified&w=1

Copy link
Member

@tomchristie tomchristie Jul 16, 2015

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.

Copy link
Author

@asedeno asedeno Jul 16, 2015

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?

Copy link
Member

@tomchristie tomchristie Jul 16, 2015

Choose a reason for hiding this comment

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

Let's find out.

Copy link
Member

@tomchristie tomchristie Jul 16, 2015

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.

Copy link
Member

@tomchristie tomchristie Jul 16, 2015

Choose a reason for hiding this comment

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

Step by little step.

@tomchristie
Copy link
Member

tomchristie commented Jul 16, 2015

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 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 Jul 16, 2015
@asedeno 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 Jul 16, 2015
@asedeno
Copy link
Author

asedeno commented Jul 20, 2015

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.

@pattisdr
Copy link
Contributor

pattisdr commented Aug 31, 2015

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

@tomchristie
Copy link
Member

tomchristie commented Sep 1, 2015

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.

@asedeno
Copy link
Author

asedeno commented Sep 1, 2015

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

@asedeno
Copy link
Author

asedeno commented Sep 21, 2015

Any word on this?

@tomchristie
Copy link
Member

tomchristie commented Sep 21, 2015

No change on my original comment.

I'd want to see:

  1. A refactoring PR that does not change the behavior, but is valid as it stands.
  2. 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.

@asedeno
Copy link
Author

asedeno commented Sep 23, 2015

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:

  1. A commit which breaks the tests, providing an example of what is broken and a way to test that it is fixed.
  2. A commit that splits off the behavior we want to modify to fix the broken test, but does not change functionality.
  3. 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.

@asedeno
Copy link
Author

asedeno commented Nov 18, 2015

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.

@tomchristie
Copy link
Member

tomchristie commented Nov 18, 2015

@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.

@asedeno
Copy link
Author

asedeno commented Nov 18, 2015

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.

@tomchristie tomchristie added this to the 3.3.2 Release milestone Nov 18, 2015
@tomchristie
Copy link
Member

tomchristie commented Nov 18, 2015

Milestoning to move it up the queue.

@asedeno
Copy link
Author

asedeno commented Nov 18, 2015

Thank you!

@xordoquy xordoquy modified the milestones: 3.3.2 Release, 3.3.3 Release Dec 14, 2015
@xordoquy xordoquy modified the milestones: 3.3.3 Release, 3.3.4 Release, 3.4.0 Release Feb 11, 2016
@asedeno
Copy link
Author

asedeno commented Feb 22, 2016

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.

@tomchristie
Copy link
Member

tomchristie commented Feb 22, 2016

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.

@asedeno
Copy link
Author

asedeno commented Feb 23, 2016

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:

  1. try it with the serializer we have, like we do right now.
  2. 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.

@tomchristie
Copy link
Member

tomchristie commented Feb 23, 2016

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.

@asedeno
Copy link
Author

asedeno commented Feb 23, 2016

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.)

@tomchristie
Copy link
Member

tomchristie commented Feb 23, 2016

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?)

@asedeno
Copy link
Author

asedeno commented Feb 23, 2016

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.

@tomchristie
Copy link
Member

tomchristie commented Feb 23, 2016

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.

@asedeno
Copy link
Author

asedeno commented Feb 23, 2016

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.

asedeno added 2 commits Feb 23, 2016
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.
@asedeno
Copy link
Author

asedeno commented Feb 25, 2016

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

@asedeno
Copy link
Author

asedeno commented Apr 5, 2016

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

@tomchristie
Copy link
Member

tomchristie commented Jun 8, 2016

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants