Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#13032 closed (fixed)

USE_THOUSAND_SEPARATOR breaks forms by formatting primary key values, causing error on save

Reported by: exogen@… Owned by: gonzalodelgado
Component: Internationalization Version: master
Severity: Keywords: formats l10n locale pycamp2010
Cc: exogen@…, apollo13 Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When using USE_THOUSAND_SEPARATOR and NUMBER_GROUPING, number values in forms (e.g. those in Django admin) are locale-formatted. Some fields clean separators from the value and continue to work, but some don't, causing errors when saving forms with inlines, for instance: invalid literal for int() with base 10: '2,978' - in this case, 2978 is the model's AutoField value.

I would strongly reconsider locale-formatting any form values and being much more cautious about when to format. Consider that it's reasonable to use an IntegerField to store a year, yet we don't separate/group those numbers.

Attachments (3)

testproject.tar.gz (3.2 KB) - added by exogen@… 5 years ago.
Test project demonstrating that USE_THOUSAND_SEPARATOR/NUMBER_GROUPING break inline saving
13032.1.diff (7.2 KB) - added by jezdez 5 years ago.
Added localize parameter to form fields to be able to selectively enable localization
13032.2.diff (10.0 KB) - added by jezdez 5 years ago.
localize as much as possible in the form field

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from USE_THOUSAND_SEPARATOR breaks forms by separating inline primary key values, breaking save to USE_THOUSAND_SEPARATOR breaks forms by formatting primary key values, causing error on save

comment:2 Changed 5 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

This needs to be fixed for 1.2 - it's a serious problem if you can't submit any form or inline with integer content. The issue of disabling comma separators for years is also worth looking at, but should be a separate ticket.

comment:3 Changed 5 years ago by gonzalodelgado

  • Owner changed from nobody to gonzalodelgado
  • Status changed from new to assigned

comment:4 Changed 5 years ago by gonzalodelgado

  • Cc exogen@… added
  • Keywords pycamp2010 added

I'm not able to reproduce this. Here's what I did:

  • Created a test project and set USE_THOUSAND_SEPARATOR = True and NUMBER_GROUPING = 2 in settings.py
  • Created a test app with the following models and admin classes
class MainTestModel(models.Model):
    name = models.CharField(max_length=100)

class InlineTestModel(models.Model):
    number = models.IntegerField()
    main = models.ForeignKey(MainTestModel)

class InlineTestAdmin(admin.TabularInline):
    model = InlineTestModel

class MainTestAdmin(admin.ModelAdmin):
    model = MainTestModel
    inlines = [InlineTestAdmin]
  • Tested creating instances of both models from the admin

Objects are created flawlessly, it would be good if the original poster could paste the full stack trace that lead to the invalid literal for int() with base 10: '2,978' exception from an AutoField as well as the NUMBER_GROUPING value used.

comment:5 Changed 5 years ago by gonzalodelgado

I hadn't set USE_L10N in settings.py, but I did that and tested again and still can't get the AutoField to fail.

Also, I wrote a form class holding all the buil-in numerical fields to test this and can't any of them to fail either:

class TestForm(forms.Form):
    intfield = forms.IntegerField(required=False)
    floatfield = forms.FloatField(required=False)
    decimalfield = forms.DecimalField(required=False)

>>> from testapp.forms import TestForm
>>> tf = TestForm({'intfield': '1,000', 'floatfield': '1,000', 'decimalfield': '1,000' })
>>> tf.is_valid()
True
>>> tf.cleaned_data
{'floatfield': 1000.0, 'intfield': 1000, 'decimalfield': Decimal('1000')}
>>> print tf
<tr><th><label for="id_intfield">Intfield:</label></th><td><input type="text" name="intfield" value="1,000" id="id_intfield" /></td></tr>
<tr><th><label for="id_floatfield">Floatfield:</label></th><td><input type="text" name="floatfield" value="1,000" id="id_floatfield" /></td></tr>
<tr><th><label for="id_decimalfield">Decimalfield:</label></th><td><input type="text" name="decimalfield" value="1,000" id="id_decimalfield" /></td></tr>

