TValue.FromVariant is causing issues in other code locations

Issue #74 closed
Todd Flora created an issue

Linas,

Now that you have a FromVariant that deals with the Oracle Issues in TUtils.FromVariant method there are other places where this needs to be called.

For instance: Spring.Persistence.Sql.Comands.Insert has 3 places where TValue.FromVariant is used.

I have already had one of these places, GetAutoGeneratedPrimaryKeyValue method, fail with an exception because of the Oracle BCD Issue where our Number(19) Sequence is being retrieved from the database. I made the change to call the TUtils version of the fromvariant method and the issue is resolved.

function TInsertExecutor.GetAutogeneratedPrimaryKeyValue(
  const entityWrapper: IEntityWrapper): TValue;
var
  statement: IDBStatement;
  value: Variant;
begin
  Result := TValue.Empty;
  if SupportsSequences then
  begin
    statement := Connection.CreateStatement;
    statement.SetSQLCommand(fGetSequenceValueSQL);
    value := statement.ExecuteQuery.GetFieldValue(0);
    {TFlora Use TUtils}
    Result := TUtils.FromVariant(value);
  end
  else if CanClientAutogenerateValue then
  begin
    {TFlora Use TUtils}
    Result := TUtils.FromVariant(Generator.GenerateUniqueId);
    fCommand.InsertFields.Add(
      TSQLInsertField.Create(
        EntityData.PrimaryKeyColumn.ColumnName,
        fTable, EntityData.PrimaryKeyColumn,
        fCommand.GetAndIncParameterName(EntityData.PrimaryKeyColumn.ColumnName)));
  end;
end;

It seems that all places where the Spring.Persistence code calls TValue.FromVariant should be replaced with the TUtils.FromVariant call instead.

What do you think?

Comments (11)

  1. Linas Naginionis

    I think we wouldn't need to use TUtils.FromVariant at all if all the vendor specific value conversions would be implemented in the adapters. This way main ORM code won't do any value conversions, it would be the responsibility of the specific adapter.

  2. Todd Flora reporter

    Understood and that makes sense, but until the conversion code is moved into the individual adapters would it be possible to modify the Spring.persistence.sql.commands.insert unit where TValue.FromVariant is called in reference to Database data to use TUtils.FromVariant instead? It is only 3 places that I see. This will at least in the interim will resolve any issues with Oracle Data Types.

  3. Stefan Glienke repo owner

    We will change the API to return TValue from the adapters instead of Variant. In the meantime I think you got this covered with changes to you local code.

  4. Stefan Glienke repo owner

    I looked into this issue again and I am not sure anymore if changing to TValue is the best way.

    As Linas originally said handling the DB specific types should be done within the adapter. But that might affect different adapters (like you can connect to Oracle by ADO or by FireDAC or a completely different component). But still it would be the cleanest way to not ripple any specifics into the DBMS agnostic layer.

    Am I right when I say that only the GetFieldValue methods in the resultset adapter need that handling as they are the only places where values come from the DB into the ORM or am I missing a place?

  5. Todd Flora reporter

    Also if a SQL Statement is used to get the sequence value and this is an Int64 the problem exists so I have changed the TValue.FromVariant call in spring.persistence.SQL.Commands.Insert.pas in the following method.

    TInsertExecutor.GetAutogeneratedPrimaryKeyValue: TValue;

    Two places in this method.

    function TInsertExecutor.GetAutogeneratedPrimaryKeyValue: TValue;
    var
      statement: IDBStatement;
      value: Variant;
    begin
      Result := TValue.Empty;
      if SupportsSequences then
      begin
        statement := Connection.CreateStatement;
        statement.SetSQLCommand(fGetSequenceValueSQL);
        value := statement.ExecuteQuery.GetFieldValue(0);
        {TFlora Use TUtils}
          Result := TUtils.FromVariant(value);
      end
      else if CanClientAutogenerateValue then
      begin
        {TFlora Use TUtils}
        Result := TUtils.FromVariant(Generator.GenerateUniqueId);
        fCommand.InsertFields.Add(
          TSQLInsertField.Create(
            EntityData.PrimaryKeyColumn.ColumnName,
            fTable, EntityData.PrimaryKeyColumn,
            fCommand.GetAndIncParameterName(EntityData.PrimaryKeyColumn.ColumnName)));
      end;
    end;
    
  6. Stefan Glienke repo owner

    Todd, I read your original post - no need to repost the exact same code again ;)

    The first place is using the GetFieldValue method I mentioned and the second is never implemented except for MongoDB and should not access the DB because otherwise the condition would not be called CanClientAutogenerateValue, right?

  7. Todd Flora reporter

    OK, I am not as familiar with each branch of code as you are. The code failed in this method when retrieving a number(19) for the Key Value using a SQL Statement. I had to change it in one or the other block in this method. I don't remember which. I saw the other spot and just thought that is should call it here also just in case this block is called ever using Oracle. I did not realize this block was for MongoDB as my world is only Oracle and MySql.

    I am unconcerned about how you solve this issue. Just that whenever an Oracle query returns a Number(19) or greater that we do not loose precision and we don't blow with an error because TValue.FromVariant does not know how to deal with the variant type returned.

    Thx, Todd.

  8. Log in to comment