#2168 - proposal replace meilisearch elasticsearch with seal#2408
#2168 - proposal replace meilisearch elasticsearch with seal#2408MrHDOLEK wants to merge 11 commits into
Conversation
…ch-with-seal # Conflicts: # composer.json # composer.lock # phpstan.neon
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 1.x #2408 +/- ##
============================================
+ Coverage 84.94% 85.19% +0.24%
- Complexity 20648 20971 +323
============================================
Files 1570 1586 +16
Lines 63532 64537 +1005
============================================
+ Hits 53968 54982 +1014
+ Misses 9564 9555 -9 🚀 New features to boost your workflow:
|
|
@MrHDOLEK as we discussed offline, please find some feedback and guidance below. Extractor / LoaderIn general I like the idea of having one extractor that accepts Seal EngineInterface which is aligned with how Doctrine Adapter works right now. Same goes for the loaders. It would also work very well with frameworks as seal provides a bundle for symfony that literally configures engine so to use it with flow it would be a simple matter of dependency injection. What to testI had to think about it a bit more but I dont think we should retest every backend that seal is testing. Doctrine adapter is testing MySQL, PostgreSql nad Sqlite because flow-php/docrtine-dbal-bulk which extends doctrine default behaviors that needs to be tested. At this time seal provides following adapters:
I would chose one of them (that requires the least configuration and is the easiest to setup in docker) and cover it with proper integration tests that would confirm that moving data from flow to seal works as expected. How to testSo in general we want to cover in tests that seal adapter can handle all types of flow Entries. You can achieve that by using one of the two extractors: They both expose static method SchemaConverterSchema converters are recursive algorithms that can convert schema in both directiosn Flow to Seal and Seal to Flow. You can find some inspirations here: It might be the easiest to use LLM to help you create one based on those 3 examples for Seal (it's a recursive brain damaging exercise that might not be worth spending time on). Of course schema converters would need a DSL method. Search Engine in TestsSo in this PR you are using traits to configure backends in tests, which is fine but there is a different pattern which I found cleaner and easier to maintain. Contexts. Here is a good example of DatabaseContext that is used in DatabaseTableListCommandTest If there will be more than one integration tests you can extract an abstract In case of any questions, you know where to find me 😁 |
norberttech
left a comment
norberttech
left a comment
There was a problem hiding this comment.
Looks much better! I left some comments, nothing critical, but there is one gap related to Delete operation.
Please let me know if you have any questions about those comments!
| $this->sealContext = null; | ||
| } | ||
|
|
||
| protected function schema(): Schema |
There was a problem hiding this comment.
I would avoid any kind of central schema at the test suit level. It's creating similar problem to tests that depends on db loaded fixtures. All tests using this are now fully depending on that schema which makes them fragile as any adjustment to this schama might break them.
Each test should define its own schema and SealTestCase should only expose the Contexts, in this case SealContext
| use Flow\ETL\Rows; | ||
| use Generator; | ||
|
|
||
| final readonly class RowsNormalizer |
There was a problem hiding this comment.
this one is missing unit tests but even more important, EntryNormalizer is missing them
| MYSQL_ROOT_PASSWORD: root | ||
| networks: | ||
| - flow-php | ||
| elasticsearch: |
There was a problem hiding this comment.
are we sure we dont wanna keep at least one real search engine in the repo? I mean I know we should be able to trust seal it works exactly same way but... Maybe keeping elasticsearch wouldnt be a bad idea and use it in integration tests keeping memory adapter for unit tests? (no strong opinion about this one yet, I never used seal so dont know how much in memory implementation is different from actual one)
|
|
||
| enum Operation: string | ||
| { | ||
| case DELETE = 'delete'; |
There was a problem hiding this comment.
It's more replace than delete cause data will be available, no?
| return; | ||
| } | ||
|
|
||
| if (count($documents) < $this->pageSize) { |
There was a problem hiding this comment.
Instead of counting, add counter variable and increment it in loop?
| { | ||
| $engine = new Engine(new MemoryAdapter(), $schema); | ||
|
|
||
| foreach (array_keys($schema->indexes) as $index) { |
There was a problem hiding this comment.
| foreach (array_keys($schema->indexes) as $index) { | |
| foreach ($schema->indexes as $index => $value) { |
3 participants
The Elasticsearch and Meilisearch adapters are replaced by a single SEAL adapter
(
flow-php/etl-adapter-seal) built on top of SEAL— a PHP Search Engine Abstraction Layer. One adapter now covers Elasticsearch, OpenSearch,
Meilisearch, Algolia, Solr, Typesense, RediSearch and Loupe: the user builds a
CmsIg\Seal\EngineInterfaceand passes it to the DSL, exactly like the PostgreSQL/Doctrineadapters take a client. Tests are backend-agnostic (Memory adapter) and prove every Flow
Entry type survives a round-trip through the adapter.
Resolves: #2168
Change Log
Added
flow-php/etl-adapter-seal) — a single, strongly typed integration for SEAL search engines (Elasticsearch, OpenSearch, Meilisearch, Algolia, Solr, Typesense, RediSearch, Loupe)from_seal()extractor andto_seal()loader, working with any SEALEngineInterfaceto_seal_schema()andseal_schema_to_flow()DSL — recursive, bi-directional Flow Schema ↔ SEAL Schema conversion (nested structures, lists and maps)seal_create_index(),seal_drop_index(),seal_create_schema()andseal_drop_schema()DSL index-lifecycle helpersFixed
Changed
Removed
flow-php/etl-adapter-elasticsearch) — superseded by the SEAL adapter; use SEAL with the Elasticsearch backend (cmsig/seal-elasticsearch-adapter)Deprecated
Security