So data seems to be cleaned properly by numerical fields even when USE_THOUSAND_SEPARATOR is True and NUMBER_GROUPING isn't 0.

I'm wondering what data did the original poster use and what the value for the NUMBER_GROUPING setting was.

comment:6 Changed 5 years ago by jezdez

  • Component changed from Uncategorized to Internationalization

Thanks for the bug report, exogen. Please provide us with more detail about your test environment.

comment:7 Changed 5 years ago by apollo13

  • Cc apollo13 added

comment:8 Changed 5 years ago by exogen@…

@gonzalodelgado: As mentioned in the report, this is happening on the model's AutoField. You need to create an InlineTestModel instance whose primary key would be formatted with the number grouping/thousand separator settings. I've attached a minimal project with such a fixture and test cases demonstrating the problem.

Changed 5 years ago by exogen@…

Test project demonstrating that USE_THOUSAND_SEPARATOR/NUMBER_GROUPING break inline saving

comment:9 Changed 5 years ago by ramiro

I've just done the following:

  • Downloaded the projct and ran it setting the USE_I10N setting to True and the language to es-ar ('.' is thousand separator, every three digits).
  • In the interactive Python shell created: a Parent isntance and 999 Child instances related to it.
  • In the admin wen to to the Parent model instance modify form and added a 1000th and a 10001 Childs using the ne JS-poweredinline without experimenting any problem (both Child are created without traceback or validation error).

So, the test case exogen describes and attaches is possible, the PK field can be POSTed to the form formatted with a thousand separator (or for instance, corrupted in any other way by e.g. a malicious user, buggy software, etc.). But what we need, I think, is a description of when/how the Django forms machinery is generating that broken value in the first place. That would be a bug.

comment:10 Changed 5 years ago by exogen@…

@ramiro: Creating them is not the problem, because they don't have any formatted primary key values to POST. After saving the parent with the 1000th and 10001st, try saving it again.

comment:11 follow-up: Changed 5 years ago by exogen@…

Alternatively, you could just run the test cases in the attached project: manage.py test testapp

comment:12 in reply to: ↑ 11 Changed 5 years ago by ramiro

Replying to exogen@gmail.com:

Alternatively, you could just run the test cases in the attached project: manage.py test testapp

I've no expressed myself clearly enough. Sorry.

I can run your test case and i see the failure you report. But it is a synthetic case: You are creating the failure condition by posting the PK with a value formatted with a thousand separator. The same error would happen If we put "foo" instead of "123,45" there and then the USE_THOUSAND_SEPARATOR format specifier/setting and the new USE_L10N formating machinery would have nothing to do with it.

What I'm trying to arrive to is to a non theoretical case where this shows itself while using inlines be in the admin or otherwise. We need this so we can have a hint about where start looking for a fix.

USE_THOUSAND_SEPARATOR and the other format specifiers new in 1.2 are related to information presentation and that's why I'd be surprised if they were used to render the hidden HTML form values that are later POSTed back by the browser.

