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

Add HStoreField, postgres fields tests #5654

Merged
merged 4 commits into from Jan 15, 2018

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Dec 5, 2017

Fixes #5644

  • Add postgres to optional reqs
  • Add postgres fields mapping tests
  • Add importable HStoreField
  • Ensure HStoreField accepts null values by default

@rpkilby rpkilby added this to the v3.8 milestone Dec 5, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

Awesome! 💃🏼

@carltongibson
Copy link
Collaborator

carltongibson commented Dec 5, 2017

@rpkilby Why did you mark this for 3.8?

@rpkilby
Copy link
Member Author

rpkilby commented Dec 5, 2017

It seemed appropriate to include it with the other composite field changes. But it's not a strong opinion.

@rpkilby
Copy link
Member Author

rpkilby commented Dec 5, 2017

Oh, right - completely forgot.

Django 1.10 does not support null values in HStoreField. I'm assuming we'll drop 1.10 support in the 3.8 release, so it made sense that this change would coincide.

@carltongibson
Copy link
Collaborator

carltongibson commented Jan 15, 2018

@rpkilby I'd like to get this in.

Could you possibly skip the test here for Django 1.10, adding getting rid of that to #5657?

That way we can progress before we make final moves towards 3.8 and/or Dropping 1.10 compat.

Ta!

@rpkilby
Copy link
Member Author

rpkilby commented Jan 15, 2018

Django 1.10 doesn't fail outright - it just saves null values as strings, which is a subtly wrong change in behavior.

@carltongibson
Copy link
Collaborator

carltongibson commented Jan 15, 2018

Hmm. OK. I can't quite see why the test is passing then... 🙂

@carltongibson
Copy link
Collaborator

carltongibson commented Jan 15, 2018

... because I'm conflating the two fields. (Ours and django.contrib.postgres').

Fine. Let's have this as is.

@rpkilby
Copy link
Member Author

rpkilby commented Jan 15, 2018

The test is passing since it's just serializer validation. Nothing is actually being written to the DB. A failing test would look like:

  • write {'foo': None} to hstore field
  • read value of hstore field
  • assert write/read values are identical
  • fail, since 'None' != None

@carltongibson carltongibson merged commit 2709de1 into encode:master Jan 15, 2018
1 check passed
@rpkilby rpkilby deleted the hstore-null-child branch Jan 15, 2018
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Test postgres field mapping

* Add HStoreField

* Ensure 'HStoreField' child is a 'CharField'

* Add HStoreField docs
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

2 participants