Last night's checkin actually worked better than I expected. However, I've spotted a few problems with the current builds, which I've fixed in ways that the group, especially Janne, should know about.
A huge number of errors were caused by calls to ContentManager.createPage(). When this method runs, it attempts to create a new JCR node in the repository. But if that node already exists, a ProviderException is thrown. Unfortunately, the equivalent method in pre-3.0 versions would silently overwrite the page instead. This is important to understand, because it means that callers need to explicitly check whether a page (node) exists before they attempt to add one. So, I've fixed this as follows: - Added a PageAlreadyExistsException class, which is thrown by ContentManager.createPage() when... wait for it... the page already exists. This makes for some mildly elaborate try/catch blocks in certain places, but that's ok. - Located every instance in the test classes where createPage() is called, and made sure that it is preceded by WikiEngine.deletePage(). This ensures that the page won't exist when createPage() is called, and thus won't throw the error in unit tests where it shouldn't. - A few of the test cases that called createPage() were actually referring to pages created in the suite's setUp(). In these cases, we now call WikiEngine.getPage() instead of ContentManager.createPage(). All of this is subject to change, of course -- let me know if we ought to be doing things differently. As of 3.0-svn build 85 (tonight), unit test success rate is up to 86%; 81 failures 48 errors. -- Andrew On Mon, Mar 16, 2009 at 9:36 PM, Andrew Jaquith <[email protected]> wrote: > I've gone through the code and have tried to catch or propagate "all" > instances of PageNotFoundException. I did this by breaking the > super/subclass relationship with ProviderException and then fixing > every compilation error. A cheap and nasty technique, but it seems to > have worked. We can restore the relationship in a later build, but > it's not obvious that we should. :) > > I will be checking in that batch, plus some completely unrelated > Stripes-related stuff, presently. > > You should expect the SVN branch to be even more broken than it has > been for a few days. Stabilizing it will be my top priority. > > Andrew > > On Mon, Mar 16, 2009 at 2:53 AM, Janne Jalkanen > <[email protected]> wrote: >> >> Yup, it definitely was a pain ;-). But I found *so* many potential NPE >> situations while doing it it was embarrassing. >> >> However, what I didn't do was to review the rest of the WikiEngine API to >> see if the methods throw the proper exceptions. Some of them might be >> throwing ProviderExceptions, and since PageNotFoundException is a PE, we >> might want to put it separately in the signature so that it can be properly >> documented. >> >> (Switched to dev-list). >> >> /Janne >> >> On 16 Mar 2009, at 02:20, Andrew Jaquith wrote: >> >>> Any objection to me changing the various WikiEngine.getPage() methods >>> so that they throw a PageNotFoundException instead of returning null? >>> Yeah, it's sort of a pain to do in the short term, but NOT doing it is >>> proving problematic in the 3.0 codebase. >>> >>> On Thu, Mar 12, 2009 at 2:28 PM, Janne Jalkanen >>> <[email protected]> wrote: >>>> >>>> Except that in this case, our API has defined that getPage() returns null >>>> if >>>> the page is not found. In JCR, however, not finding an item is >>>> considered >>>> to be an exception, so unless we change the way we work, it is mandatory. >>>> >>>> For all other exceptions, we catch and propagate them upwards properly. >>>> >>>> /Janne >>>> >>>> On Mar 12, 2009, at 20:24 , Foster Schucker wrote: >>>> >>>>> +1 providing something catches the exception and DOES something with it. >>>>> Just catching and returning null is worse. >>>>> >>>>> Harry Metske wrote: >>>>> >>>>>> +1 >>>>>> >>>>>> >>>>>> >>>>>> 2009/3/12 Janne Jalkanen <[email protected]> >>>>>> >>>>>> >>>>>>> Simple enough reason: getPage(WikiName,int) is not catching the >>>>>>> PathNotFoundException - look at the catch clause in getPage(WikiName) >>>>>>> ;-). >>>>>>> It should do that and return null. >>>>>>> >>>>>>> My bad, sorry! >>>>>>> >>>>>>> (A general question, should we start throwing something like >>>>>>> PageNotFoundExceptions as opposed to returning nulls? That would >>>>>>> encourage a >>>>>>> bit safer coding and would eliminate a number of if(getPage() == null) >>>>>>> tests >>>>>>> across the codebase.) >>>>>>> >>>>>>> /Janne >>>>>>> >>>>>>> >>>> >>>> >> >> >
