UpsertBy without defined key columns in the InMemory adapter #262

Closed
Vidarls opened this Issue Mar 13, 2013 · 3 comments

Comments

Projects
None yet
2 participants
@Vidarls
Contributor

Vidarls commented Mar 13, 2013

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

This comment has been minimized.

Show comment
Hide comment
@markrendle

markrendle Mar 14, 2013

Owner

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.

Owner

markrendle commented Mar 14, 2013

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

This comment has been minimized.

Show comment
Hide comment
@Vidarls

Vidarls Mar 14, 2013

Contributor

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.

Contributor

Vidarls commented Mar 14, 2013

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

This comment has been minimized.

Show comment
Hide comment
@markrendle

markrendle May 21, 2013

Owner

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

Owner

markrendle commented May 21, 2013

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

@markrendle markrendle closed this May 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment