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

only allow integer field to accept integers and strings, fixes #2835 #2836

Merged
merged 1 commit into from Apr 20, 2015

Conversation

ryangallen
Copy link
Contributor

@ryangallen ryangallen commented Apr 17, 2015

No description provided.

@kevin-brown
Copy link
Member

kevin-brown commented Apr 17, 2015

I can't say I'm thrilled at the idea of doing type checking here, but I will point out that in Python 2 you also have to be able to handle long. And you can pass in a tuple as the second argument to isinstance instead of doing or multiple times.

Django itself handles this by calling str before int (and also stripping trailing decimals). This will trigger errors if decimal values are passed in, and it avoids type checking.

@ryangallen
Copy link
Contributor Author

ryangallen commented Apr 17, 2015

Thanks Kevin, good call. I adjusted and squashed the change.

…s being stored as ints, fixes encode#2835.

Match IntegerField validation with Django IntegerField, prevents decimal values being stored as ints, fixes encode#2835
 On branch master
@jpadilla
Copy link
Member

jpadilla commented Apr 20, 2015

LGTM

kevin-brown added a commit that referenced this pull request Apr 20, 2015
only allow integer field to accept integers and strings, fixes #2835
@kevin-brown kevin-brown merged commit 5cc0a3c into encode:master Apr 20, 2015
@kevin-brown
Copy link
Member

kevin-brown commented Apr 20, 2015

Looks good, thanks for making the change.

@@ -682,7 +682,7 @@ def to_internal_value(self, data):
self.fail('max_string_length')

try:
data = int(data)
data = int(re.compile(r'\.0*\s*$').sub('', str(data)))
Copy link
Member

@tomchristie tomchristie Apr 21, 2015

Choose a reason for hiding this comment

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

  1. We should push this to self.re_decimal as per in Django so that it's only compiled once.
  2. We should comment that this regex substitution is so that we allow eg '1.0' as an int, but not '1.2'

Copy link
Contributor Author

@ryangallen ryangallen Apr 21, 2015

Choose a reason for hiding this comment

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

@tomchristie
Copy link
Member

tomchristie commented Apr 21, 2015

Coupla inline comments that would be good to resolve.

@tomchristie tomchristie added this to the 3.1.2 Release milestone Apr 21, 2015
tinchodipalma added a commit to tinchodipalma/django-rest-framework that referenced this pull request May 25, 2015
The following items had a wrong href value:

- Dont require pk strictly for related fields. (encode#2745, encode#2754)
- Restrict integer field to integers and strings. (encode#2835, encode#2836)
@tinchodipalma tinchodipalma mentioned this pull request May 25, 2015
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