Posts
48
Comments
146
Trackbacks
0
Why does SubSonic’s SimpleRepository ‘Add<T>’ return a decimal instead of an int? (Part 2)

Last time I was taking a look at SubSonic’s SimpleRepository functionality and wondering about the return value of the ‘Add<T>’ method. More specifically, I was wondering why the ‘object’ instance being returned was typed as a decimal rather than an int when the object I was persisting had a primary key field that is typed as an int.

I had discovered that the while Add<T> was returning a decimal, it was also updating the primary key field (PostID on my Post class instance in this case) with the same value; essentially I was getting the correct number back both from the return value and the PostID field but the values were being typed differently.

Since SubSonic is an open source project I have the luxury of pulling down the code and having a look for myself. This was easily accomplished by doing a git clone of the repository at github: git://github.com/subsonic/SubSonic-3.0.git to a folder on my local machine. Opening the solution and building the source code only took a second and I was then able to directly reference the newly built SubSonic.Core assembly in the bin folder of my local SubSonic copy from my sample project so that I can easily F11 into the Add<T> method and see what’s going on.

This little block of code appeared to be the origin of the return value for the method:

object result = null;
using(var rdr = item.ToInsertQuery(_provider).ExecuteReader())
{
    if(rdr.Read()) 
    result = rdr[0];
}

 

The ‘rdr’ variable above is a simple System.Data.IDataReader, the run-time instance of which is a SqlDataReader in this case. To try and understand what’s happening it’s helpful to know what the SQL that the data reader is reading from looks like. Drilling down a little bit further into the ‘ToInsertQuery’ method can show us the SQL that’s being built up at run time:

image

 

Pulling that SQL out and formatting it some gives us this:

INSERT INTO [Posts](
  [Posts].[Title],
  [Posts].[Body],
  ...)
VALUES (
  @ins_PostsTitle,
  @ins_PostsBody,
  ...)

SELECT SCOPE_IDENTITY() as new_id

 

So our data reader is reading back the result of ‘SELECT SCOPE_IDENTITY()’. Taking a quick look at the ‘Posts’ table that the SimpleRepository created in my database reveals that the PostID column is indeed set as the primary key/identity and is typed as an int. Some poking around in the MSDN documentation reveals what’s going on:

This article on SCOPE_IDENTITY shows that that it has a return type of ‘NUMERIC(38,0)’ while this article on SQL Server Data Type Mappings in ADO .NET shows us that numeric SQL Data Type gets converted to the ‘decimal’ .NET Framework type automatically. So that mystery is solved, but this still doesn’t explain how the integer typed ‘PostID’ field was correctly updated with the new identity value while the return type remained a decimal. The next code block down in the Add<T> shows us how:

if (result != null && result != DBNull.Value) {
  try {
    var tbl =  _provider.FindOrCreateTable(typeof(T));
    var prop = item.GetType().GetProperty(tbl.PrimaryKey.Name);
    var settable = result.ChangeTypeTo(prop.PropertyType);
    prop.SetValue(item, settable, null);

    } catch(Exception x) {
      //swallow it - I don't like this per se but this is a convenience and we
      //don't want to throw the whole thing just because we can't auto-set the value
    }
}
return result;

The point of this is to provide convert the ‘result’ variable to the type of the primary key of the object being persisted. I’m not sure that I agree with swallowing the exception here, though that’s easy for me to say as an outside observer who hasn’t poured hours and hours into this code. The main reason I don’t agree with it is that I feel like the calling method really needs to be able to rely getting back the identity of the newly created record and if something blows up I would want to know about it. You can always still rely on the ‘result’ object that gets returned, but as we’ve already seen you can’t simply cast that to an int as you might think you could. I’m kind of curious to know under what circumstances this code throws an exception and see if there’s any way to make it more reliable. I can see that the definition of the ‘ChangeTypeTo’ extension method can explicitly throw an exception when the underlying database is SQLLite, but the exception thrown also hints at the workaround for that issue. I suppose this kind of thing can be the cost of doing business when you’re trying to support multiple database platforms; not everyone has the luxury of being “SQL Server only”.

Oh, and the (Exception x) isn’t needed in this case since we’re not doing anything with the caught exception object; I think a simple ‘catch’ would do just fine, but I digress. :-)

It wouldn’t be very difficult to modify this code to attempt to return a properly typed ‘object’ variable since we’re already trying to convert the ‘result’ object for the purposes of setting the primary key field in the provided ‘Post’ instance by trying to do something like this:

object typedResult = null;
if (result != null && result != DBNull.Value) {
  try {
    var tbl =  _provider.FindOrCreateTable(typeof(T));
    var prop = item.GetType().GetProperty(tbl.PrimaryKey.Name);
    var settable = result.ChangeTypeTo(prop.PropertyType);
    typedResult = settable;
    prop.SetValue(item, settable, null);

    } catch(Exception x) {
         //swallow it - I don't like this per se but this is a convenience and we
                    //don't want to throw the whole thing just because we can't auto-set the value
    }
 }

 return typedResult ?? result;

 

This would attempt to return a properly typed result if the type conversation was successful but would return the original result if needed. This would let the calling code look closer to what I originally expected would work:

SimpleRepository repo = new SimpleRepository("SampleDB", SimpleRepositoryOptions.RunMigrations);
object returnValue = repo.Add<Post>(newPost);
int newPostID = (int)returnValue;
 

The problem here is that since the type conversion could swallow an exception there’s no guarantee that our cast to an int would work at runtime. This could pose an issue if you were relying on being able to determine the newly created ID of the object immediately after its edited. For example, you might want to take the user to the ‘view’ screen for the post right after they create it. So what’s the solution? I’m not sure that I have the right answer, especially if you don’t have any other means to uniquely identify your records. I think it’s pretty safe to assume that you’re going to get some kind of return value back. The calling code could ‘ToString()’ the returned object and use Int32.Parse but that kind of smells to me. You could also use ‘Convert.ToInt32’, which I think I like better. I think I would also be in favor of removing the empty catch block to be able to rely on the type conversion when the method returns. I think it partially comes down to whether or not you think these potentially platform-specific quirks should be the burden of the library or the library’s consumer. Given that the consumer is always going to be in a better position to know about the specific needs of context in which the library is going to be used I think I’m leaning toward the latter.

That said this is open source so you can always fork it and modify it for your own purposes which is part of the fun. :-)

posted on Wednesday, November 18, 2009 8:16 PM Print
Comments
Gravatar
# re: Why does SubSonic’s SimpleRepository ‘Add<T>’ return a decimal instead of an int? (Part 2)
Bruce Onder
11/21/2009 10:34 AM
Thanks for the deep dive on this, Jesse. It's been something I've been curious about, and have had a task list to find out about, but you've done my work for me! :)

So - thanks!
Gravatar
# re: Why does SubSonic’s SimpleRepository ‘Add<T>’ return a decimal instead of an int? (Part 2)
Mark Abraham
11/30/2009 11:42 AM
I've fixed the type conversion in a recent change, as follows in Extensions\database.cs, line 180-ish:

//MAbraham1, 2009/11/18: if property type is integer, why convert to decimal (valueType)?
else if (currentProp.PropertyType != valueType)
{
currentProp.SetValue(item, rdr.GetValue(i).ChangeTypeTo(currentProp.PropertyType), null);
}
else
currentProp.SetValue(item, rdr.GetValue(i).ChangeTypeTo(valueType), null);

Post Comment

Title *
Name *
Email
Comment *  
Verification
Meta
Tag Cloud