Fixed #20347 -- Added formset_factory absolute_max kwarg #1593

Closed
wants to merge 63 commits into
from
@ethurgood

Added a keyword argument named absolute_max to django.forms.formsets.formset_factory(). This allows a developer to explicitly provide the absolute maximum number of forms to accept via POST, instead of relying on the former behavior limiting this to max_num + 1000. This separates the form display logic from the form processing logic.

Eric Thurgood and others added some commits Sep 7, 2013
Eric Thurgood Fixed #20347 -- Added formset_factory absolute_max kwarg.
Added a keyword argument named absolute_max to django.forms.formsets .formset_factory(). This allows a developer to explicitly provide the absolute maximum number of forms to accept via POST, instead of relying on the former behavior limiting this to max_num + 1000. This separates the form display logic from the form processing logic.
b691268
Eric Thurgood Merge remote-tracking branch 'upstream/master' into sprint 3681034
@evildmp evildmp Consolidated documentation for F() and Q() 7ea73f1
@timgraham timgraham Fixed instructions for running a subset of tests. 88a3d0f
@dlanger dlanger Fixed #4287 -- Fixed NaN and +/- Infinity handling in FloatField
NaN, +Inf, and -Inf are no longer valid values for FloatFields.
0c68da9
@apollo13 apollo13 Fixed #20812 -- Error out if __unicode__/__str__ doesn't return a tex…
…t type.
47c2049
@danrjohnson danrjohnson Fixed #21043 -- Made resolve() handle reverse_lazy objects.
Thanks Keryn Knight for the report.
dda2db0
Matthew Rich re-indented method documentation within RelatedManager b1c8c01
@adrianholovaty adrianholovaty Fixed versionadded and ordering of note in admin/index.txt 6b5f6d9
@adrianholovaty adrianholovaty Added new AdminSite attributes to 1.7 release notes de2875f
@mburst mburst Fixed #21049 -- Fixed autoreload for Python 3
Changed th system module values check to return a list.
In Python 3 it returns a dict_view which could occassionally produce
a runtime error of "dictionary changed size during iteration".
633a9b3
@andrewgodwin andrewgodwin Adding 'sqlmigrate' command and quote_parameter to support it. 78b63e2
@timgraham timgraham Fixed #20646 -- Clarified the use of AbstractBaseUser.REQUIRED_FIELDS
Thanks craigbruce.
75b05b0
@timgraham timgraham Fixed some sphinx errors and added some links. a82af6a
@kedmiston kedmiston Fixed #19295 -- Documented that CachedStaticFilesStorage isn't compat…
…ible with runserver --insecure.
fb1ed08
Rudy Mutter Fixed #20821 -- Added tooltips to Admin SelectBox widget
The Admin widget, which can be used to filter multiple selects
can sometimes be too narrow and hide information such as
user permissions. This commit adds tooltips to the select
options so that a user can hover over and see the hidden text.
f03aa06
@ianawilson ianawilson adds fix and test for when a template is not specified at all to rend…
…er(). fixes #21058. by jambonrose and ianawilson
e42b531
@jphalip jphalip Added Rudy Mutter to AUTHORS for his work, ref #20821. 3261cf9
@andrewgodwin andrewgodwin Add -l alias for migrate --list 7ec8d7e
@adrianholovaty adrianholovaty Moved a settings usage up the stack in utils/formats.py #unsettings 3a29754
@timgraham timgraham Fixed a link in topics/testing/overview.txt 1772405
@ianawilson ianawilson Removed unnecessary, leftover imports e82f44d
@rca rca Added yaml directly into BUILTIN_SERIALIZERS.
The serializer definitely exists, but the dependent yaml module may not
be installed.  The register_serializer() function will catch exceptions
and will stub in a fake serializer object that will raise the exception
when the serializer is used.
4a09e8d
@rca rca Added tests for missing pyyaml.
This test makes sure an YAML import errors are communicated to the
caller rather than stating the serializer does not exist.
fcce21f
@ianawilson ianawilson adds fix for SingleObjectTemplateResponseMixin raising a TemplateDoes…
…NotExist when it should have raised an ImproperlyConfigured. fixes 16502. by @ianawilson, @jambonrose
566c3ae
@aaugustin aaugustin Minor factorization. 5ca1a8b
@aaugustin aaugustin Fixed #11811 -- Data-loss bug in queryset.update.
It's now forbidden to call queryset.update(field=instance) when instance
hasn't been saved to the database ie. instance.pk is None.
382cc34
@ianawilson ianawilson fixed test name from an old, overly specific iteration of the test 01a2fb1
@rca rca Moved get_serializer() call in dumpdata command.
Moved the get_serializer() call within the condition that checks public
serializers.  This will allow exceptions other than
SerializerDoesNotExist to be raised in order to provide the caller with
useful information, e.g when pyyaml is not installed.
9a29225
@loic loic Fixed #21037 -- Made MigrationWriter raise a ValueError when serializ…
…ing lambda functions.
55572e9
@mburst mburst Added myself to the contributors list. 5e79f1d
@timgraham timgraham Fixed test failure introduced in efd1e60 (sqlmigrate) e4e7eb0
@timgraham timgraham Fixed Python 3 syntax error introduced in [c72392d] 511c516
@rca rca Updated NoYamlSerializerTestCase to run with yaml.
In order to verify the behavior of using the yaml serializer when yaml
is on the system, fake the ImportError when the serializer is being
registered.
266e646
@rca rca Added name to AUTHORS (per Russell Keith-Magee) 3bb7deb
@rca rca Fixed existing tests to handle BadSerializer.
When a BadSerializer instance is stubbed in for the yaml serializer,
make sure tests do not fail.
54962f1
@rca rca Cleanup commit after peer review. 84af02f
@adamsc64 adamsc64 Fixed #11857 -- Added missing 'closed' property on TemporaryFile class.
- TemporaryFile now minimally mocks the API of the Python standard
  library class tempfile.NamedTemporaryFile to avoid AttributeError
  exceptions.