Btw, I can save the Parent object without problem having 1002 related Children. This is the relevant form HTML rendering fragment:

        <tr class="row2 has_original"
             id="child_set1000">

        <td class="original">
          <p>
           Prueba 1
          
            </p>
          <input type="hidden" name="child_set-999-id" value="1000" id="id_child_set-999-id" />
          <input type="hidden" name="child_set-999-parent" value="1" id="id_child_set-999-parent" />
          
        </td>
        
          
            
              <td class="name">
              
                  
                  <input name="child_set-999-name" value="Prueba 1" class="vTextField" maxlength="75" type="text" id="id_child_set-999-name" />

              
              </td>
            
          
        
        
          <td class="delete"><input type="checkbox" name="child_set-999-DELETE" id="id_child_set-999-DELETE" /></td>
        
        </tr>


        <tr class="row1 has_original"
             id="child_set1001">
        <td class="original">
          <p>
           Prueba 2
          
            </p>
          <input type="hidden" name="child_set-1000-id" value="1001" id="id_child_set-1000-id" />

          <input type="hidden" name="child_set-1000-parent" value="1" id="id_child_set-1000-parent" />
          
        </td>
        
          
            
              <td class="name">
              
                  
                  <input name="child_set-1000-name" value="Prueba 2" class="vTextField" maxlength="75" type="text" id="id_child_set-1000-name" />
              
              </td>
            
          
        
        
          <td class="delete"><input type="checkbox" name="child_set-1000-DELETE" id="id_child_set-1000-DELETE" /></td>
        
        </tr>
     
        
        <tr class="row2 has_original"
             id="child_set1002">
        <td class="original">

          <p>
           Prueba 3
          
            </p>
          <input type="hidden" name="child_set-1001-id" value="1002" id="id_child_set-1001-id" />
          <input type="hidden" name="child_set-1001-parent" value="1" id="id_child_set-1001-parent" />
          
        </td>
        
          
            
              <td class="name">
              
                  
                  <input name="child_set-1001-name" value="Prueba 3" class="vTextField" maxlength="75" type="text" id="id_child_set-1001-name" />
              
              </td>

            
          
        
        
          <td class="delete"><input type="checkbox" name="child_set-1001-DELETE" id="id_child_set-1001-DELETE" /></td>
        
        </tr>
     
        
        <tr class="row1  empty-form"
             id="child_set-empty">
        <td class="original">
          
          <input type="hidden" name="child_set-__prefix__-id" id="id_child_set-__prefix__-id" />
          <input type="hidden" name="child_set-__prefix__-parent" value="1" id="id_child_set-__prefix__-parent" />
          
        </td>
        
          
            
              <td class="name">
              
                  
                  <input id="id_child_set-__prefix__-name" type="text" class="vTextField" name="child_set-__prefix__-name" maxlength="75" />

              
              </td>
            
          
        
        
          <td class="delete"></td>
        
        </tr>
     
     </tbody>
   </table>

comment:13 Changed 5 years ago by anonymous

@ramiro: What I'm saying is that when USE_THOUSAND_SEPARATOR and NUMBER_GROUPING are enabled, Django admin outputs the AutoField value with a comma - just as I've done in the test case. It's not synthetic, it's mocking the actual admin form behavior. If you can't reproduce it with your locale, I'm not sure why.

comment:14 Changed 5 years ago by anonymous

Note that you could add this test case to verify:

def test_change_form_outputs_locale_formatted_autofield(self):
    response = self.client.get('/admin/testapp/parent/1/')
    self.assertContains(response,
        '<input type="hidden" name="child_set-0-id" value="12,345"')

comment:15 follow-up: Changed 5 years ago by exogen@…

The key to ramiro's discrepancy may be that USE_I18N and USE_L10N don't currently enable or disable number formatting. To enable it, you have to set USE_THOUSAND_SEPARATOR and NUMBER_GROUPING explicitly. Can you reproduce the behavior when you set those, ramiro?

comment:16 in reply to: ↑ 15 Changed 5 years ago by ramiro

Replying to exogen@gmail.com:

The key to ramiro's discrepancy may be that USE_I18N and USE_L10N don't currently enable or disable number formatting. To enable it, you have to set USE_THOUSAND_SEPARATOR and NUMBER_GROUPING explicitly. Can you reproduce the behavior when you set those, ramiro?

Yes, had arrived to the same conclusion and can reproduce the problem. It seems we have a few concurrent bugs to fix in this front. Thanks exoweb!

Changed 5 years ago by jezdez

Added localize parameter to form fields to be able to selectively enable localization

comment:17 Changed 5 years ago by jezdez

Ok, so I think we may need to bite the bullet and move back a little, not allowing automatic localization of form fields and widgets.

The attached patch adds a localize parameter to form fields which -- if {{{USE_L10N = True}} -- would lead to their values being rendered and parsed by using the localization machinery.

Changed 5 years ago by jezdez

localize as much as possible in the form field

comment:18 Changed 5 years ago by jezdez

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12867]) Fixed #13032 - Added localize parameter to form fields to be able to selectively enable localization.

comment:19 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top