You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jspwiki.apache.org by Andrew Jaquith <an...@gmail.com> on 2009/10/18 06:12:48 UTC
Re: svn commit: r826317 - in /incubator/jspwiki/trunk:
src/java/org/apache/wiki/WikiEngine.java src/java/org/apache/wiki/content/ContentManager.java
tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
I agree that this might potentially be a bug -- or more correctly put,
a not-very-well-documented way for a developer to get into trouble.
But I am not sure the solution is quite as easy as creating a new
version and saving the old one. Not all saves are guaranteed to
complete because of the workflow stuff. It might be better to simply
have WikiEngine.createNode() look for the existence of Nodes whose
isNew() returns true, and return it instead (which would mean the
contents might get overwritten, but whatever).
Andrew
On Sat, Oct 17, 2009 at 6:52 PM, Janne Jalkanen
<ja...@ecyrd.com> wrote:
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> ======================================================================
>> ---
>> incubator/jspwiki/trunk/tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
>> (original)
>> +++
>> incubator/jspwiki/trunk/tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
>> Sat Oct 17 22:49:04 2009
>> @@ -2327,7 +2327,14 @@
>> {
>> Benchmark sw = new Benchmark();
>> sw.start();
>> +
>> + // Create the page and save it to disk.
>> + if ( !testEngine.pageExists( PAGE_NAME ) )
>> + {
>> + testEngine.saveText( PAGE_NAME, brokenPageText );
>> + }
>>
>> + // Test the rendering speed
>
> Not sure whether this is the right fix - I mean, saveText() should actually
> check for this itself, and in fact, it should save a new version of the page
> whenever it's called - NOT create a new SNS. Yes, it fixes the test, but I
> have a feeling that it only hides a real bug.
>
> /Janne
>
Re: svn commit: r826317 - in /incubator/jspwiki/trunk: src/java/org/apache/wiki/WikiEngine.java src/java/org/apache/wiki/content/ContentManager.java tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
Posted by Janne Jalkanen <Ja...@ecyrd.com>.
> But I am not sure the solution is quite as easy as creating a new
> version and saving the old one. Not all saves are guaranteed to
> complete because of the workflow stuff. It might be better to simply
> have WikiEngine.createNode() look for the existence of Nodes whose
> isNew() returns true, and return it instead (which would mean the
> contents might get overwritten, but whatever).
Yup, this should work... The other possibility would be to throw a
PageExistsException (which would enforce one to be a bit more careful
with the code), but perhaps returning the already-existing -page is
nicer.
/Janne
Re: svn commit: r826317 - in /incubator/jspwiki/trunk:
src/java/org/apache/wiki/WikiEngine.java src/java/org/apache/wiki/content/ContentManager.java
tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
Posted by Andrew Jaquith <an...@gmail.com>.
Sorry. I should have said: ContentManager.addPage().
On 10/18/09, Andrew Jaquith <an...@gmail.com> wrote:
> Just checked in a fix. I added some code to ContentManager.addNode()
> that checks for the existence of an unsaved Node. If one is found, it
> is re-used in the WikiPage that is returned to the caller. Works
> nicely and doesn't break anything.
>
> On 10/18/09, Andrew Jaquith <an...@gmail.com> wrote:
>> I agree that this might potentially be a bug -- or more correctly put,
>> a not-very-well-documented way for a developer to get into trouble.
>>
>> But I am not sure the solution is quite as easy as creating a new
>> version and saving the old one. Not all saves are guaranteed to
>> complete because of the workflow stuff. It might be better to simply
>> have WikiEngine.createNode() look for the existence of Nodes whose
>> isNew() returns true, and return it instead (which would mean the
>> contents might get overwritten, but whatever).
>>
>> Andrew
>>
>> On Sat, Oct 17, 2009 at 6:52 PM, Janne Jalkanen
>> <ja...@ecyrd.com> wrote:
>>>> =
>>>> =
>>>> =
>>>> =
>>>> =
>>>> =
>>>> =
>>>> =
>>>> ======================================================================
>>>> ---
>>>> incubator/jspwiki/trunk/tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
>>>> (original)
>>>> +++
>>>> incubator/jspwiki/trunk/tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
>>>> Sat Oct 17 22:49:04 2009
>>>> @@ -2327,7 +2327,14 @@
>>>> {
>>>> Benchmark sw = new Benchmark();
>>>> sw.start();
>>>> +
>>>> + // Create the page and save it to disk.
>>>> + if ( !testEngine.pageExists( PAGE_NAME ) )
>>>> + {
>>>> + testEngine.saveText( PAGE_NAME, brokenPageText );
>>>> + }
>>>>
>>>> + // Test the rendering speed
>>>
>>> Not sure whether this is the right fix - I mean, saveText() should
>>> actually
>>> check for this itself, and in fact, it should save a new version of the
>>> page
>>> whenever it's called - NOT create a new SNS. Yes, it fixes the test,
>>> but
>>> I
>>> have a feeling that it only hides a real bug.
>>>
>>> /Janne
>>>
>>
>
Re: svn commit: r826317 - in /incubator/jspwiki/trunk:
src/java/org/apache/wiki/WikiEngine.java src/java/org/apache/wiki/content/ContentManager.java
tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
Posted by Andrew Jaquith <an...@gmail.com>.
Just checked in a fix. I added some code to ContentManager.addNode()
that checks for the existence of an unsaved Node. If one is found, it
is re-used in the WikiPage that is returned to the caller. Works
nicely and doesn't break anything.
On 10/18/09, Andrew Jaquith <an...@gmail.com> wrote:
> I agree that this might potentially be a bug -- or more correctly put,
> a not-very-well-documented way for a developer to get into trouble.
>
> But I am not sure the solution is quite as easy as creating a new
> version and saving the old one. Not all saves are guaranteed to
> complete because of the workflow stuff. It might be better to simply
> have WikiEngine.createNode() look for the existence of Nodes whose
> isNew() returns true, and return it instead (which would mean the
> contents might get overwritten, but whatever).
>
> Andrew
>
> On Sat, Oct 17, 2009 at 6:52 PM, Janne Jalkanen
> <ja...@ecyrd.com> wrote:
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ======================================================================
>>> ---
>>> incubator/jspwiki/trunk/tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
>>> (original)
>>> +++
>>> incubator/jspwiki/trunk/tests/java/org/apache/wiki/parser/JSPWikiMarkupParserTest.java
>>> Sat Oct 17 22:49:04 2009
>>> @@ -2327,7 +2327,14 @@
>>> {
>>> Benchmark sw = new Benchmark();
>>> sw.start();
>>> +
>>> + // Create the page and save it to disk.
>>> + if ( !testEngine.pageExists( PAGE_NAME ) )
>>> + {
>>> + testEngine.saveText( PAGE_NAME, brokenPageText );
>>> + }
>>>
>>> + // Test the rendering speed
>>
>> Not sure whether this is the right fix - I mean, saveText() should
>> actually
>> check for this itself, and in fact, it should save a new version of the
>> page
>> whenever it's called - NOT create a new SNS. Yes, it fixes the test, but
>> I
>> have a feeling that it only hides a real bug.
>>
>> /Janne
>>
>