On Wed, Sep 10, 2014 at 6:49 AM, Nipuni Perera <[email protected]> wrote:
> Hi,
>
> Yes. I can add a null check to indexingConfig element and continue only if
> the value is available (But it will add the whole method execution inside
> constructor RegistryConfigLoader() to an if block - line 75 to 150 of [1]).
> Is this preferable?
>
>
That's a readability issue! :) I'm not sure about the code, but it sounds
like you could move this whole block to a separate method.
> public RegistryConfigLoader() {
> try {
> FileInputStream fileInputStream = new
> FileInputStream(getConfigFile());
> StAXOMBuilder builder = new StAXOMBuilder(
>
> CarbonUtils.replaceSystemVariablesInXml(fileInputStream));
> OMElement configElement = builder.getDocumentElement();
> OMElement indexingConfig = configElement.getFirstChildWithName(
> new QName("indexingConfiguration"));
> if (indexingConfig != null)
> { //newly
> added if block
> ...
> ...
> }
> }
> } catch (FileNotFoundException e) {
> // This virtually cannot happen as registry.xml is necessary
> to start up the registry
> log.error("registry.xml has not been found", e);
> } catch (RegistryException e) {
> log.error(e.getMessage(),e);
> } catch (XMLStreamException e) {
> String msg = "error building registry.xml, check for badly
> formed xml";
> log.error(msg, e);
> } catch (CarbonException e) {
> log.error("An error occurred during system variable
> replacement", e);
> }
> }
>
> It seems if the indexingConfig is null, no need to call this constructor.
> RegistryConfigLoader() is called inside IndexingManager() constructor.
>
> private IndexingManager() {
> try {
> registry =
> Utils.getRegistryService().getRegistry(CarbonConstants.REGISTRY_SYSTEM_USERNAME);
> registryConfig = new RegistryConfigLoader();
> // call to constructor RegistryConfigLoader()
> indexer = new AsyncIndexer();
> } catch (RegistryException e) {
> log.error("Could not initialize registry for indexing", e);
> }
> }
> Can't we check indexingConfig value before calling RegistryConfigLoader()
> ?
>
> [1]
> https://github.com/Nipuni/carbon-registry/blob/master/components/registry/org.wso2.carbon.registry.indexing/src/main/java/org/wso2/carbon/registry/indexing/RegistryConfigLoader.java
>
> Thanks,
> Nipuni
>
>
> On Wed, Sep 10, 2014 at 8:53 AM, Kasun Gajasinghe <[email protected]> wrote:
>
>>
>> As a best practice, do not catch the Exception class, instead catch
>> specific subclasses. But, you do not need to catch any exceptions in this
>> particular case? Can't you just verify whether the indexingConfiguration
>> element is available or not?
>>
>> KasunG
>>
>> On Wed, Sep 10, 2014 at 4:39 AM, Nipuni Perera <[email protected]> wrote:
>>
>>> Hi,
>>>
>>> I am working on issue[1]. I could find that the RegistryConfigLoader()
>>> returns a null value. (line 73 in [2]).
>>>
>>> OMElement indexingConfig = configElement.getFirstChildWithName(new
>>> QName("indexingConfiguration"));
>>>
>>> This value is used in several places. but NPE is thrown in line 88,
>>>
>>> lastAccessTimeLocation = indexingConfig.getFirstChildWithName(new
>>> QName("lastAccessTimeLocation")).getText();
>>>
>>> as the catch clause catches only
>>>
>>> catch (OMException e) {
>>> ... // set default value
>>> }
>>>
>>> while in other places (eg: line 78,79) it is
>>>
>>> catch (Exception e) {
>>> ... // set default value
>>> }
>>>
>>> This exception occurred due to a missing XML elements in registry.xml.
>>> This missing element ( <indexingConfiguration> </indexingConfiguration> )
>>> will be used if the product is using registry search feature. But this NPE
>>> should be handled for the products that does not use this feature. This can
>>> be fixed adding similar catch clause (catching Exception e ) in the
>>> catch clause(line 90,91) (current implementation use catch clauses to set
>>> default values if an exception occurs. Adding an extra catch clause to the
>>> later try block can be used to add similar behavior). Is there a better
>>> way to handle this?
>>>
>>> [1] https://wso2.org/jira/browse/CARBON-14904
>>> [2]
>>> https://githubc.com/Nipuni/carbon-registry/blob/master/components/registry/org.wso2.carbon.registry.indexing/src/main/java/org/wso2/carbon/registry/indexing/RegistryConfigLoader.java
>>> <https://github.com/Nipuni/carbon-registry/blob/master/components/registry/org.wso2.carbon.registry.indexing/src/main/java/org/wso2/carbon/registry/indexing/RegistryConfigLoader.java>
>>>
>>> Thanks,
>>> Nipuni
>>> --
>>> Nipuni Perera
>>> Software Engineer; WSO2 Inc.; http://wso2.com
>>> Email: [email protected]
>>> Git hub profile: https://github.com/nipuni
>>> Mobile: +94 (71) 5626680
>>> <http://wso2.com>
>>>
>>>
>>> _______________________________________________
>>> Dev mailing list
>>> [email protected]
>>> http://wso2.org/cgi-bin/mailman/listinfo/dev
>>>
>>>
>>
>>
>> --
>>
>> *Kasun Gajasinghe*Senior Software Engineer, WSO2 Inc.
>> email: kasung AT spamfree wso2.com
>> linked-in: http://lk.linkedin.com/in/gajasinghe
>> blog: http://kasunbg.org
>>
>>
>>
>
>
>
> --
> Nipuni Perera
> Software Engineer; WSO2 Inc.; http://wso2.com
> Email: [email protected]
> Git hub profile: https://github.com/nipuni
> Mobile: +94 (71) 5626680
> <http://wso2.com>
>
>
--
*Kasun Gajasinghe*Senior Software Engineer, WSO2 Inc.
email: kasung AT spamfree wso2.com
linked-in: http://lk.linkedin.com/in/gajasinghe
blog: http://kasunbg.org
_______________________________________________
Dev mailing list
[email protected]
http://wso2.org/cgi-bin/mailman/listinfo/dev
