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

Prohibit null characters in CharField by default #6073

Merged
merged 2 commits into from Oct 2, 2018

Conversation

jleclanche
Copy link
Contributor

@jleclanche jleclanche commented Jul 8, 2018

Description

Followup on #6068.

I've left the first commit as a diff to the second one. Please feel free to squash.

I'd love if someone else could edit in the documentation example mentioned by @rpkilby because I've no idea how to achieve that :)

@jleclanche jleclanche force-pushed the master branch 2 times, most recently from f69e835 to 1c0b55c Compare Jul 8, 2018
@jleclanche
Copy link
Contributor Author

jleclanche commented Jul 8, 2018

Ready for review

Copy link
Member

@rpkilby rpkilby left a comment

Aside from the one requested change, this looks good to me. 👍

with pytest.raises(serializers.ValidationError) as exc_info:
field.run_validation(value)
assert exc_info.value.detail == [
ProhibitNullCharactersValidator.message
Copy link
Member

@rpkilby rpkilby Jul 9, 2018

Choose a reason for hiding this comment

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

I typically recommend testing against explicit expected values. e.g., the preceding test is:

assert exc_info.value.detail == ['This field may not be blank.']

Copy link
Contributor Author

@jleclanche jleclanche Jul 9, 2018

Choose a reason for hiding this comment

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

Isn't that more prone to errors and random breakage eg. if Django decides to update the strings? Unlike the code itself, strings themselves aren't a reliable API with forward-compatibility for obvious reasons.

I wanted to test against the code (null_characters_not_allowed) but I don't think DRF exposes it, does it?

Copy link
Member

@rpkilby rpkilby Jul 9, 2018

Choose a reason for hiding this comment

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

It is more prone to breaking, but we're depending on the behavior that Django provides. If the behavior changes, it helps to know about it.

I wanted to test against the code (null_characters_not_allowed) but I don't think DRF exposes it, does it?

Hm. I'm not entirely sure how Django and DRF ValidationErrors interact at the moment in regards to the error code. I know some changes to ErrorDetail were made in #5785, but idk if this is easily testable at the field level, or if the error code is preserved.

@jleclanche
Copy link
Contributor Author

jleclanche commented Jul 9, 2018

Updated

rpkilby
rpkilby approved these changes Jul 9, 2018
@rpkilby
Copy link
Member

rpkilby commented Jul 10, 2018

Adding to the 3.9 milestone for consideration.

Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. This looks good.

(@rpkilby: fancy adding the pop example to the release notes for 3.9?)

@rpkilby
Copy link
Member

rpkilby commented Jul 10, 2018

I've never been able to successfully update a PR's commits. Here's what I would add to the changelog:

* Change `CharField` to disallow null bytes. [#6073][gh6073]
  To revert to the old behavior, subclass `CharField` and remove `ProhibitNullCharactersValidator` from the validators.
  ```python
  class NullableCharField(serializers.CharField):
      def __init__(self, *args, **kwargs):
          super().__init__(*args, **kwargs)
          self.validators = [v for v in self.validators if not isinstance(v, ProhibitNullCharactersValidator)]
  ```

@jleclanche
Copy link
Contributor Author

jleclanche commented Aug 19, 2018

Any update on this?
I rebased against master. Can't find a changelog to update.

@codecov-io
Copy link

codecov-io commented Sep 2, 2018

Codecov Report

Merging #6073 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6073      +/-   ##
==========================================
+ Coverage   96.18%   96.18%   +<.01%     
==========================================
  Files         128      128              
  Lines       17624    17637      +13     
  Branches     1459     1461       +2     
==========================================
+ Hits        16951    16964      +13     
  Misses        465      465              
  Partials      208      208

@rpkilby
Copy link
Member

rpkilby commented Sep 5, 2018

Hi @jleclanche - sorry for the delay here. The changelog is in a slightly unusual place. You'd want to edit the release notes

@jleclanche
Copy link
Contributor Author

jleclanche commented Sep 7, 2018

Not sure what to put in there anymore as it's been too long since the context switch. If someone can write something in that'd be great.

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 7, 2018

It’s fine. This will go into 3.9 I’ll put something in the release notes for it. Thanks all.

@rpkilby
Copy link
Member

rpkilby commented Sep 7, 2018

Thanks @jleclanche for putting this together. @carltongibson, notes in #6073 (comment)

@carltongibson carltongibson merged commit 0eb2dc1 into encode:master Oct 2, 2018
1 check passed
carltongibson added a commit to carltongibson/django-rest-framework that referenced this pull request Oct 2, 2018
carltongibson added a commit to carltongibson/django-rest-framework that referenced this pull request Oct 10, 2018
tomchristie added a commit that referenced this pull request Oct 18, 2018
* Release notes to 5174a26

* Update version for v3.9.0

* Removed exclude_from_schema per deprecation policy.

* Updated list_route() and detail_route() deprecations.

* Weakened to PendingDeprecationWarning for `base_name`

cc @rpkilby.

* Add (beginning of) 3.9 release announcement.

@tomchristie: Input on OpenAPI and What’s Next very welcome! :)

* Add announcement section for Extra Actions in Browsable API

* Update release notes and add deprecation note for Django Guardian backend.

* Add release note for #6073

* Add release notes to dd19a44

* Adding release notes

* Update 3.9 announcement

* Add Oct 18 release date
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Implement an allow_null_bytes argument to CharField (default True)
* Switch to using native ProhibitNullCharactersValidator instead
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Release notes to 5174a26

* Update version for v3.9.0

* Removed exclude_from_schema per deprecation policy.

* Updated list_route() and detail_route() deprecations.

* Weakened to PendingDeprecationWarning for `base_name`

cc @rpkilby.

* Add (beginning of) 3.9 release announcement.

@tomchristie: Input on OpenAPI and What’s Next very welcome! :)

* Add announcement section for Extra Actions in Browsable API

* Update release notes and add deprecation note for Django Guardian backend.

* Add release note for encode#6073

* Add release notes to dd19a44

* Adding release notes

* Update 3.9 announcement

* Add Oct 18 release date
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants