[jira] [Comment Edited] (OAK-3201) Use static references in SecurityProviderImpl for composite services

["Francesco Mari (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-3201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14734718#comment-14734718 ] 

Francesco Mari edited comment on OAK-3201 at 9/8/15 12:24 PM:
--------------------------------------------------------------

[~chetanm], I'm aware of the change of behaviour. I decided to fix the issue this way because I didn't find contexts where {{SecurityProviderImpl}} uses multiple implementations of its dependencies. 

Personally, I think that {{cardinality.minimum}} is a workaround for a use case that was probably not well defined. {{cardinality.minimum}} would prevent {{SecurityProviderImpl}} to start unless the minimum amount of implementations for a reference are found. But who defines what is the minimum amount of implementations to expect? In example, just because Oak provides one implementation of {{AuthorizableActionProvider}} that we use as a default, we can't arbitrarily use a {{cardinality.minimum=1}} for the reference to {{AuthorizableActionProvider}}. What if a user writes a custom implementation of {{AuthorizableActionProvider}} that is supposed to live along our default implementation? What if this user wants {{SecurityProviderImpl}} to start only if the two implementations of {{AuthorizableActionProvider}} are available?

Sure, we can define our own mechanism to substitute {{cardinality.minimum}}. But I think that a different approach would be cleaner:

- Create a new implementation of {{AuthorizableActionProvider}}.
- If multiple implementation of {{AuthorizableActionProvider}} should be wired together, use {{CompositeAuthorizableActionProvider}} as a base class for the implementation.
- Register the new implementation.
- Disable the default implementation or give the new implementation a higher service ranking (restart bundles as needed).

With this approach, we don't have to hardcode a {{cardinality.minimum}} in {{SecurityManagerImpl}}, or implement a similar mechanism on our own.

This solution would also allowed us to switch to a dynamic approach, like we currently have. We could create an implementation like the following:

{code}
public class DynamicAuthorizableActionProvider implements AuthorizableActionProvider {
  public List<? extends AuthorizableAction> getAuthorizableActions(@Nonnull SecurityProvider securityProvider) {
    return getComposite(lookupImplementationsOtherThanMe()).getAuthorizableActions(securityProvider)
  }
}
{code}

But at least, it would be clear that, by using this dynamic solution, you could have a potentially half-iniitialized repository.


was (Author: frm):
[~chetanm], I'm aware of the change of behaviour. I decided to fix the issue this way because I didn't find contexts where {{SecurityProviderImpl}} uses multiple implementations of its dependencies. 

Personally, I think that {{cardinality.minimum}} is a workaround for a use case that was probably not well defined. {{cardinality.minimum}} would prevent {{SecurityProviderImpl}} to start unless the minimum amount of implementations for a reference are found. But who defines what is the minimum amount of implementations to expect? In example, just because Oak provides one implementation of {{AuthorizableActionProvider}} that we use as a default, we can't arbitrarily use a {{cardinality.minimum=1}} for the reference to {{AuthorizableActionProvider}}. What if a user writes a custom implementation of {{AuthorizableActionProvider}} that is supposed to live along our default implementation? What if this user wants {{SecurityProviderImpl}} to start only if the two implementations of {{AuthorizableActionProvider}} are available?

Sure, we can define our own mechanism to substitute {{cardinality.minimum}}. But I think that a different approach would be cleaner:
- Create a new implementation of {{AuthorizableActionProvider}}.
- If multiple implementation of {{AuthorizableActionProvider}} should be wired together, use {{CompositeAuthorizableActionProvider}} as a base class for the implementation.
- Register the new implementation.
- Disable the default implementation or give the new implementation a higher service ranking (restart bundles as needed).
With this approach, we don't have to hardcode a {{cardinality.minimum}} in {{SecurityManagerImpl}}, or implement a similar mechanism on our own.

This solution would also allowed us to switch to a dynamic approach, like we currently have. We could create an implementation like the following:

{code}
public class DynamicAuthorizableActionProvider implements AuthorizableActionProvider {
  public List<? extends AuthorizableAction> getAuthorizableActions(@Nonnull SecurityProvider securityProvider) {
    return getComposite(lookupImplementationsOtherThanMe()).getAuthorizableActions(securityProvider)
  }
}
{code}

But at least, it would be clear that, by using this dynamic solution, you could have a potentially half-iniitialized repository.

> 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.6
>
>         Attachments: OAK-3201-01.patch, OAK-3201-02.patch, OAK-3201-03.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)