[jira] Commented: (DERBY-4062) Remove single-argument getDataValue overrides from DataValueFactory interface

["Bryan Pendleton (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/DERBY-4062?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12705659#action_12705659 ] 

Bryan Pendleton commented on DERBY-4062:
----------------------------------------

Consider this little snippet of code from SQLBooleanConstantNode.java:

                if ( val == null )
                {
                        setValue(getTypeServices().getNull() );
                }
                else
                {
                        setValue(getDataValueFactory().getDataValue(val.booleanValue()));
                }

It seems like there are (at least) 4 possible ways to re-write it, with (I believe) the same result.

The first way I thought of was to replace the call to the single-argument version of
getDataValue(boolean) with a call to getDataValue(boolean, BooleanDataValue). This makes
this bit of code a little bit messier, but it allows us to remove getDataValue(boolean) from
the DataValueFactory interface.

                if ( val == null )
                {
                        setValue(getTypeServices().getNull() );
                }
                else
                {
                        setValue(getDataValueFactory().getDataValue(val.booleanValue(), (BooleanDataValue)null));
                }

The second way I thought of is to use Knut's suggestion of directly calling the SQLBoolean
constructor that we want, which in this case is SQLBoolean(boolean):

                if ( val == null )
                {
                        setValue(getTypeServices().getNull() );
                }
                else
                {
                        setValue(new SQLBoolean(val.booleanValue()));
                }

The third way to handle this is to observe that DataValueFactoryImpl.getDataValue(Boolean)
already knows what to do if the passed-in Boolean object is null, so we can simplify
this entire code in SQLBooleanConstantNode.java, I think, with the following:

                setValue(getDataValueFactory().getDataValue(val));

The fourth way to handle this is to again eliminate the call to the DataValueFactory,
which doesn't seem to be adding much value here, and simply construct a SQLBoolean
object using the SQLBoolean(Boolean) constructor, which, too, knows what to do if
the passed-in Boolean object is null, so we can just write:

                setValue(new SQLBoolean(val));

This last code seems quite clear and simple, but I'm worried that maybe I'm over-simplifying
things too much? Still, I think I'll at least pursue this 4th approach, and see what sort of
a patch proposal I end up with.


> Remove single-argument getDataValue overrides from DataValueFactory interface
> -----------------------------------------------------------------------------
>
>                 Key: DERBY-4062
>                 URL: https://issues.apache.org/jira/browse/DERBY-4062
>             Project: Derby
>          Issue Type: Sub-task
>            Reporter: Bryan Pendleton
>            Assignee: Bryan Pendleton
>            Priority: Minor
>         Attachments: addToInterface.diff, getDataValueCalls.out
>
>
> [ Issue title edited to reflect the discussion about how to clarify the use of this interface. ]
> I believe the problem involves o.a.d.iapi.types.DataValueFactory.
> This interface defines dozens and dozens of overloads of the method
> getDataValue(), for lots of different combinations of datatypes.
> For most of the Java "boxed" types (Short, Long, Float, Double, etc.),
> DataValueFactory defines a pair of getDataValue() methods. For example,
> here are the method pair that the interface defines for Short:
>         /**
>          * Get a SQL smallint with the given value.  A null argument means get
>          * a SQL null value.  The second form uses the previous value (if non-null)
>          * to hold the return value.
>          *
>          */
>         NumberDataValue         getDataValue(Short value);
>         NumberDataValue         getDataValue(Short value, NumberDataValue previous)
>                                                         throws StandardException;
> HOWEVER, for the Integer type, DataValueFactory doesn't define both overloads,
> but only defines the 'previous'-style overload:
>         /**
>          * Get a SQL int with the given value.  A null argument means get
>          * a SQL null value.  Uses the previous value (if non-null)
>          * to hold the return value.
>          *
>          */
>         NumberDataValue         getDataValue(Integer value, NumberDataValue previous)
>                                                         throws StandardException;
> The actual implementation, in o.a.d.iapi.types.DataValueFactoryImpl, though,
> does implement both the Integer overloads. But this method is NOT present
> in the DataValueFactory interface:
>         NumberDataValue         getDataValue(Integer value);
>  Because this method is not present in the interface, code such as
>    row.setColumn(SYSXPLAIN_RESULTSET_NO_OPENS, dvf.getDataValue(no_opens));
> which the code anticipates will invoke the above method, instead calls the method
>    public UserDataValue getDataValue(Object value); 
> which has a very different behavior (instead of returning a SQLInteger, it returns a UserType).
> This accidental invocation of the wrong implementation method was causing data corruption
> errors in regression tests for the DERBY-2487 patch, which uses the above setColumn call.
> Instead of inserting SQLInteger values into the system table, the code was inserting
> java.lang.Integer UserType values; since those values don't match the defined type of
> the column(s) in the system catalog, the table appeared to be corrupt.
> I believe that this problem never affects external Derby applications, but only internal Derby code,
> as the DataValueFactory interface is an internal interface only. Still, since it appeared to
> cause data corruption and invalid query results, it is potentially a quite serious problem.
> See this thread in the derby-dev archives for a bit more discussion:
> http://mail-archives.apache.org/mod_mbox/db-derby-dev/200902.mbox/%3C4997818E.3080007@amberpoint.com%3E

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.