[jsword-devel] NPE in RawFileBackend#getCardinality

[DM Smith]

Most of the locations specified in a conf are to a directory.

For some it is to the prefix of the name of a data file.

So there might be modules/comments/rawcom/asdf/asdf for an asdf module.
But that is not a file or a directory. The last element of the path is a prefix. So the directory is modules/comments/rawcom/asdf and the files probably are asdf.idx and asdf.dat.

So if a module has been partially uninstalled, such that the directory has been removed but the conf remains, then you are in trouble. The code looks at the last element and sure enough it is not a directory. It doesn't exist. Now it backs up one, and since it is not a conf that specifies the prefix as part of the path, you get the common directory for holding all files of that type.

At this point, one can only test for the existence of expected files to find that they are not there. Then you know the module has been partially wacked.

Note, it is ok in JSword and SWORD for the module data to remain if the conf is gone. If there is no conf, there is no module.

Hope that helps.

In Him,
	DM


On Feb 12, 2013, at 3:02 PM, Chris Burrell <chris at burrell.me.uk> wrote:

> Can you explain this piece of code to me. It doesn't work very well to test the containing directory, as that might well be the directory holding all the modules of a particular type. For example, I have mak.conf, but it is not present in: C:\Users\Chris\Application Data\Sword\modules\comments\rawcom
> 
> Why are we then testing that rawcom exists and if it does accepting it?
> 
> SwordBookMetaData.java:
> 
>         URI location = NetUtil.lengthenURI(library, datapath);
>         File bookDir = new File(location.getPath());
>         // For some modules, the last element of the DataPath
>         // is a prefix for file names.
>         if (!bookDir.isDirectory()) {
>             // Shorten it by one segment and test again.
>             lastSlash = datapath.lastIndexOf('/');
>             datapath = datapath.substring(0, lastSlash);
>             location = NetUtil.lengthenURI(library, datapath);
>             bookDir = new File(location.getPath());
>             if (!bookDir.isDirectory()) {
>                 // TRANSLATOR: This indicates that the Book is only partially installed.
>                 throw new MissingDataFilesException(JSMsg.gettext("The book is missing its data files", cet.getValue(ConfigEntryType.INITIALS)));
>             }
>         }
> 
> 
> 
> 
> On 12 February 2013 19:26, DM Smith <dmsmith at crosswire.org> wrote:
> Sounds good.
> 
> On Feb 12, 2013, at 2:18 PM, Chris Burrell <chris at burrell.me.uk> wrote:
> 
>> I agree with not coding against dodgy developers (although to be fair I'm not entirely sure how the conf files have been left over).
>> 
>> But, I'd like to suggest we add a line in the reading of the conf file to validate that the module data directory actually exists and is non-empty. If the directory is empty/doesn't exist, we remove the book from the returned set of installed()
>> 
>> Chris
>> 
>> 
>> 
>> On 11 February 2013 23:23, DM Smith <dmsmith at crosswire.org> wrote:
>> Ah. I see. The JSword code (IIRC) deletes the conf first and then the files. I downloaded the module just fine and deleted it from w/in the app as well. I think we should not go out of our way for a developer that wacks parts of modules. :)
>> 
>> That said, your question regarding the error is still worthy of solving. There are many reasons that a backend might fail and it would make sense to have robust code to handle such failures.
>> 
>> In Him,
>> 	DM
>> 
>> 
>> 
>> On Feb 11, 2013, at 5:59 PM, Chris Burrell <chris at burrell.me.uk> wrote:
>> 
>>> Dm
>>> 
>>> The problem is that I must likely deleted the files. But not the Conf file.  
>>> Just wanted to highlight npe as we probably want to avoid having one at that stage
>>> Chris
>>> 
>>> On 11 Feb 2013 22:54, "DM Smith" <dmsmith at crosswire.org> wrote:
>>> 
>>> On Feb 11, 2013, at 5:27 PM, Chris Burrell <chris at burrell.me.uk> wrote:
>>> 
>>> > Hi
>>> >
>>> > On line 124, we have a potential NPE:
>>> >
>>> > state.getIdxRaf() can return a null value if for some reason we are attempting to read the cardinality of a module, but the idx file for the module is not present.
>>> >
>>> > In this case, I'm looking at the 2babdict module, for which I have a conf file but no module files under modules/lexdict/rawld
>>> >
>>> > Obviously, this is an issue, but I wonder if the reading of the .conf file should identify the absence of modules and therefore
>>> > a- report early to the user that something is wrong
>>> > b- not show-up in the list of installed() modules
>>> 
>>> The code regarding backends is in process of change, maybe done.
>>> 
>>> The way the code used to work:
>>> For the list of installed books (i.e. for each conf) build an appropriate backend, which would log an error and throw an exception if the files for the module were not present and mark the module as unsupported.
>>> 
>>> The problem with that model is that it is expensive on startup when lots of modules are installed. There is a Jira issue regarding improving this performance.
>>> 
>>> The other problem is that it has no memory of the error and on next startup, it does the work again.
>>> 
>>> It seems to me that when the module is installed, that'd be a great time to validate it fully and mark it as validated.
>>> 
>>> In JSword we have ReporterUtil to let the user know of events like this. It might not work in STEP as it is tied to a listener mechanism.
>>> 
>>> It'd be good to iterate on this to get a good solution.
>>> 
>>> I'm looking at 2babdict to see if I can identify the problem.
>>> 
>>> In Him,
>>>         DM
>>> 
>> 
>> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.crosswire.org/pipermail/jsword-devel/attachments/20130212/12ce1123/attachment-0001.html>