tripodsan commented on a change in pull request #80:
URL:
https://github.com/apache/jackrabbit-filevault/pull/80#discussion_r430102948
##########
File path:
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageManagerImpl.java
##########
@@ -88,8 +88,8 @@
* @param session repository session
* @param roots the root paths to store the packages.
*/
- public JcrPackageManagerImpl(Session session, String[] roots) {
- this(new JcrPackageRegistry(session, roots));
+ public JcrPackageManagerImpl(@NotNull Session session, @Nullable String[]
roots, @Nullable String[] additionalAuthorizableIdsAllowedToExecuteHooks,
@Nullable String[]
additionalAuthorizableIdsAllowedToInstallPackagesRequiringRoot) {
+ this(new JcrPackageRegistry(session,
additionalAuthorizableIdsAllowedToExecuteHooks,
additionalAuthorizableIdsAllowedToInstallPackagesRequiringRoot, roots));
Review comment:
I don't like this in particular:
- what about creating a `JcrPackageRegistryConfig` that holds the `roots`,
`authIdsExec` and `authIdsRootInstall` value?
- or at least creating a `JcrPackageRegistrySecurity` config that holds the
`authIdsExec` and `authIdsRootInstall` values?
in any case: also add default constructors that implement the old api.
##########
File path:
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/AbstractPackageRegistry.java
##########
@@ -59,6 +65,17 @@
*/
public static final String DEFAULT_PACKAGE_ROOT_PATH_PREFIX =
DEFAULT_PACKAGE_ROOT_PATH + "/";
+ protected final String[] additionalAuthorizableIdsAllowedToExecuteHooks;
+
+ protected final String[]
additionalAuthorizableIdsAllowedToInstallPackagesRequiringRoot;
Review comment:
I think those a very long field names :-)
##########
File path:
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageManagerImpl.java
##########
@@ -88,8 +88,8 @@
* @param session repository session
* @param roots the root paths to store the packages.
*/
- public JcrPackageManagerImpl(Session session, String[] roots) {
- this(new JcrPackageRegistry(session, roots));
+ public JcrPackageManagerImpl(@NotNull Session session, @Nullable String[]
roots, @Nullable String[] additionalAuthorizableIdsAllowedToExecuteHooks,
@Nullable String[]
additionalAuthorizableIdsAllowedToInstallPackagesRequiringRoot) {
+ this(new JcrPackageRegistry(session,
additionalAuthorizableIdsAllowedToExecuteHooks,
additionalAuthorizableIdsAllowedToInstallPackagesRequiringRoot, roots));
Review comment:
it's not about backward compatibility, but to write `...null, null)`
alot is kind of disturbing.
##########
File path:
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/AbstractPackageRegistry.java
##########
@@ -59,6 +83,18 @@
*/
public static final String DEFAULT_PACKAGE_ROOT_PATH_PREFIX =
DEFAULT_PACKAGE_ROOT_PATH + "/";
+ protected final @NotNull SecurityConfig securityConfig;
+
+ public AbstractPackageRegistry(SecurityConfig securityConfig) {
+ super();
Review comment:
not needed.
##########
File path:
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/AbstractPackageRegistry.java
##########
@@ -44,6 +50,24 @@
*/
public abstract class AbstractPackageRegistry implements PackageRegistry,
InternalPackageRegistry {
+ public static final class SecurityConfig {
+ private final String[] authIdsForHookExecution;
+ private final String[] authIdsForRootInstallation;
+
+ public SecurityConfig(String[] authIdsForHooks, String[]
authIdsForRoots) {
+ super();
Review comment:
not needed.
##########
File path:
vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/registry/impl/FSPackageRegistry.java
##########
@@ -138,15 +138,31 @@ public FSPackageRegistry(@NotNull File homeDir) throws
IOException {
* @throws IOException If an I/O error occurs.
*/
public FSPackageRegistry(@NotNull File homeDir, InstallationScope scope)
throws IOException {
+ this(homeDir, scope, null);
+ }
+
+ public FSPackageRegistry(@NotNull File homeDir, InstallationScope scope,
@Nullable AbstractPackageRegistry.SecurityConfig securityConfig) throws
IOException {
+ super(securityConfig);
this.homeDir = homeDir;
+ log.info("Jackrabbit Filevault FS Package Registry initialized with
home location {}", this.homeDir.getPath());
this.scope = scope;
loadPackageCache();
}
-
/**
* Deafult constructor for OSGi initialization (homeDir defined via
activator)
+ * @throws IOException
*/
- public FSPackageRegistry() {
+ @Activate
Review comment:
since when can you use a constructor as activator?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
