[hibernate-dev] Patch for HHH-272: Custom SQL for column gets and sets

[Rob Hasselbaum]

Hi Steve,

Thanks for the feedback. I am working on moving the patch to trunk as 
you requested. I'll update the bug when that's done. My comments on the 
rest of your feedback are inline below.

On 09/03/2009 04:39 PM, Steve Ebersole wrote:
> 1) Personally, I don't like the attribute names sql-get and sql-set.
> When I think through trying to describe and explain this feature to
> people, the terms "wrap" and "unwrap" keep coming to my head as being
> descriptive, relevant and natural.  It was really the "naturalness"
> aspect that got me with sql-get and sql-set.  Other terms I could think
> of included encode/decodeAny other suggestions? 
>   

I went with "sql-set" and "sql-get" because they are consistent with the 
existing "sql-type" attribute and make sense to me, but I'm not married 
to the terms. Wrap and unwrap sound good, too. Bear in mind that the 
expressions need not be function calls, which wrap and unwrap might 
suggest. The functional tests do math, for instance.

> 2) Can anyone foresee a valid use-case for allowing one of these to be
> defined w/o the other?  The only thing I came up with was for immutable
> properties.  So something like <column name="xyz" unwrap="decode(xyz)"
> insert="false" update="false"/>, which can also be defined using a
> formula like <formula>decode(xyz)</formula>.  The reason I ask is that
> if they should always be used together then maybe it is better to
> enforce that in the DTD and/or binder
>   

I couldn't think of a use case for a one-way conversion either. In fact, 
you could end up reading out a different value than the one you put in 
if you're not careful. But I'd regret adding an artificial restriction 
if someone came up with a legitimate use case next year.

> 3) You renamed the getTemplate method on Selectable to
> getGetterTemplate.  Everything in the o.h.mapping package is an SPI that
> is used by many other libraries.  We need to be very careful about
> changing stuff in here.  I am not sure if folks bind to this particular
> method.  But since this is just a cosmetic change, I think it should be
> reverted.
>
>   

Sorry, this was an oversight. At first, I thought I was going to need to 
store a setter template in addition to a getter template in the Column 
class,  It turned out that I didn't need it, but I forgot to revert the 
name of the getter template. I'll fix that.

> Still not done looking through the whole patch.  Will try to finish up
> tomorrow.
>
>   

Thanks,
-Rob