Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IEnumerable<TEntity> support #15

Merged
merged 3 commits into from
Sep 6, 2018

Conversation

LonghronShen
Copy link
Contributor

Since UpsertRange now just support TEntity[]. I'm trying to add overloads which consumes IEnumerable, but discussions are needed since this may broken old APIs.

@artiomchi
Copy link
Owner

Oooh, that's a great idea! I don't know why I haven't thought of adding an IEnumerable extension as well - it's just common sense :)

Sorry for taking the liberty to make changes - I have a bit of an OCD with formatting and extra line breaks and the like :)

Also, I noticed you changed the methods to use IEnumerable, but internally you ended up indiscriminately converting it to a list anyway.

I think it would make a lot more sense to use ICollection internally. It can accept both arrays and lists natively, without conversion. On the extension method, I just check if the IEnumerable that's passed in is compatible, and just take it as is. Otherwise, I convert it to a list.

Another reason I think this approach is superior is ensures there will be no issues with lazy loaded sets, since we either ensure that the collection is already loaded into memory before it's passed to us, or we convert it to an array to do that.

Regarding backwards compatibility, all these changes are done in mostly new classes that haven't been released yet, so there are no issues there. And we're just adding new extension methods, so there should be no problems there either.

What do you think? Do you have anything to add to this before it's merged?

@LonghronShen
Copy link
Contributor Author

LonghronShen commented Sep 6, 2018

Oh, thank you for your notice! I think ICollection<T> is a better choice, or IReadOnlyCollection<T> is better?
Aother issue is about the UpdateColumns function. Why not have a Expression<Func<TEntity, TEntity, TEntity>> updater since we want to use the new data from the given range. What do you think about? This feature is not related to this PR, so you can merge this one first.

@artiomchi
Copy link
Owner

IReadOnlyCollection is actually a separate interface that's implemented by less classes than ICollection, so it wouldn't work the same way.

Regarding UpdateColumns - yeah.. I was thinking of adding that as an override, but haven't implemented it yet. Not enough hours in the day :)

I'm not very happy with the method names either.. I'm considering renaming them before the release.. maybe from .On(...).UpdateColumns(...) to .On(...).WhenUpdate(...) or WhenMatched?

Or better yet: .MatchOn(...).OnUpdate(...). What do you think?

@LonghronShen
Copy link
Contributor Author

LonghronShen commented Sep 6, 2018

Oh, right, ICollection<T> is better.
Thinking of the PostgresSQL upsert syntax:

INSERT INTO table_name(column_list) VALUES(value_list)
ON CONFLICT target action;

I want to write:

dbContext.Items.UpsertRange(newItems)
    .OnConflict(x => x.IdentitcalField)
    .DoUpdate((matched, newItem) => new Item()
    {
        Field = newItem.Field
    });
dbContext.Items.UpsertRange(newItems)
    .OnConflict(x => x.IdentitcalField)
    .DoNothing();

What about let the Run function go away?

@artiomchi
Copy link
Owner

Umm. Actually, yeah.. that sounds better.. although I'm not too keen on the word Conflict..OnMatch, maybe, since it's more generic and pretty accurate in what it means?

Regarding .Run - there's a couple reasons:

  • To be able to have a synchronous and an asynchronous version of the call without duplicating the Update call method code
  • To allow calling Upsert without specifying the update columns (which will currently update all other columns)

@LonghronShen
Copy link
Contributor Author

I agree with you about that two points. But it's semetically better to duplicate the update calls, like this:

dbContext.Items.UpsertRange(newItems)
    .OnConflict(x => x.IdentitcalField)
    .DoUpdate((matched, newItem) => new Item()
    {
        Field = newItem.Field,
		Visit = matched.Visit + 1
    });
dbContext.Items.UpsertRange(newItems)
    .OnConflict(x => x.IdentitcalField)
    .DoUpdateAsync((matched, newItem) => new Item()
    {
        Field = newItem.Field,
		Visit = matched.Visit + 1
    });
dbContext.Items.UpsertRange(newItems)
    .OnConflict(x => x.IdentitcalField)
    .DoUpdate();
dbContext.Items.UpsertRange(newItems)
    .OnConflict(x => x.IdentitcalField)
    .DoUpdateAsync();
dbContext.Items.UpsertRange(newItems)
    .OnConflict(x => x.IdentitcalField)
    .DoNothing();
dbContext.Items.UpsertRange(newItems)
    .OnConflict(x => x.IdentitcalField)
    .DoNothingAsync();

// Infer the match fields to detected primary keys or alternative keys
dbContext.Items.UpsertRange(newItems);
dbContext.Items.UpsertRangeAsync(newItems);

Of course, this would increase our work. What do you think about?
By the way, I think this PR can be merged first, and I think we can talk about the design in an issue.

@artiomchi
Copy link
Owner

Yeah, I was thinking the same (about moving this to a new issue) :)

@artiomchi artiomchi merged commit d35760c into artiomchi:develop Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants