markrendle / Simple.Data

A light-weight, dynamic data access component for C# 4.0
MIT License
1.33k stars 303 forks source link

UpsertBy without defined key columns in the InMemory adapter #262

Closed Vidarls closed 11 years ago

Vidarls commented 11 years ago

I'm not sure if this classifies as a bug, or if it's just me not understanding the limitations.

I started using upserts without bothering to read up on them assuming they where simply a convenience wrapper around

if (db.Table.FindById(1).Any())
{
  db.Table.UpdateById(dataWithId1);
}
 else
{
  db.Table.Insert(dataWithId1);
}

Following that assumption I imagined the mechanics would be the same as for the discrete calls

For inserts they are. UpsertById(1)works perfectly fine.

When it comes time to update using upsert, I get an ArgumentNullException from the Simple.Data.Adapter.Upsert() method due to the table not having any defined keys.

Reading the tests for the InMemoryAdapter i found this:

[Test]
        public void UpsertWithoutDefinedKeyColumnsSHouldThrowMeaningfulException()
        {
            var adapter = new InMemoryAdapter();
            Database.UseMockAdapter(adapter);
            var db = Database.Open();
            var exception = Assert.Throws<InvalidOperationException>(() => db.Test.Upsert(Id: 1, HasTowel: true));
            Assert.AreEqual("No key columns defined for table \"Test\"", exception.Message);
        }

Indicating that the InMemoryAdapter in fact needs defined keys to even use the Upsert methods. This conflicts with my experience where the first upsert call worked just fine, and the second one threw an obscure ArgumentNull exception.

Adding to that, the code that requires keys are in the core Simple.Data.Adapter implementation, not in the InMemoryAdapter.

Glancing over the Simple.Data.Ado implementation, it seems to work as I first anticipated, with no requirement to the criteria being defined as keys in the schema.

Are there some background behind the InMemory adapter requiring defined keys to use upsert? If not I'll have a go at changing it.

markrendle commented 11 years ago

Upsert() (as opposed to UpsertByX()) needs there to be defined keys in the schema, otherwise how would it know which columns to use as criteria and which to use for modified values.

I'll fix up the exception in the InMemoryAdapter.

Vidarls commented 11 years ago

I was expecting UpserBy to not require defined keys, but got slightly confused by the test and the exception. Appreciate the fix, thank you for still putting in the effort.

markrendle commented 11 years ago

The Adapter structure is being rearchitected for 2.0, this will be improved as part of that process.