- The symbol django.core.files.NamedTemporaryFile is actually assigned
  as a different class on different operating systems.
- The bug only occurred if Django is running on Windows, hence why it
  was hard to diagnose.
a64d7c4
@timgraham timgraham Fixed deprecation warning on Python 3 159a192
@adamsc64 adamsc64 Added Christopher Adams to the AUTHORS file.
- Note that 'Chris Adams' and 'Christopher Adams' are two different
  contributors.
6fed6b3
@EricBoersma EricBoersma Fixed #20007 -- Configured psycopg2 to return UnicodeArrays
Thanks hogbait for the report.
cb38f90
@alex alex Fixed a number of flake8 errors -- particularly around unused imports…
… and local variables
02ed7c9
@alex alex Fixed this syntax error on py32 0b997fc
@aaugustin aaugustin Fixed #21032 -- pip 1.4 can't install pytz. 0c2c4b5
@loic loic Fixed regression introduced by efd1e60, 'map' returns an iterator on …
…PY3.
a12ac16
@polmuz polmuz Add `response_delete` and `render_delete_form` methods to `ModelAdmin`
This make it easier to control the delete flow.
8904fd5
@polmuz polmuz Add docs for `response_add`, `response_change` and `response_delete` fff333b
@aaugustin aaugustin Moved two WSGI-specific functions to the WSGI handler.
They were defined in base when the mod_python handler used them. See bfcecbf.
10b44a5
@aaugustin aaugustin Refactored the unmangling of the WSGI environ. 157e6cf
@aaugustin aaugustin Minor cleanup in the WSGI handler. a1f6403
@aaugustin aaugustin Fixed #20557 -- Properly decoded non-ASCII cookies on Python 3.
Thanks mitsuhiko for the report.

Non-ASCII values are supported. Non-ASCII keys still aren't, because the
current parser mangles them. That's another bug.
9bb95c2
@aaugustin aaugustin Fixed tests introduced in previous commit on Python 2. Refs #20557. 7a36c75
@loic loic Fixed regression introduced by a962286, changed ugettext to ugettext_…
…lazy.
5ee0e80
@kedmiston kedmiston Fixed 16992 -- Added InnoDB warning regarding reuse of AUTO_INCREMENT…
… values.

Thanks kent at nsc.liu.se for the report.
003867b
@andrewgodwin andrewgodwin RunSQL migration operation and alpha SeparateDatabaseAndState op'n. b44de0f
@aaugustin aaugustin Fixed an encoding issue in the test client.
Fixed
comment_tests.tests.test_comment_view.CommentViewTests.testCommentPostRedirectWithInvalidIntegerPK.

