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

Problem: REST API does not accept null for string values #135

Merged
merged 1 commit into from May 22, 2019

Conversation

dkliban
Copy link
Member

@dkliban dkliban commented May 16, 2019

Solution: update serializers to accept null

re: #4676
https://pulp.plan.io/issues/4676

@codecov
Copy link

codecov bot commented May 16, 2019

Codecov Report

Merging #135 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
- Coverage    67.2%   67.17%   -0.04%     
==========================================
  Files          64       64              
  Lines        3034     3034              
==========================================
- Hits         2039     2038       -1     
- Misses        995      996       +1
Impacted Files Coverage Δ
pulpcore/app/serializers/publication.py 91.56% <ø> (ø) ⬆️
pulpcore/app/serializers/progress.py 100% <ø> (ø) ⬆️
pulpcore/app/serializers/repository.py 100% <ø> (ø) ⬆️
pulpcore/app/serializers/content.py 90.81% <ø> (ø) ⬆️
pulpcore/app/models/repository.py 64.51% <100%> (ø) ⬆️
pulpcore/app/models/progress.py 51.72% <100%> (ø) ⬆️
pulpcore/app/viewsets/task.py 97.01% <0%> (-1.5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4f8bac...aef490e. Read the comment docs.

@bmbouter
Copy link
Member

Are there any alternatives to this implementation? I'm concerned from the DRF docs that say "doing so means that there will be two differing types of empty value permissible for string representations, which can lead to data inconsistencies and subtle application bugs."

@dkliban
Copy link
Member Author

dkliban commented May 16, 2019

The alternative is to not accept empty strings and only accept null.

@bmbouter
Copy link
Member

Adding WIP since we have a meeting to discuss tomorrow w/ @jlsherrill

@dkliban
Copy link
Member Author

dkliban commented May 22, 2019

@bmbouter This is ready for re-review.

@dkliban dkliban force-pushed the allow_null branch 2 times, most recently from cebd69e to c7d0b5f Compare May 22, 2019 03:33
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

This looks like exactly what we talked about. Thanks @dkliban !

@bmbouter
Copy link
Member

@dkliban actually it needs a breaking change 3.0 release note...

Solution: update serializers to accept null

This patch also removes the ability to specify an empty string for string fields.

re: #4676
https://pulp.plan.io/issues/4676
Breaking Changes
----------------

All the string fields in the REST API no longer accept an empty string as a value. These fields
Copy link
Member

Choose a reason for hiding this comment

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

this looks great, thanks. +1 to merge

@dkliban dkliban merged commit 6143ca3 into pulp:master May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants