NH-3142: Batch-loading of lazy children failing when key is composite
(regressed in 3.2)
Sorry about the long rant, but I would appreciate if someone has any
existing knowledge or thoughts to share on the matter.
The Parameter class begin it's life in 2004, containing properties
such as table alias and DbType. It evolved to became first immutable,
then a singleton. Perhaps Flyweight is a better classification, but
the point is Parameter instances had no individual state and all
instances were considered equal.
This state persisted until 2008-10-15 when Ayende added what was then
called the OriginalPositionInQuery property which is in fact mutable -
at the same time the equality comparison was strengthened to make even
operator== consider two separate instances the same. The
OriginalPositionInQuery property was ignored for equality. This fix
was for NH-1528 and Ayende at the time stated "I am not really happy
about what I had to do".
Then in 2011-05-23 it gained the BackTrack property (which is also
mutable) as a more general method of tracking where the parameter
comes from. In a way, I suppose one can argue that all parameters are
now named, at least internally.
The point of all this? Well there is at least some code, untouched for
many years, which still regards all parameter instances as equal. For
instance, the sql generated to batch load collection elements when the
parent has a composite identifier will use the same parameter
instances multiple times. Loader.GetParameterSpecifications() will
then attempt to set the backtrack, but of course all backtrack values
except those from the last parent in the batch will be overwritten,
making the query contain identifiers for only one parent in the batch
- all but one of the collections will be initialized as empty.
I'm unsure about the proper fix - longterm and shorttem:
Loader.GetParameterSpecifications() is side-effecty and quite ugly to
my eyes. It could be altered to copy the SqlString (which will also
create separate parameter instances) before setting the backtrack. The
caller copies the SqlString immediately after anyway, so this would
just rearrange it a bit.
Alternatively, since Parameter instances really are named parameters
nowadays, instead of just placeholders, maybe JoinWalker.WhereString()
should be fixed to not reuse the same parameter instances? As it is,
they sort of start out as positional, but then becomes effectively
named when the BackTrack is set. Given my next point, I'm leaning
towards this solution.
I think the handling of BackTrack etc. is quite ugly at the moment,
but I'm not clear on what to do longterm. Perhaps it's possible to
make the BackTrack also immutable and be set by the constructor. Today
there is a lot of nearly-duplicated code that sets the BackTrack
spread over several places.
Another little gem:
foreach (object part in sql.Parts)
{
if (part == Parameter.Placeholder)
Placeholder since a couple of years back allocate a new instance, so
we instantiate a useless parameter for each part in the SqlString. :)
/Oskar
