[
https://issues.apache.org/jira/browse/OAK-3201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14791710#comment-14791710
]
Francesco Mari commented on OAK-3201:
-------------------------------------
bq. Setting immediate to true - That should not be required per se as DS can
figure out that there is a requirement of services of given type and those
components would be then activated. So some component would have dependency on
SecurityProvider which in turn expresses its dependency on various providers
and thus DS would initialize them. So you can omit that change.
I will revert those changes before committing (or if I post another patch for
review, of course).
bq. SecurityProviderRegistrationTest - Various stub class can possibly be
removed with Mocks.
I hoped to use Groovy's MockFor, but it doesn't work. I will user Mockito
instead.
bq. Preconditions - areSatisfied - You can also use Guava Sets.difference there
I will use Guava for that, but I will not propose a new patch for review
because of this change.
bq. createCompositePrincipalConfiguration, createCompositeTokenConfiguration -
Given that by the time SecurityProvider is being constructed all the
dependencies are met you can just as well populate the actual composites.
This may work the first time, but it doesn't work if another, non-required
PrincipalConfiguration or TokenConfiguration is subsequently bound to
SecurityProviderRegistration. Moreover, an unregistration of the
SecurityProvider followed by a registration creates a new instance of
SecurityProvider that has to be bound to the configurations (again).
bq. createCompositeRestrictionProvider,
createCompositeAuthorizableActionProvider - Here also we just well initialize
the composites with required instances. This would prevent unnecessary garbage
creation of a new list for every invocation.
Similar problem of the point above. New, non-required instances of
RestrictionProvider or AuthorizableActionProvider can be bound at any time
after the registration of the SecurityProvider.
bq. getRequiredServicePids - It would be better to also pass a default value
there and not rely on metatype providing default values.
I already provide a default value, which is the empty array. If you think that
the default value should be the PIDs of services that I think should be
required, I don't feel comfortable with that. If the list of required PIDs
change over time, I don't want to recompile oak-core because the default value
was hardcoded in SecurityProviderRegistration.
> Use static references in SecurityProviderImpl for composite services
> --------------------------------------------------------------------
>
> Key: OAK-3201
> URL: https://issues.apache.org/jira/browse/OAK-3201
> Project: Jackrabbit Oak
> Issue Type: Bug
> Components: core
> Reporter: Francesco Mari
> Assignee: Francesco Mari
> Fix For: 1.3.7
>
> Attachments: OAK-3201-01.patch, OAK-3201-02.patch, OAK-3201-03.patch,
> OAK-3201-04.patch, OAK-3201-05.patch, OAK-3201-06.patch, OAK-3201-07.patch,
> OAK-3201-08.patch, mbean-test.log
>
>
> {{SecurityProviderImpl}} has dynamic references to many other services, like
> {{RestrictionProvider}}, that represent the configuration of this component.
> Being these services dynamic, the OSGi runtime has no clear dependency
> relationship between the {{SecurityProviderImpl}} and the required services.
> Thus, it may happen that an instance of {{SecurityProviderImpl}} is published
> before the services it requires are started, creating a window where the
> {{SecurityProviderimpl}} is operating differently from the way it's
> configured.
> I suggest to turn the dynamic references in {{SecurityProviderImpl}} to
> static ones to improve the consistency of the implementation.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
