-
Notifications
You must be signed in to change notification settings - Fork 3
Sed 3921 introduced settings with multitenancy capabilities #433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Sed 3921 introduced settings with multitenancy capabilities #433
Conversation
...ase-plugins/src/main/java/step/plugins/parametermanager/EncryptedEntityExportBiConsumer.java
Outdated
Show resolved
Hide resolved
...ase-plugins/src/main/java/step/plugins/parametermanager/EncryptedEntityImportBiConsumer.java
Outdated
Show resolved
Hide resolved
...ase-plugins/src/main/java/step/plugins/parametermanager/EncryptedEntityExportBiConsumer.java
Outdated
Show resolved
Hide resolved
...ase-plugins/src/main/java/step/plugins/parametermanager/EncryptedEntityExportBiConsumer.java
Outdated
Show resolved
Hide resolved
...ase-plugins/src/main/java/step/plugins/parametermanager/EncryptedEntityImportBiConsumer.java
Outdated
Show resolved
Hide resolved
step-core-model/src/main/java/step/usersettings/UserSetting.java
Outdated
Show resolved
Hide resolved
step-core/src/main/java/step/parameter/AbstractEncryptedValuesManager.java
Outdated
Show resolved
Hide resolved
step-core/src/main/java/step/parameter/AbstractEncryptedValuesManager.java
Outdated
Show resolved
Hide resolved
step-core/src/main/java/step/parameter/AbstractEncryptedValuesManager.java
Outdated
Show resolved
Hide resolved
step-core/src/main/java/step/usersettings/UserSettingAccessorImpl.java
Outdated
Show resolved
Hide resolved
…-Settings-with-multitenancy-capabilities
…-Settings-with-multitenancy-capabilities
…-Settings-with-multitenancy-capabilities # Conflicts: # pom.xml
david-stephan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iegorov777 please check the individual comments
...tion-packages-junit5/src/test/java/step/junit5/runner/StepRunnerWithPlansAnnotationTest.java
Show resolved
Hide resolved
| // schedules will be deleted in deleteAdditionalData via hooks | ||
| deleteResources(automationPackage); | ||
| deleteAdditionalData(automationPackage, new AutomationPackageContext(operationMode, resourceManager, null, null,null, extensions)); | ||
| // TODO: archive, packageContent, enricher and validator are only used in AutomationPackageContext for save/update - maybe it is better to create separate class AutomationPackageContextForSave rather then use nulls here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be clarified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can discuss this. I'm not very happy with this constructor for AutomationPackageContext, where pass many parameters and many of them are null in the most of cases
| try (FileInputStream fis = new FileInputStream(apFile.getFile())) { | ||
| // the apVersion is null (we always use the actual version), because we only create the isolated in-memory AP here | ||
| inMemoryPackageManager.createAutomationPackage(fis, apFile.getFile().getName(), null, null, enricher, predicate); | ||
| inMemoryPackageManager.createAutomationPackage(fis, apFile.getFile().getName(), null, null, enricher, predicate, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might not be possible, but since predicate enricher and validator all comes from the objectHookRegistry maybe we could just pass it here somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-stephan to be clarified
| */ | ||
| public ObjectId createAutomationPackage(InputStream packageStream, String fileName, String apVersion, String activationExpr, ObjectEnricher enricher, ObjectPredicate objectPredicate) throws AutomationPackageManagerException { | ||
| return createOrUpdateAutomationPackage(false, true, null, packageStream, fileName, apVersion, activationExpr, enricher, objectPredicate, false).getId(); | ||
| public ObjectId createAutomationPackage(InputStream packageStream, String fileName, String apVersion, String activationExpr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might not be possible, but since predicate enricher and validator all comes from the objectHookRegistry maybe we could just pass it here somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-stephan for now I keep separate parameters for enricher, predicate and validator
step-core-model/src/main/java/step/projectsettings/ProjectSetting.java
Outdated
Show resolved
Hide resolved
step-core-model/src/main/java/step/multitenancy/TenantUniqueEntity.java
Outdated
Show resolved
Hide resolved
step-core-model/src/main/java/step/multitenancy/TenantUniqueEntity.java
Outdated
Show resolved
Hide resolved
step-core/src/main/java/step/projectsettings/ProjectSettingManager.java
Outdated
Show resolved
Hide resolved
step-core/src/main/java/step/projectsettings/ProjectSettingManager.java
Outdated
Show resolved
Hide resolved
…-Settings-with-multitenancy-capabilities # Conflicts: # pom.xml # step-automation-packages/step-automation-packages-junit5/src/test/java/step/junit5/runner/StepRunnerWithPlansAnnotationTest.java # step-automation-packages/step-automation-packages-manager/src/main/java/step/automation/packages/AutomationPackageManager.java # step-automation-packages/step-automation-packages-manager/src/main/java/step/automation/packages/execution/RepositoryWithAutomationPackageSupport.java
...-base-plugins/src/main/java/step/plugins/projectsettings/ProjectSettingControllerPlugin.java
Outdated
Show resolved
Hide resolved
...-base-plugins/src/main/java/step/plugins/projectsettings/ProjectSettingControllerPlugin.java
Outdated
Show resolved
Hide resolved
step-core/src/main/java/step/encryption/AbstractEncryptedValuesManager.java
Outdated
Show resolved
Hide resolved
step-core/src/main/java/step/core/settings/AbstractSettingAccessorWithHook.java
Outdated
Show resolved
Hide resolved
| super((Collection<T>) collectionDriver); | ||
| } | ||
|
|
||
| public Optional<T> findDuplicate(EntityWithUniqueAttributes e){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the trick to make this properly work is to use getObjectFilter to get the filters in context, as commented on EE PR we should not need any custom / specific validator in EE. Also this should not be specific to project settings and should be reusable for any entity for which we want to ensure uniqueness based on the set of attributes.
So again in EE getObjectFilter will automatically return the OR filters with the current project id + all other global projects which is used to retrieve all entities accessible in the current session context. So In this case any settings with the same set of attributes will be a duplicate, no need of the implementation in multitenancy manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-stephan
Sorry, maybe I misunderstand how the QLFilters work, but it seems like current (old) ObjectFilter in MultitenancyManager:
- doesn't filter entities in case user works from global context:
protected String getOQLFilterForTenant(TenantContext tenantContext) {
if(tenantContext == null || tenantContext.getTenant().isGlobal()) {
return "";
}
- for non-global context, as you say, it returns all entities linked with global project OR with current project
So in both cases after this filter we can have entities (project settings) linked with several global projects. Taking into account that the project is also an attribute it seems like without special validator in EE we cannot cover the situation, when user tries to save a project setting already linked with ANOTHER global project.
The common check by uniqueness of all attributes in this case won't work, because project attribute will be different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iegorov777 I get the confusion as we use "global" wording for both the tenant and the projects, but it is not the same thing. There is only one tenant which is global (tenantContext.getTenant().isGlobal())), this is the [All] tenant that you can select in Step EE, in that case there is no filters and you see data for all projects together. When you are in a non global tenant, you always see data from all global projects (that is the or clause built in the else of getOQLFilterForTenant) + data from the current project (global project or not).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-stephan understood, thank you.
Now I've changed the implementation and in OS we have a common validator, which can check the uniqueness by ALL attriubtes, and also it can be externally configured (in Step EE) to validate attirubes exluding projectId and taking priority attribute into account.
Unfortunatelly we cannot just remove the project attribute from this validation, because in this case for LOCAL products we will have the identical set of attributes (priority will be the same), but we should allow create the same setting for several local products. So the solution is a little more tricky than I expected.
…-Settings-with-multitenancy-capabilities # Conflicts: # pom.xml
…-Settings-with-multitenancy-capabilities # Conflicts: # pom.xml
…th-multitenancy-capabilities # Conflicts: # pom.xml # step-automation-packages/step-automation-packages-manager/src/main/java/step/automation/packages/AutomationPackageManager.java
No description provided.