Skip to content
This repository has been archived by the owner on May 11, 2021. It is now read-only.

Auto create does not work with django-jsonfield 1.0.3 #97

Closed
Jaidan opened this issue Sep 29, 2016 · 9 comments
Closed

Auto create does not work with django-jsonfield 1.0.3 #97

Jaidan opened this issue Sep 29, 2016 · 9 comments

Comments

@Jaidan
Copy link

Jaidan commented Sep 29, 2016

Setup.py allows django-jsonfield>=0.9.2. This is not true as it appears changes exist in all version >= 1.0 (a validation check) that are breaking to Gargoyle during flag autocreation.

@adamchainz
Copy link
Owner

Hi, it's working for me on 1.0.1. Please paste more details!

@adamchainz
Copy link
Owner

Also the root problem is technically in django-modeldict-yplan ( https://github.com/YPlan/django-modeldict ), which is what depends on django-jsonfield.

@Jaidan
Copy link
Author

Jaidan commented Sep 29, 2016

Interesting. We're running into the issue when running tests, though I don't see any reason to believe it wouldn't happen normally as well. The first is_active the test runs into results in: https://github.com/YPlan/gargoyle/blob/master/gargoyle/manager.py#L61. This cascades down into django-jsonfield. Where it eventually fails due to https://github.com/bradjasper/django-jsonfield/blob/1.0.3/jsonfield/fields.py#L81

Above that we're seeing a call in django.db.models to getdefault. This returns an empty string for the Switch.value field. The empty string causes the ValueError in jsonfield.

I'm not positive that the underlying modeldict is the right place to fix this (though maybe). We're going to try a few things (like specifying a default for the JSONField) that could be done by gargoyle.

@Jaidan
Copy link
Author

Jaidan commented Sep 29, 2016

To be fair it could be something we've customized, and I haven't fully ruled that out yet. But after some severe spelunking yesterday all signs pointed to empty string causing issues. We are moving over from a heavily customized (to remove the original non threadsafe caching) fork of the original discus gargoyle. I'll be digging into it more today as well, so I'll let you know if anything falls out (as well as a pull request if I find a fix and rule out our own code conflicting.

@Jaidan Jaidan changed the title Does not with with django-jsonfield 1.0.3 Auto create does not work with django-jsonfield 1.0.3 Sep 29, 2016
@adamchainz
Copy link
Owner

The caching in django-modeldict-yplan might still not be threadsafe, there was a fix, but adamchainz/django-modeldict#18 is still open.

You've linked to jsonfield not django-jsonfield that's hosted at https://bitbucket.org/schinckel/django-jsonfield . django-modeldict-yplan can only use the latter afaik.

Also please try django-jsonfield 1.0.2, that's what we're currently using.

Oh, and also, what database are you using? There was a problem in django-jsonfield with postgres that caused #86 .

@Jaidan
Copy link
Author

Jaidan commented Sep 29, 2016

My testing in our staging environment was unable to reproduce the non threadsafe behavior when using the yplan fork.

Our flags are all in our mysql database.

That said...I had no idea there were two packages and they overlap eachother's namespace. We've been accidently using the wrong one this whole time...which worked fine until we tried the fork. We have both installed apparently and we're losing. Pip happily installed both and we have jsonfield pinned for use in with other pieces of the project.

I think we can close this out...yplan-gargoyle doesn't work with jsonfield, which it isn't trying to but happens to have the same namespace and a nearly identical api as the correct package django-jsonfield. And if you have both installed bad things happen.

@Jaidan Jaidan closed this as completed Sep 29, 2016
@adamchainz
Copy link
Owner

Yes pip doesn't notice package collisions.

Maybe we can add a check to django-modeldict-yplan that detects if jsonfield is installed rather than django-jsonfield.

@Jaidan
Copy link
Author

Jaidan commented Sep 29, 2016

We actually had both installed. Someone over a year ago installed jsonfield in the project for some reason. Pip happily installed the django-jsonfield==1.0.1 in accordance with gargoyle's setup.py. So this would be pretty hard to detect, in our development environments django-jsonfield got lucky and won in our production and CI environment jsonfield won.

@adamchainz
Copy link
Owner

I think it would still be possible, there might be some difference we can notice at import time, or by checking the list of installed packages. Added #98 to do this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants