#2168 - proposal replace meilisearch elasticsearch with seal#2408
#2168 - proposal replace meilisearch elasticsearch with seal#2408MrHDOLEK wants to merge 10 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 54983 +1015
+ Misses 9564 9554 -10 🚀 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
| seal_drop_index($this->engine, $this->indexName); | ||
| } | ||
|
|
||
| seal_create_index($this->engine, $this->indexName); |
There was a problem hiding this comment.
so in general I dont think that SealContext shold expect Schema, oposite, I think it should just expose methods that would simplify creating the engine for specific test.
For example:
SealContext::engine(Seal\Schema $schema)
and then in each test that requires a specific schema you should actually define a FlowSchema and use to_seal_schema to convert FlowSchema automatically to Seal Schema
| static::assertSame(1, $items[0]['quantity']); | ||
| } | ||
|
|
||
| protected function schema(): Schema |
There was a problem hiding this comment.
we should try to avoid at all cost protected/private methods in tests
|
|
||
| final class SealAllEntryTypesTest extends SealTestCase | ||
| { | ||
| public function test_round_trip_of_all_flow_entry_types(): void |
There was a problem hiding this comment.
I would make this test heavier, I would build here a proper ETL pipelines that:
Indexing Pipeline
- extract from StaticOrdersExtractor (like 100 rows)
- load to Seal
- Assert inserted rows (passing analyze() to makes DF return report)
Extracting Pipeline
- Extract from seal
- fetchToArray
- assert
| { | ||
| public function test_converting_flow_schema_to_a_seal_index(): void | ||
| { | ||
| $sealSchema = (new SchemaConverter())->toSealSchema( |
There was a problem hiding this comment.
in tests we should use DSL methods
| 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
|
|
||
| ## Description | ||
|
|
||
| ETL Adapter that provides Loaders and Extractors that work with any search engine supported by Seal. |
There was a problem hiding this comment.
We gonna need more than this :D
| } | ||
|
|
||
| #[DocumentationDSL(module: Module::SEAL, type: Type::HELPER)] | ||
| function seal_create_index(EngineInterface $engine, string $index): void |
There was a problem hiding this comment.
I dont think we should be exposing those seal_ functions, they are rather Seal related than Flow. DSL purpose is to cover Flow features
| $this->engine->bulk( | ||
| $this->index, | ||
| (new RowsNormalizer(new EntryNormalizer()))->normalize($rows), | ||
| [], |
There was a problem hiding this comment.
this stands for deleteDocumentIdentifiers - similarly to how Docrtine and Flow PostgreSql is done, we should also allow to delete documents not only index them with this SealLoader
| 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