feat: add supports for JsonStreamer on indexation #218
Conversation
damienalexandre
left a comment
damienalexandre
left a comment
There was a problem hiding this comment.
Thanks a lot for this nice addition 👍
Would there be a way to make it less "intrusive", meaning having less changes in the core classes signatures?
Otherwise I guess it can only be a major release bump 🤔
| ->args([ | ||
| '$client' => service(Client::class), | ||
| '$serializer' => service('serializer'), | ||
| '$serializer' => abstract_arg('elastically.abstract.document_serializer'), |
There was a problem hiding this comment.
I think that's a BC Break, as the Client $serializer argument is supposed to be a SerializerInterface.
Maybe it should be named differently? ($documentSerializer?)
There was a problem hiding this comment.
@damienalexandre should be Backward compatible now, let me know what you think
| '$serializer' => abstract_arg('elastically.abstract.document_serializer'), | ||
| '$bulkMaxSize' => abstract_arg('bulk size'), | ||
| '$bulkRequestParams' => [], | ||
| '$contextBuilder' => abstract_arg('elastically.abstract.static_context_builder'), |
There was a problem hiding this comment.
Same remark, it could be a BC Break to remove this 🤔
The less we change the Client arguments the better.
There was a problem hiding this comment.
I could do some kind of Adapter implementing Serializer interface and internaly using the streamer, but it feels kinda dodgy
| */ | ||
| private function supports(object $document): bool | ||
| { | ||
| $key = \sprintf('elastically_supports_%s', md5($document::class)); |
There was a problem hiding this comment.
Maybe use the modern and faster xxHash instead of MD5:
| $key = \sprintf('elastically_supports_%s', md5($document::class)); | |
| $key = \sprintf('elastically_supports_%s', hash('xxh128', $document::class)); |
There was a problem hiding this comment.
Good point thanks! I used xxh3 as it is the fastest and collision are unlikely
GaryPEGEOT
changed the title
|
Saddly I cannot use the JsonStreamer to read the response as it uses the denormalizer (which is normal as data is already an array at this point) |
3 participants
Fixes #216