mfenniak / rethinkdb-net

A C# / .NET client driver for RethinkDB.
Other
247 stars 37 forks source link

UpdateAndReturnChanges and DeleteAndReturnChanges overloads for sequences #212

Closed jonorossi closed 9 years ago

jonorossi commented 9 years ago

These two methods only took an IMutableSingleObjectQuery<T>, unlike their Update()/Delete() cousins.

I've added overloads that accept an ISequenceQuery<T>. I didn't implement this for Replace because most the time it doesn't make sense, unless maybe you use .Limit(1), but then just use .Nth(0) and make sure the object exists first.

This obviously allows you to update/delete rows and get the objects back without querying the database again:

var u = conn.Run(Author.Table
    .Filter(r => r.TVShow == "House of Cards")
    .UpdateAndReturnChanges(r => new Author { Name = "Something Here" }));
jonorossi commented 9 years ago

FYI, the build only failed because of 3 tests in RealtimePushTests timed out. I actually had one of those unit tests fail on my machine, running it again it worked.

With this PR you can emulate the output clause in SQL Server by selecting what you've just updated/deleted (obviously use OldValue for delete):

var response = conn.Run(Author.Table
    .Filter(r => r.TVShow == "House of Cards")
    .UpdateAndReturnChanges(r => new Author { Name = "Kevin Spacey" })
);
foreach (var author in response.Changes.Select(o => o.NewValue)) {
    Console.WriteLine(author.Id);
}

I'll see if we can make a nicer syntax to get the objects.

mfenniak commented 9 years ago

Awesome, this change looks great. I've fired a rebuild of your last commit to clear that timeout issue.

Would you mind adding an update to the RELEASE-NOTES.md, please?

jonorossi commented 9 years ago

Thanks, release notes updated.

jonorossi commented 9 years ago

One thing I noticed is that response.Changes will be null instead of an empty array if the update/delete didn't match any rows. I assume this happens because the array is just deserialized and the "changes" element doesn't show up if there are no changes.

https://github.com/mfenniak/rethinkdb-net/blob/master/rethinkdb-net/DmlResponse.cs#L55

Shouldn't be a problem to handle in my application code, but API-wise it is a little strange getting a DmlResponse<T> that Changes can sometimes be null. Thoughts?

mfenniak commented 9 years ago

Hm... yes, that is a little awkward. It is consistent with the RethinkDB response to the same query, where the changes field will be missing if no changes were made, but that doesn't make it a good idea. All the uint fields have an implied default value of 0 when they're not included in the response from the server, so there's some precedent to make a reasonable value for Changes.

If DmlResponse<T> had a default value of new DmlResponseChange<T>[0] on the Changes field, I imagine that would get rid of the null value and still allow the value to be overwritten when it is returned from the server. I'm not certain that would work, but I think it'd be a decent approach to addressing the issue.

mfenniak commented 9 years ago

@jonorossi -- I'd like to add you as a collaborator on this repo, if you don't mind.

This will allow you to merge your own PRs if you feel confident about them, or make other changes in the repo as you see fit. Your contributions to date have been great, and I want you to know that you're very welcome to make more and to be a part of the team on this project. There's no obligation involved to provide any future work, just the opportunity to do so if you want.

All I ask is that you keep the release notes updated with any changes, and make a PR/issue for discussion of anything you'd like to have review or feedback on.

jonorossi commented 9 years ago

I've pushed #213 to fix the null vs empty array issue. I spent most the time trying to work out why Mono is missing System.Array.Empty<T>(), can't find a bug report for it.

I don't mind if you add me as a collaborator, I really haven't had any problem with you accepting pull requests, but I also don't mind doing some myself when I'm confident with them. Many thanks for starting this project. While I'm enjoying RethinkDB and it fits my project's needs I'm going to be working a lot more on rethinkdb-net.