Refs #20530.
0d32fb7
@aaugustin aaugustin Oops :( 810fa86
@polmuz polmuz Improved docs for `contrib.admin.options.ModelAdmin.response_*`
Added links to code references in the docs for `response_add`,
`response_change` and `response_delete`.
d3f9e47
@aaugustin aaugustin Fixed #20530 -- Properly decoded non-ASCII query strings on Python 3.
Thanks mitsuhiko for the report.

This commit just adds a test since the problem was fixed in 8aaca65.
f9e4a2b
@timgraham timgraham Fixed #20938 -- Added cached sessions note to deployment checklist.
Thanks mjtamlyn for the suggestion.
8da0e7c
Eric Thurgood Merge remote-tracking branch 'origin/sprint' into sprint 2b7b006
Eric Thurgood Merge remote-tracking branch 'upstream/master' into sprint 91bb6c5
Eric Thurgood Fixed #20347 -- Added formset_factory absolute_max kwarg.
refactored formset_factory() to make the 4 possible scenarios of max_num and
absolute_max more clear.

PEP8 on unit tests.

Additional tests, cleaner testing using: with self.assertRaises(ValueError)
5a99202
@carljm
Django member

Hi @ethurgood - thanks for this pull request! It seems to have been polluted by quite a few unrelated changes, making it hard to review. Could you clean it up or close it and submit a new clean one? You might find it useful to create a git branch per pull request rather than a generic branch like sprint, since pull requests remain tied to their source branch and will automatically include any new commits on that branch.

Thanks!

@ethurgood

Hey @carljm - A couple of the onsite core devs said not to worry about rebasing/squashing the merge commit because the "modified files" tab only shows what I actually changed. Is it not showing up like that for you?

@carljm carljm commented on the diff Sep 9, 2013
django/forms/formsets.py
if max_num is None:
max_num = DEFAULT_MAX_NUM
- # hard limit on forms instantiated, to prevent memory-exhaustion attacks
- # limit is simply max_num + DEFAULT_MAX_NUM (which is 2*DEFAULT_MAX_NUM
- # if max_num is None in the first place)
- absolute_max = max_num + DEFAULT_MAX_NUM
+ if absolute_max is None:
@carljm
Django member
carljm added a line comment Sep 9, 2013

This logic could be consolidated to avoid repeating the same conditionals twice. Just check both max_num and absolute_max independently for None and set default as needed, then once they are both set, check that absolute_max > max_num. The minor difference in error message here doesn't justify all the repeated logic, IMO; a simple "absolute_max must be greater than max_num" is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carljm carljm commented on the diff Sep 9, 2013
docs/topics/forms/formsets.txt
@@ -108,7 +108,10 @@ objects, up to ``extra`` additional blank forms will be added to the formset,
so long as the total number of forms does not exceed ``max_num``.
A ``max_num`` value of ``None`` (the default) puts a high limit on the number
-of forms displayed (1000). In practice this is equivalent to no limit.
+of forms displayed (1000). In practice this is equivalent to no limit. If
@carljm
Django member
carljm added a line comment Sep 9, 2013

I'm not sure that this ValueError case is really necessary to describe here; it's more about absolute_max than about max_num.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carljm carljm commented on the diff Sep 9, 2013
docs/topics/forms/formsets.txt
+ ... 'form-MAX_NUM_FORMS': u'',
+ ... }
+ >>> formset = ArticleFormSet(data)
+ >>> len(formset.forms)
+ 10
+ >>> formset.is_valid()
+ False
+ >>> formset.non_form_errors()
+ [u'Please submit 10 or fewer forms.']
+
+
+An ``absolute_max`` value of ``None`` (the default) results in an
+``absolute_max`` value equal to ``max_num`` + 1000. (If ``max_num`` was also
+``None``, the resulting ``absolute_max`` value is 2000).
+
+If both ``absolute_max`` and ``max_number`` are specified and ``absolute_max``
@carljm
Django member
carljm added a line comment Sep 9, 2013

I think these two paragraphs can safely be condensed into "If absolute_max is less than max_num, a ValueError will be raised." The interaction with default values is obvious enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carljm carljm commented on the diff Sep 9, 2013
tests/forms_tests/tests/test_formsets.py
+ self.validate_formset_number_of_forms(formset,
+ 2*formsets.DEFAULT_MAX_NUM)
+
+ def test_explicit_absolute_max_higher_than_default(self):
+ # absolute_max can now be explicitly specified via kwarg to
+ # formset_factory.
+ # This behavior was changed from absolute_max always being max_num +
+ # 1000 in the patch for #20347
+ data = {
+ 'form-TOTAL_FORMS': '2505',
+ 'form-INITIAL_FORMS': '0',
+ 'form-MAX_NUM_FORMS': '0',
+ }
+
+ # absolute_max can now be explicitly set, not being affected by
+ # max_num
@carljm
Django member
carljm added a line comment Sep 9, 2013

no need for line break here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carljm
Django member

Hmm, you're right, I didn't actually even look at the "modified files" tab; I'd never seen a case before where GitHub showed so many unrelated commits in the commit list that then didn't appear in the diff.

It looks like what happened is that a bunch of commits from the main repo got rebased in your branch at some point, so they now have different commit hashes and appear to git to be different commits, but they disappear from the diff because the same changes are present in both places. But you're totally right, while this will need to be dealt with at merge time (so we don't get all these dupe commits into the history with a naive merge), it's not a problem at all for review, sorry for the false alarm!

Thanks for the pull request, I made a few minor suggestions but generally this looks pretty good!

@timgraham
Django member

@ethurgood, if you could update the PR per Carl's comments, please send a new PR, thanks.

@timgraham timgraham closed this Jun 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment