-
Notifications
You must be signed in to change notification settings - Fork 340
Fix extraction in Solr >= 8. #490
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
base: master
Are you sure you want to change the base?
Conversation
|
@DhavalGojiya Your review, please. |
|
The difference between Solr ≤ 7 and Solr ≥ 8 is clear:
However, since the conditional logic was added in this PR at Otherwise, the test case will start to break. We are running tests only against Solr 4.10.4, so it's not an issue right now, but I recommend updating this as well. 1- tests.test_client.SolrTestCase.test_extractLine 1070 in 9b35283
The updated code should be: m = extracted["metadata"]
if "file" in m["stream_name"]:
# For Solr >= 8.
self.assertEqual(["file"], m["stream_name"])
else:
# For Solr <= 7.
self.assertEqual([fake_f.name], m["stream_name"])2- tests.test_client.SolrTestCase.test_extract_special_char_in_filenameLine 1113 in 9b35283
Updated code: m = extracted["metadata"]
if "file" in m["stream_name"]:
# For Solr >= 8.
self.assertEqual(["file"], m["stream_name"])
else:
# For Solr <= 7.
self.assertEqual([quote(fake_f.name.encode("utf-8"))], m["stream_name"])My Local test resultsdhaval@lg-ubuntu:~/Open-Source/pysolr$ uv run python -m unittest -v tests.test_client.SolrTestCase.test_extract
test_extract (tests.test_client.SolrTestCase.test_extract) ... ok
----------------------------------------------------------------------
Ran 1 test in 0.539s
OK
dhaval@lg-ubuntu:~/Open-Source/pysolr$ uv run python -m unittest -v tests.test_client.SolrTestCase.test_extract_special_char_in_filename
test_extract_special_char_in_filename (tests.test_client.SolrTestCase.test_extract_special_char_in_filename) ... ok
----------------------------------------------------------------------
Ran 1 test in 0.499s
OK |
|
This script for starting Solr servers at If you temporarily need to test and verify changes related to this PR, I have created a complete Docker based solution. Here is my services:
solr:
image: solr:8.9.0 # Update this tag to switch between Solr <= 7 and Solr >= 8
container_name: solr-standalone
ports:
- "8983:8983"
volumes:
- solrdata:/var/solr
environment:
- SOLR_CORE0=core0
- SOLR_CORE1=core1
- CONFIGSET=/opt/solr/server/solr/configsets/sample_techproducts_configs
command: >
bash -c "
precreate-core $$SOLR_CORE0 $$CONFIGSET &&
precreate-core $$SOLR_CORE1 $$CONFIGSET &&
exec solr-foreground
"
volumes:
solrdata:
and then
|
|
I have quite good experience with working Apache Solr at Docker, Docker compose and server level. Do we want to consider replacing the current Also, the current script is hard-coded to Solr version 4.10.4, and it breaks when we try to use newer versions of solr. |
|
Yes, please to Solr in Docker in GitHub Actions. Given Solr versions, I doubt that Solr < v7 is worth worrying about. Perhaps even Solr v7 is not worth worrying about. |
I think we can directly jump to Solr 9.x.x versions. |
acdha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable but I'm also wondering whether we should simply let this be a breaking change in a non-patch release since the Solr project themselves are phasing out Solr 8 support as its end of life approaches:
Does pysolr follow semver? |
I think this comes down to what you think of as a breaking change: if this required you to update your Python code, it'd definitely become 4.0.0 but I'm not sure dropping support for version of Solr which hasn't been supported in years is the same. |
Yeah, we are going to change/update/add only the test case and assertions with new Solr versions after dropping old Solr version support. The actual Pysolr package source code remains unchanged, so a new minor bump to |

Related to django-haystack/django-haystack#2000
Didn't run the test suite...