Pyelasticsearch 0.4 compatibility (fairly urgent) #759

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@erikrose

This will keep people from getting errors when they try to use haystack with yesterday's pyelasticsearch release.

erikrose added some commits Mar 20, 2013
@erikrose erikrose Port to_python over from pyelasticsearch so django-haystack works wit…
…h 0.4.

This should keep people from getting "hit in the face" with errors when they try to use the haystack ES backend. :-) pyelasticsearch/pyelasticsearch#86

We removed the internally unused and undocumented to_python in pyelasticsearch 0.4. I copied the version from 4266529eaefb96af0a8e99017740cbb56608dbe4 and removed the `six` dependencies.

I'm actually not sure why haystack needs to call anything like to_python itself; pyelasticsearch implicitly converts most data types, though datetimes are a notable hole. But this is a quick fix.
181bbc2
@erikrose erikrose Add myself to AUTHORS. ac5a0c4
@erikrose erikrose referenced this pull request in pyelasticsearch/pyelasticsearch Mar 20, 2013
Closed

Re-add to_python #86

@erikrose

I'm having some trouble porting it to the 1.2.X branch and wouldn't mind a hand.

@acdha
Contributor
acdha commented Mar 20, 2013

1.2.X looks like you'd have to create an entire elasticsearch backend. I'm voting that since 1.2.x never shipped with ES we leave it that way.

@acdha acdha added a commit to acdha/django-haystack that closed this pull request Mar 20, 2013
@acdha acdha Support pyelasticsearch 0.4 change (closes #759)
pyelasticsearch 0.4 removed the `to_python` method Haystack used.

Thanks to @erikrose for the quick patch
8e86909
@erikrose

Ah, how about that. I just assumed that 1.2.7 had ES. That explains the git deleted-file merge conflicts. :-) Thanks for the quick merge!

@acdha
Contributor
acdha commented Mar 20, 2013

Thanks for the high-quality patch ;)

@orf
orf commented Mar 20, 2013

Woah, wait. Eval? Really?

@acdha
Contributor
acdha commented Mar 20, 2013

@orf I'm mixed on that - it's ugly but people have been running that for awhile. I'd like to remove it - first might be checking to see what breaks when it's removed (I'm assuming dict/list support) and seeing if we could repurpose the existing JSON decoder instead.

@erikrose

I think the ultimate solution is to get rid of any analog of to_python completely in haystack. pyelasticsearch should cover it. I'm working on date/datetime recognition, which is the only thing it's missing, right now.

@orf
orf commented Mar 21, 2013

A JSON based decoder might be faster and definitely more secure. _to_python gives no indication that only "safe" strings should be passed to it, what if a future code change accidentally allowed user inputted strings to reach the eval()?

@floppya floppya added a commit to floppya/django-haystack that referenced this pull request Mar 29, 2013
@acdha @floppya acdha + floppya Support pyelasticsearch 0.4 change (closes #759)
pyelasticsearch 0.4 removed the `to_python` method Haystack used.

Thanks to @erikrose for the quick patch
f149ead
@jezdez jezdez added a commit that referenced this pull request Apr 3, 2013
@jezdez jezdez Port the `from_python` method from pyelasticsearch to the Elasticsear…
…ch backend, similar to `to_python` in 181bbc2.

Fixes #762. Refs #759.
5409197
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment