felix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Felix Meschberger <fmesc...@gmail.com>
Subject Re: [jira] Issue Comment Edited: (FELIX-639) Need more logs from SCR
Date Wed, 06 Aug 2008 11:18:10 GMT
Hi,

Pierre De Rop schrieb:
> Hi Felix;
> 
> Sorry for my wrong suggestion about raising exceptions on invalid 
> scr.xml descriptor.

Don't worry ;-)

> But, as you suggested, some logs (warn/info, or whatever level) would be 
> just fine and helpful.

Ok. So will add some checks and logs.

Regards
Felix

> 
> /Pierre
> 
> Felix Meschberger (JIRA) wrote:
>>     [ 
>> https://issues.apache.org/jira/browse/FELIX-639?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12616036#action_12616036

>> ]
>> fmeschbe edited comment on FELIX-639 at 8/6/08 2:31 AM:
>> -----------------------------------------------------------------
>>
>> Thanks for submitting this issue. Please allow me to comment on this 
>> quickly:
>>
>> (1) "Unexpected Elements": Such elements are explicitly allowed as per 
>> the Declarative Services specification. So your example is perfectly 
>> valid, except that you have to use namespaces here (as Carsten 
>> Ziegeler already stated). The namespace may only be omitted in case of 
>> having a single <component> element in the XML file. So the 
>> ParseException in the XMLHandler is not appropriate as this would 
>> violate the spec.
>>
>> What I could imagine is, that we should add a check for this 
>> situation: if there is no namespace, only a single <component> element 
>> is allowed, else an ERROR is logged. Otherwise multiple <component> 
>> elements are allowed.
>>
>> (2) Duplicate Reference names: I could not find a note in the 
>> Declarative Services speicifcation which states that these names must 
>> be unique inside a component. The spec only states that they are local 
>> to the component and may be used for ComponentContext.locateService. 
>> So throwing an exception during validation in case of duplicate names 
>> is IMHO not appropriate because validation failure means the component 
>> is not going to be activated. Rather I would log a warning (at most, 
>> if not just an INFO) message and go on.
>>
>>       was (Author: fmeschbe):
>>     Thanks for submitting this issue. Please allow me to comment on 
>> this quickly:
>>
>> (1) "Unexpected Elements": Such elements are explicitly allowed as per 
>> the Declarative Services specification. So your example is perfectly 
>> valid, except that you have to use namespaces here (as Carsten 
>> Ziegeler already stated). The namespace may only be omitted in case of 
>> having a sling <component> element in the XML file. So The 
>> ParseException in the XMLHandler will certainly not be done as 
>> proposed as this would violate the spec.
>>
>> What I could imagine is, that we should add a check for this 
>> situation: if there is no namespace, only a single <component> element 
>> is allowed, else an ERROR is logged. Otherwise multiple <component> 
>> elements are allowed.
>>
>> (2) Duplicate Reference names: I could not find a note in the 
>> Declarative Services speicifcation which states that these names must 
>> be unique inside a component. The spec only states that they are local 
>> to the component and may be used for ComponentContext.locateService. 
>> So throwing an exception during validation in case of duplicate names 
>> is IMHO not appropriate because validation failure means the component 
>> is not activated. Rather I would log a warning (at most, if not just 
>> an INFO) message and go on.
>>    
>>> Need more logs from SCR
>>> -----------------------
>>>
>>>                 Key: FELIX-639
>>>                 URL: https://issues.apache.org/jira/browse/FELIX-639
>>>             Project: Felix
>>>          Issue Type: Improvement
>>>          Components: Declarative Services (SCR)
>>>    Affects Versions: scr-1.0.2
>>>         Environment: linux
>>>            Reporter: Pierre De Rop
>>>            Priority: Minor
>>>         Attachments: ComponentMetadata.java, XmlHandler.java
>>>
>>>
>>> As explained in the dev post 
>>> http://www.mail-archive.com/dev@felix.apache.org/msg05030.html,
>>> I would like the SCR to be improved in order to log some WARNINGs, 
>>> when a SCR.xml descriptor contains invalid elements, event if it is 
>>> well formed.
>>> for example, the following SCR.xml has two errors:
>>> <components>
>>>   <component name="Component1">
>>>     <implementation class="test.ds.hello.HelloComponent1"/>
>>>     <reference name="LOG" interface="org.osgi.service.log.LogService" 
>>>       bind="setLog"       unbind="unsetLog"
>>>       cardinality="1..n"
>>>       policy="dynamic"/>
>>>   </component>
>>>   <component name=""Component2">
>>>     <implementation class="test.ds.hello.HelloComponent2"/>
>>>     <reference name="LOG" interface="org.osgi.service.log.LogService" 
>>>       bind="setLog"       unbind="unsetLog"
>>>       cardinality="1..n"
>>>       policy="dynamic"/>
>>>         <reference name="LOG" 
>>> interface="org.osgi.service.log.LogService"       bind="setLog2" 
>>>       unbind="unsetLog2"
>>>       cardinality="1..n"
>>>       policy="dynamic"/>
>>>   </component>
>>> </components>
>>> -> the two components are embedded in an invalid <components> xml 
>>> root element
>>> -> the component "Component2" has two references with the SAME name
>>> I would like the SCR to log some WARNINGs message, when finding these 
>>> sort of errors:
>>> 1/ log a WARNING message when finding an unknown/invalid xml element
>>> 2/ log a WARNING message when duplicate reference names are detected 
>>> for one given component.
>>> Here is a tentative fix which is working for my use cases:
>>> 1/ in the  org.apache.felix.scr.impl.ComponentMetadata.validate() 
>>> method (around line 299):
>>>         // Check that the references are ok.
>>>     // We'll especially Check if there's not duplicate names in the 
>>> component references.
>>>     HashSet refs = new HashSet();
>>>         Iterator referenceIterator = m_references.iterator();
>>>         while ( referenceIterator.hasNext() )
>>>         {
>>>         ReferenceMetadata refMeta = ( ReferenceMetadata ) 
>>> referenceIterator.next();
>>>             refMeta.validate( this );
>>>         if (! refs.add(refMeta.getName())) {
>>>           throw validationFailure( "Detected duplicate reference 
>>> name: \"" + refMeta.getName() + "\"");
>>>         }
>>>         }
>>> 2/ in org.apache.felix.scr.impl.XmlHandler.startElement method 
>>> (around line 215, at the end of the method):
>>> +++ src/main/java/org/apache/felix/scr/impl/XmlHandler.java     
>>> (working copy)
>>> @@ -211,14 +211,21 @@
>>>                      ref.setUnbind( attrib.getProperty( "unbind" ) );
>>>                      m_currentComponent.addDependency( ref );
>>> -                }
>>> +                }
>>> +               else {
>>> +                   throw new ParseException("Invalid SCR xml element 
>>> found in " + m_bundle.getLocation() +
>>> +                                            ": \"" + localName + 
>>> "\"", null);
>>> +               }
>>>              }
>>>              catch ( Exception ex )
>>>              {
>>>                  ex.printStackTrace();
>>>                  throw new ParseException( "Exception during 
>>> parsing", ex );
>>>              }
>>> -        }
>>> +        } else {
>>> +         throw new ParseException("Invalid SCR xml element found in 
>>> " + m_bundle.getLocation() +
>>> +                                  ": \"" + localName + "\"", null);
>>> +       }
>>>      }
>>> -> Here is the complete/modifed method:
>>>     public void startElement( String uri, String localName, 
>>> Properties attrib ) throws ParseException
>>>     {
>>>         // according to the spec, the elements should have the 
>>> namespace,
>>>         // except when the root element is the "component" element
>>>         // So we check this for the first element, we receive.
>>>         if ( firstElement )
>>>         {
>>>             firstElement = false;
>>>             if ( localName.equals( "component" ) && "".equals( uri )
)
>>>             {
>>>                 overrideNamespace = NAMESPACE_URI;
>>>             }
>>>         }
>>>         if ( overrideNamespace != null && "".equals( uri ) )
>>>         {
>>>             uri = overrideNamespace;
>>>         }
>>>         if ( NAMESPACE_URI.equals( uri ) )
>>>         {
>>>             try
>>>             {
>>>                 // 112.4.3 Component Element
>>>                 if ( localName.equals( "component" ) )
>>>                 {
>>>                     // Create a new ComponentMetadata
>>>                     m_currentComponent = new ComponentMetadata();
>>>                     // name attribute is mandatory
>>>                     m_currentComponent.setName( attrib.getProperty( 
>>> "name" ) );
>>>                     // enabled attribute is optional
>>>                     if ( attrib.getProperty( "enabled" ) != null )
>>>                     {
>>>                         m_currentComponent.setEnabled( 
>>> attrib.getProperty( "enabled" ).equals( "true" ) );
>>>                     }
>>>                     // immediate attribute is optional
>>>                     if ( attrib.getProperty( "immediate" ) != null )
>>>                     {
>>>                         m_currentComponent.setImmediate( 
>>> attrib.getProperty( "immediate" ).equals( "true" ) );
>>>                     }
>>>                     // factory attribute is optional
>>>                     if ( attrib.getProperty( "factory" ) != null )
>>>                     {
>>>                         m_currentComponent.setFactoryIdentifier( 
>>> attrib.getProperty( "factory" ) );
>>>                     }
>>>                     // Add this component to the list
>>>                     m_components.add( m_currentComponent );
>>>                 }
>>>                 // 112.4.4 Implementation
>>>                 else if ( localName.equals( "implementation" ) )
>>>                 {
>>>                     // Set the implementation class name (mandatory)
>>>                     m_currentComponent.setImplementationClassName( 
>>> attrib.getProperty( "class" ) );
>>>                 }
>>>                 // 112.4.5 [...] Property Elements
>>>                 else if ( localName.equals( "property" ) )
>>>                 {
>>>                     PropertyMetadata prop = new PropertyMetadata();
>>>                     // name attribute is mandatory
>>>                     prop.setName( attrib.getProperty( "name" ) );
>>>                     // type attribute is optional
>>>                     if ( attrib.getProperty( "type" ) != null )
>>>                     {
>>>                         prop.setType( attrib.getProperty( "type" ) );
>>>                     }
>>>                     // 112.4.5: If the value attribute is specified, 
>>> the body of the element is ignored.
>>>                     if ( attrib.getProperty( "value" ) != null )
>>>                     {
>>>                         prop.setValue( attrib.getProperty( "value" ) );
>>>                         m_currentComponent.addProperty( prop );
>>>                     }
>>>                     else
>>>                     {
>>>                         // hold the metadata pending
>>>                         m_pendingProperty = prop;
>>>                     }
>>>                 }
>>>                 // 112.4.5 Properties [...] Elements
>>>                 else if ( localName.equals( "properties" ) )
>>>                 {
>>>                     readPropertiesEntry( attrib.getProperty( "entry" 
>>> ) );
>>>                 }
>>>                 // 112.4.6 Service Element
>>>                 else if ( localName.equals( "service" ) )
>>>                 {
>>>                     m_currentService = new ServiceMetadata();
>>>                     // servicefactory attribute is optional
>>>                     if ( attrib.getProperty( "servicefactory" ) != 
>>> null )
>>>                     {
>>>                         m_currentService.setServiceFactory( 
>>> attrib.getProperty( "servicefactory" ).equals( "true" ) );
>>>                     }
>>>                     m_currentComponent.setService( m_currentService );
>>>                 }
>>>                 else if ( localName.equals( "provide" ) )
>>>                 {
>>>                     m_currentService.addProvide( attrib.getProperty( 
>>> "interface" ) );
>>>                 }
>>>                 // 112.4.7 Reference element
>>>                 else if ( localName.equals( "reference" ) )
>>>                 {
>>>                     ReferenceMetadata ref = new ReferenceMetadata();
>>>                     ref.setName( attrib.getProperty( "name" ) );
>>>                     ref.setInterface( attrib.getProperty( "interface" 
>>> ) );
>>>                     // Cardinality
>>>                     if ( attrib.getProperty( "cardinality" ) != null )
>>>                     {
>>>                         ref.setCardinality( attrib.getProperty( 
>>> "cardinality" ) );
>>>                     }
>>>                     if ( attrib.getProperty( "policy" ) != null )
>>>                     {
>>>                         ref.setPolicy( attrib.getProperty( "policy" ) );
>>>                     }
>>>                     //if
>>>                     ref.setTarget( attrib.getProperty( "target" ) );
>>>                     ref.setBind( attrib.getProperty( "bind" ) );
>>>                     ref.setUnbind( attrib.getProperty( "unbind" ) );
>>>                     m_currentComponent.addDependency( ref );
>>>                 }         else {
>>>             throw new ParseException("Invalid SCR xml element found 
>>> in " + m_bundle.getLocation() +                          ": \"" + 
>>> localName + "\"", null);
>>>         }
>>>             }
>>>             catch ( Exception ex )
>>>             {
>>>                 ex.printStackTrace();
>>>                 throw new ParseException( "Exception during parsing", 
>>> ex );
>>>             }
>>>         } else {
>>>       throw new ParseException("Invalid SCR xml element found in " + 
>>> m_bundle.getLocation() +                    ": \"" + localName + 
>>> "\"", null);
>>>     }
>>>     }
>>>     
>>
>>   
> 
> 

Mime
View raw message