Fix timefield serialize elasticsearch #13193
Conversation
|
@Talha-Rizwan thanks for picking up a PR and adding tests. Could you please look at the CI failures and work through them, including adoption of the recommendation in the comment above. |
|
@zerolab are you able to do another review? |
|
@lb- will have a look tomorrow |
There was a problem hiding this comment.
Thank you @Talha-Rizwan, a few more tweaks mostly around the new test case which I feel should be folded in TestElasticsearch7Mapping
|
|
||
| def setUp(self): | ||
| self.author = models.Author.objects.create(name="Test Author") | ||
| self.backend = Elasticsearch7SearchBackend({}) |
There was a problem hiding this comment.
please use get_search_backend("elasticsearch")
| self.index.delete() # Reset the index just to be sure | ||
| self.index.put() | ||
| self.index.add_model(models.Book) |
There was a problem hiding this comment.
question: why is this needed?
If 100% required, I'd use already established patterns, like https://github.com/wagtail/wagtail/blob/main/wagtail/search/tests/test_page_search.py#L16-L21
| try: | ||
| self.index.add_item(self.book) | ||
| success = True | ||
| error_message = None | ||
| except ( | ||
| ElasticsearchException, | ||
| RequestError, | ||
| SerializationError, | ||
| TypeError, | ||
| ) as e: | ||
| success = False | ||
| error_message = str(e) | ||
|
|
||
| self.assertTrue( | ||
| success, f"TimeField indexing should succeed. Error: {error_message}" | ||
| ) |
There was a problem hiding this comment.
| try: | |
| self.index.add_item(self.book) | |
| success = True | |
| error_message = None | |
| except ( | |
| ElasticsearchException, | |
| RequestError, | |
| SerializationError, | |
| TypeError, | |
| ) as e: | |
| success = False | |
| error_message = str(e) | |
| self.assertTrue( | |
| success, f"TimeField indexing should succeed. Error: {error_message}" | |
| ) | |
| self.index.add_item(self.book) |
and let the test suite throw whatever exceptions it needs to
| try: | ||
| serializer = JSONSerializer() | ||
| serializer.dumps(document) | ||
| json_serializable = True | ||
| except (TypeError, ValueError): | ||
| json_serializable = False | ||
|
|
||
| self.assertTrue( | ||
| json_serializable, | ||
| "Document should be JSON serializable with Elasticsearch JSONSerializer", | ||
| ) |
There was a problem hiding this comment.
I don't think this adds any value here since other tests already check the serialisation (e.g. TestElasticsearch7Mapping.test_get_document)
| @@ -1450,3 +1459,94 @@ def test_urls(self, Elasticsearch): | |||
| ], | |||
| timeout=10, | |||
| ) | |||
|
|
|||
|
|
|||
| @unittest.skipIf(ELASTICSEARCH_VERSION[0] != 7, "Elasticsearch 7 required") | |||
There was a problem hiding this comment.
I left a few comments on specific things in the test case, but the more I look at this, the more I am convinced the tests in this test case should be folded in the other test cases, or are already covered (like the mapping or getting the results)
e.g. TestElasticsearch7Mapping.test_get_mapping should cover test_timefield_mapping and TestElasticsearch7Mapping.test_get_document (this last one would need another with a publication_date value)
|
(drive-by comment, not a full review...) Please could you add some tests to confirm that running a search filtered by a Also, please can we make sure that we run these tests against all Elasticsearch versions, not just version 7? We don't want to ship a fix that only works on Elasticsearch 7 and breaks again when people upgrade to 8. This could be done by putting these tests in |
Issue Tracking: #13032
Extension of PR : #13080
Added tests to verify that TimeField serialization in elastic-search is now fixed.