tjwatson commented on a change in pull request #19:
URL: https://github.com/apache/felix-dev/pull/19#discussion_r413000181
##########
File path:
scr/src/main/java/org/apache/felix/scr/impl/BundleComponentActivator.java
##########
@@ -98,132 +98,83 @@
private static class ListenerInfo implements ServiceListener
{
- private Map<Filter,
List<ExtendedServiceListener<ExtendedServiceEvent>>> filterMap = new
HashMap<>();
+ List<ExtendedServiceListener<ExtendedServiceEvent>> listeners = new
ArrayList<>();
@Override
public void serviceChanged(ServiceEvent event)
{
ServiceReference<?> ref = event.getServiceReference();
Review comment:
The `ref` variable is no longer used.
##########
File path:
scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
##########
@@ -2239,52 +2238,16 @@ else if ( m_dependencyMetadata.getScope() ==
ReferenceScope.prototype_required )
m_componentManager.getLogger().log(LogService.LOG_DEBUG, "Setting
target property for dependency {0} to {1}",
null, getName(), target );
BundleContext bundleContext = m_componentManager.getBundleContext();
Review comment:
There is an additional block above that is no longer needed also:
```
// eventFilter
String eventFilterString;
if (m_target != null
&& m_dependencyMetadata.getScope() ==
ReferenceScope.prototype_required)
{
final StringBuilder sb = new
StringBuilder("(&").append(PROTOTYPE_SCOPE_CLAUSE).append(m_target).append(")");
eventFilterString = sb.toString();
}
else if ( m_dependencyMetadata.getScope() ==
ReferenceScope.prototype_required )
{
eventFilterString = PROTOTYPE_SCOPE_CLAUSE;
}
else
{
eventFilterString = m_target;
}
```
##########
File path: scr/src/test/resources/integration_test_FELIX_6161.xml
##########
@@ -0,0 +1,64 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+<components>
+ <scr:component name="felix.6161"
+ xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0";
+ immediate="true"
+ enabled="false"
+ configuration-policy="optional"
+ activate="activate"
+ modified="modified">
+ <implementation
class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ <service>
+ <provide
interface="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </service>
+ <reference
+ name="one"
+
interface="org.apache.felix.scr.integration.components.SimpleService"
+ cardinality="0..1"
+ policy="dynamic"
+ bind="setSimpleService"
+ unbind="unsetSimpleService"
+ target="(value=foo)"
+ />
+ </scr:component>
+
+ <scr:component name="felix.6161.2nd"
+ xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0";
+ immediate="true"
+ enabled="false"
+ configuration-policy="optional"
+ activate="activate"
+ modified="modified">
+ <implementation
class="org.apache.felix.scr.integration.components.SimpleComponent" />
+ <service>
+ <provide
interface="org.apache.felix.scr.integration.components.SimpleComponent" />
+ </service>
+ <reference
+ name="one"
+
interface="org.apache.felix.scr.integration.components.SimpleService"
+ cardinality="0..1"
+ policy="dynamic"
+ bind="setSimpleService"
+ unbind="unsetSimpleService"
+ target="(value=foo)"
+ />
+ </scr:component>
Review comment:
I suggest a third component that uses a different target filter from the
other two. And a forth that has an invalid filter for target.
##########
File path:
scr/src/main/java/org/apache/felix/scr/impl/manager/DependencyManager.java
##########
@@ -2239,52 +2238,16 @@ else if ( m_dependencyMetadata.getScope() ==
ReferenceScope.prototype_required )
m_componentManager.getLogger().log(LogService.LOG_DEBUG, "Setting
target property for dependency {0} to {1}",
null, getName(), target );
BundleContext bundleContext = m_componentManager.getBundleContext();
- Filter eventFilter = null;
Review comment:
I tried to find if there is a specified behavior in the DS spec if the
target has an invalid filter. I could not find anything. Regardless, the
current Felix SCR impl seems to log an error here and then create this target
`(component.id=-1)` filter to be used instead.
I suggest we still need this check with something like this:
```
if (target != null)
{
try
{
FrameworkUtil.createFilter(target);
}
catch (InvalidSyntaxException e)
{
m_componentManager.getLogger().log(LogService.LOG_ERROR,
"Invalid syntax in target property for dependency {0} to
{1}", null,
getName(), target);
//create a filter that will never be satisfied
target = "(component.id=-1)";
}
}
m_target = target;
```
Where we do the check on the incoming `target` param before setting it to
the instance `m_target`
----------------------------------------------------------------
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:
[email protected]
