Repository navigation
Store the data of a tenant-aware entity in a separate search index per tenant - #5769
fractal3000 wants to merge 20 commits into
Conversation
(cherry picked from commit 78953206ec282c0a380f7725722ed2089a5c11a5)
…s the cluster (cherry picked from commit 9f36bb2e25544d2a2252ed85088bfd6c4198868c)
(cherry picked from commit c071f5465b7ed5f3afe47c223fbadfafa91a0ab5)
(cherry picked from commit 60900db99687eec2df4fe3e26bc121dd4fbe6c83)
…dex is missing (cherry picked from commit 7de6e4c711a9606ee9d4deaa840c7068e36a98c0)
…n use (cherry picked from commit 031d8ad2feb99807dddea733f31cc78e192aa6f3)
…ng queue (cherry picked from commit 5dd6fe1596139c2626d2acc52df7bdf86b628f7f)
(cherry picked from commit d654102941b18baeb23628b36b03b8793f4cf183)
… worded (cherry picked from commit 5ddb3d407fd3d4e4af78b98682b43f6d69d917b9)
…efinitions at startup (cherry picked from commit 56cb88bd1547284bd3bdd651d69727910a6faacc)
(cherry picked from commit 1526cbfc10bad01f3563ededa0b14060bf0d3aad)
(cherry picked from commit b908c92eabab2d46935c3822229c355982e85509)
(cherry picked from commit 77464c313e349e751299d08a983cc6ce1de9f37d)
(cherry picked from commit 0c0789fd42426c4c5c57b0cef187fb375d5936ae)
(cherry picked from commit a7d66e2a6b8fdda4c9f1b1ad2e04e3495410cd3c)
(cherry picked from commit 70bc3a773e74dec70efdc2f2f5b23fff76b3e2a1)
…formatting (cherry picked from commit 58c6e5e0362b939d263363531f9617c182414acb)
(cherry picked from commit 975cbdbc1b1faaf0912f5d48407f228da6b3dc66)
Gavrilov-Ivan
left a comment
There was a problem hiding this comment.
General notes & questions:
- What will super-user find if he searches tenant-aware entity in multitenancy env?
- Why can't we index data of super-user into separate index (for tenant-aware entity)? Any limitations?
- Why exactly do we need tenant id in queue item? what will be if tenant id is not specified? or specified incorrectly?
- We need to do something with lefover sessions of async enqueuing
| */ | ||
| @Internal | ||
| @Component("mten_TenantLoader") | ||
| public class TenantLoader { |
There was a problem hiding this comment.
- Need to retest.
Tenant condition is applied to Query as additional criteria and constraint\unconstraint DM doesn't matter.
Run itasSysteminstead. And related tests are useless because you actually mocks the outcome. - Do we need so restricted fetch plans here instead of just local?
- Do we need this class at all?
| * tenant, and these indexes become available independently of each other. | ||
| */ | ||
| @Component("search_IndexStateRegistry") | ||
| public class IndexStateRegistry { |
There was a problem hiding this comment.
API methods changed their parameter from entityName to indexName.
But signatures are still the same.
Silent breaking change.
Looks like there should be new methods with new names with the indexName parameter.
Deprecate old methods if there is still possible to "redirect" call from entityName to indexName. Remove otherwise.
| * Provides functionality for enqueuing entity instances and processing the queue. | ||
| */ | ||
| @NullMarked | ||
| public interface IndexingQueueManager { |
There was a problem hiding this comment.
BIg overgrowth of API due to massive overriding.
Looks like we should introduce some kind of scope-like object to pass it as an argument.
New methods have default behavior. Old methods redirect to the scoped ones.
| * @param tenantId tenant the record belongs to, or {@code null} if it is unknown | ||
| * @return amount of enqueued instances | ||
| */ | ||
| int enqueueIndexByEntityId(Id<?> entityId, @Nullable String tenantId); |
There was a problem hiding this comment.
Bad design.
What if provided tenantId doesn't match the actual tenant of the instance with the provided Id.
Confusing at least. Error-prone at most.
| * @return tenant of the record, or null | ||
| */ | ||
| @Nullable | ||
| String getTenant(Object entity); |
| * the record was saved outside this data store, its tenant attribute was not loaded, or it has none | ||
| */ | ||
| @Nullable | ||
| protected String rememberedTenant(Id<?> entityId) { |
| * @return tenant of the record, or {@code null} if the entity has no tenant attribute or the value is not set | ||
| */ | ||
| @Nullable | ||
| protected String tenantOfDeletedRecord(MetaClass metaClass, AttributeChanges changes) { |
| .collect(Collectors.toSet()); | ||
| } | ||
|
|
||
| protected void assertReady() { |
| * for search using the provided {@link VirtualSubfieldsProvider}. | ||
| * | ||
| * @param entities list of entity names to evaluate for search scope. If empty, all indexed entities are considered. | ||
| * @param entities list of entity names to evaluate for search scope. If empty, all indexed entities are considered. |
| </changeSet> | ||
| </databaseChangeLog> No newline at end of file | ||
|
|
||
| <changeSet author="search" id="4"> |
Gavrilov-Ivan
left a comment
There was a problem hiding this comment.
General notes & questions:
- What will super-user find if he searches tenant-aware entity in multitenancy env?
- Why can't we index data of super-user into separate index (for tenant-aware entity)? Any limitations?
- Why exactly do we need tenant id in queue item? what will be if tenant id is not specified? or specified incorrectly?
- We need to do something with lefover sessions of async enqueuing
Fixes #4191.
An entity with a
@TenantIdattribute was mapped to one shared index. Documents of different tenants lived together, and nothing in the search path separated them: the add-on constrains database queries, not the search engine. Such an entity is now mapped to one index per tenant,search_index_<entityName>_<tenantId>. Isolation became a property of storage instead of a property of the query.Everything else follows from that. An index name is computed rather than stored. A deletion carries the tenant, because after the fact there is nowhere to read it from. An administrative operation returns one row per affected index.
The commercial
jmix-dynmodelmodule consumes the changed API and is updated separately, in jmix-framework/jmix-premium#323.Upgrading
Index names change for an application where Search and Multitenancy already worked together. The former index stays on disk but is neither read nor written, so search over such entities returns nothing until a full reindex.
jmix.search.split-indexes-by-tenants=falsekeeps the former layout: the same index names, no splitting, and none of the new definition checks apply. The property defaults totrue.Breaking changes
Seven public types changed without a compatibility bridge. Fifteen methods differ only in return type, and Java will not hold two such signatures in one interface, so a
defaultbridge is impossible here rather than merely undesirable. Collapsing the list back into a scalar would lie: a split entity has many indexes, and one flag over fifty of them either reports a false success or reports a failure without naming the index.IndexManager— the four single-index methods are gone; the rest returnList<IndexOperationResult<…>>instead ofMap<IndexConfiguration, …>and take a tenant.IndexingQueueManager— the four session operations return a list instead ofboolean/void. A call that ignored the result still compiles.IndexConfiguration—getIndexName()removed; askIndexLayout.indexName(cfg, tenantId).EntityIndexer—deleteCollectionByTargetsadded without adefault.IndexSearchRequestScope— the record gained anindexNamecomponent.SearchProperties,IndexConfigurationManager— constructor parameters appended.Behaviour changes: records of a tenant-aware entity with an empty tenant are no longer indexed; three new checks reject an index definition instead of accepting it (the application still starts, only that entity drops out); JMX operations print a
Tenantcolumn.One break is silent. The indexing queue used to call
EntityIndexer.deleteCollectionByEntityIdsand now callsdeleteCollectionByTargets. An application that subclassed an engine indexer and overrode the former still compiles, and its override no longer runs for queued deletions. The old method is deprecated so that the compiler warns on that@Override, which is the only signal that reaches such an application on its own.Five deprecations, all
since = "3.1", forRemoval = true:SearchProperties.searchIndexNamePrefixand its getter,EntityIndexer.deleteCollectionByEntityIds,IndexConfigurationManager.getIndexConfigurationByIndexNameand itsOptform. All of them keep working.Limitations
@TenantIdsits outside JPA is not split. The add-on fills the attribute fromJpaDataStorealone, so such an entity has no tenants to be split by.Checked
:search,:search-elasticsearch,:search-opensearch,:search-starter. A test application covers both engines end to end, including tenant isolation in a live index, the no-add-on path, dynamic attributes, a contributed definition, and the compatibility mode: 31 of 31 on OpenSearch, 30 of 31 on Elasticsearch (the one failure is a race inside the scenario itself, which passes when run alone).