QOF Book Merge design issues

[Derek Atkins]

Neil Williams <linux at codehelp.co.uk> writes:

> On Sunday 05 December 2004 1:30 am, Derek Atkins wrote:
>> Neil,
>>
>> In my quick attempt to debug the book merge test, I started taking
>> a closer look into the book merge code.  I see a major (critical)
>> design issue:  you've got static variables holding the merge state!
>> BAD idea.
>
> OK, . . . I never envisaged any process having >1 instance of 
> qof_book_mergeData though - I couldn't see the need.

We're building a general-purpose Library.  Imagine an application that
has multiple simultaneous sessions..  The application could be running
two merge processes into two different target books simultaneously.
Your code can't handle that.

Admittedly the current gnucash code doesn't work that way -- we have
lots of places where the session/book is assumed by the code, but that
doesn't mean new code should continue to assume that.  I'd like to
make the apis all pass around session/book/whatever contexts so you
never need to assume the book/session/whatever.

>> You really need to have a "merge context" that gets created when you
>> initialize the merge process and contains all the state of the merge.
>
> Can that be global or does it have to be passed to and fro with every single 
> function? Do I have to return a qof_book_mergeData* from Init, pass it to 
> qof_book_mergeUpdateResult and redefine the entire 
> qof_book_mergeRuleForeachCB callback then pass mergeData back to Commit?

It needs to be passed to and fro with every function.  So, yes, that's
what you need to do.  Note that you can create a link to the context
internally.  E.g., an Account has a link to the Book which has a link
to the Session -- so you only need an Account to know the whole
context.

> i.e. would just making the current pointers non-static work?

No.

>> Otherwise, what happens if the user initiates two merge processes at
>> the same time?  Your current code will blow up if the user does that.
>
> Is there a real situation where two instances would be necessary AND 
> desirable? 

Yes.  See my first paragraph above.

> Couldn't I add code to make the second mergeInit just abort if it can detect 
> that another instance already exists? That could be simple as 
> g_return_val_if_fail(mergeData != NULL, -1); in qof_book_mergeInit, before 
> any of the other code executes or modifies the current statics.

No.  That's not very friendly to the user.

> e.g. there would be more trauma if two instances tried to operate on the same 
> book. Imagine the nightmare of the importBook of instance A being the 
> targetBook of instance B! qof_object_foreach <=> qof_book_mergeForeach may 
> descend into an infinite loop / race condition. (Or have I got that mixed 
> up?) The only way to detect that is for each instance to know about the other 
> and compare QofBook pointers. How would that be achieved?

You don't have to worry about it -- let the application be intelligent
about it.

> I can't see it ever being possible to have more than one instance where each 
> points to the same targetBook - updating the same entities twice is certain 
> to cause data loss. IMHO, merges must be sequential, not simultaneous.

Sure, but that doesn't mean you can't have multiple simultaneous
merges going on.  They just need to use separate books.  Your code
doesn't allow that.

> Can users be expected to cope with resolving conflicts in two different but 
> simultaneous merges?

Yes.

>> Just my $2.02, but you really need to pass around your context instead
>> of making it a static.
>
> That complicates the API considerably. e.g. using existing qof_object_foreach 
> and other callbacks becomes tricky, all kinds of extra data gets put into 
> qof_book_mergeData just so that it can be passed around as a gpointer 
> user_data. 

So?  That's not a major problem, IMHO.  That's exactly WHY we have
user_data callback arguments, so you can pass back a context pointer
and not have global storage.

-derek

-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available