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

DateTimeField does not handle empty values correctly (#3726) #3731

Merged
merged 6 commits into from Dec 18, 2015

Conversation

mjparker777
Copy link
Contributor

@mjparker777 mjparker777 commented Dec 13, 2015

Closes #3726

Added code to handle None and empty string.
Also added outputs in testing.

@jpadilla
Copy link
Member

jpadilla commented Dec 13, 2015

Like #2869 (comment) says maybe we should also check out if TimeField needs any fixing. That could be a separate PR as well.

@mjparker777
Copy link
Contributor Author

mjparker777 commented Dec 13, 2015

You are correct. I updated the timefield and the test output for timefield also.

@mjparker777
Copy link
Contributor Author

mjparker777 commented Dec 13, 2015

FYI - I see that someone added a string conversion in datefield:

            if (isinstance(value, str)):
                value = datetime.datetime.strptime(value, '%Y-%m-%d').date()

I think this is problematic.
First the format is very specific. It requires the order of Y m d and the - as the separator.
I dont think this should be carried into the time and datetime fields (I actually think it should be removed). This would require string conversion methods for each that would require covering all of the possible string representations for each.

The datetime, date, and time fields have options to allow the fields to not be required so covering empty options is needed but other than that it should be the correct object i.e. datetime, date or time.

Thoughts? If you agree then this pull request should be good to go.

@xordoquy xordoquy modified the milestones: 3.3.2 Release, 3.3.3 Release Dec 14, 2015
@mjparker777
Copy link
Contributor Author

mjparker777 commented Dec 16, 2015

I am not sure what the labels mean? Does the "Needs further review" apply to me or someone else? Are we good on this?

@@ -1183,6 +1186,9 @@ def to_internal_value(self, value):
self.fail('invalid', format=humanized_format)

def to_representation(self, value):
if not value:
Copy link
Member

@tomchristie tomchristie Dec 17, 2015

Choose a reason for hiding this comment

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

You shouldn't need to handle the None case explicitly, as I believe that's handled as a catch all.

What's the behavior if you don't include this?

Is this behavior in order to catch empty strings and return None for them? (In which case I suspect this may be valid)

Copy link
Contributor Author

@mjparker777 mjparker777 Dec 17, 2015

Choose a reason for hiding this comment

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

Hi Tom - If None is passed in then when it gets to this method it is an empty string ''.
With completed = serializers.DateTimeField(required=False) and completed is None you see this output:

2015-12-17 18:58:53,631 ERROR RequestDetailsHandler::get::371 
Traceback (most recent call last):
  File "/x/local/aitv2/automatic_integration_v2/eci/handlers/RequestDetailsHandler.py", line 368, in get
    return response.Response(RequestDetailsResponseSerializer(response_data).data,
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 503, in data
    ret = super(Serializer, self).data
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 239, in data
    self._data = self.to_representation(self.instance)
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 472, in to_representation
    ret[field.field_name] = field.to_representation(attribute)
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 614, in to_representation
    self.child.to_representation(item) for item in iterable
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 472, in to_representation
    ret[field.field_name] = field.to_representation(attribute)
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 472, in to_representation
    ret[field.field_name] = field.to_representation(attribute)
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/fields.py", line 1071, in to_representation
    value = value.isoformat()
AttributeError: 'str' object has no attribute 'isoformat'

@tomchristie
Copy link
Member

tomchristie commented Dec 17, 2015

Minor point, but it'd be good if DateField mirrored these in layout too - i.e. move this https://github.com/tomchristie/django-rest-framework/pull/3731/files#diff-af402f515db75c7c6754453cf15455d9L1123 to the top of the method.

Otherwise I think this is looking good, tho just some clarification on exactly what those tests end up with if we don't include the fix would be helpful. Is it only the blank string case that is currently broken?

@mjparker777
Copy link
Contributor Author

mjparker777 commented Dec 17, 2015

Hi Tom -
Modified datefield to match the other two.

If None is passed in then when it gets to this method (to_representation(self, value)) it (value) is an empty string ''.
With completed = serializers.DateTimeField(required=False) and completed is None you see this output:

2015-12-17 18:58:53,631 ERROR RequestDetailsHandler::get::371 
Traceback (most recent call last):
  File "/x/local/aitv2/automatic_integration_v2/eci/handlers/RequestDetailsHandler.py", line 368, in get
    return response.Response(RequestDetailsResponseSerializer(response_data).data,
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 503, in data
    ret = super(Serializer, self).data
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 239, in data
    self._data = self.to_representation(self.instance)
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 472, in to_representation
    ret[field.field_name] = field.to_representation(attribute)
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 614, in to_representation
    self.child.to_representation(item) for item in iterable
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 472, in to_representation
    ret[field.field_name] = field.to_representation(attribute)
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/serializers.py", line 472, in to_representation
    ret[field.field_name] = field.to_representation(attribute)
  File "/Users/miparker/.virtualenvs/aitv3/lib/python2.7/site-packages/rest_framework/fields.py", line 1071, in to_representation
    value = value.isoformat()
AttributeError: 'str' object has no attribute 'isoformat'

tomchristie added a commit that referenced this pull request Dec 18, 2015
Issue 3726 DateTimeField not handling empty values
@tomchristie tomchristie merged commit 18cdfcd into encode:master Dec 18, 2015
2 checks passed
@xordoquy xordoquy changed the title Issue 3726 DateTimeField not handling empty values DateTimeField does not handle empty values correctly (#3726) Feb 11, 2016
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