Skip to content
This repository was archived by the owner on Feb 22, 2020. It is now read-only.

Added Support for 2.0 async driver #24

Open
wants to merge 2 commits into
base: v2
Choose a base branch
from
Open

Conversation

dhanilan
Copy link

@dhanilan dhanilan commented Jan 4, 2016

Added Support for 2.0 async driver .I Will write test cases in the next commit. Any ideas for removing UpdateDefinition class usage in Update methods are welcome.

@RobThree
Copy link
Owner

RobThree commented Jan 4, 2016

Cool, cool. I'll have a look later this week. Thanks! 👍

@tegeek
Copy link
Collaborator

tegeek commented Jan 4, 2016

The PR looks OK to me. Regarding the UpdateAsyc, I think it'll be good idea to have another version of Update with T entity. So might be UpdateEntityAsync(T entity)?

I'll approve PR once I see some tests. Good work.

@RobThree
Copy link
Owner

RobThree commented Jan 4, 2016

Code looks okay indeed (haven't tested it yet though) but the docblocks might need a little brush-ups to be in line (same wording, style) with the rest of the code, I'll get back to it as soon as I can.

@dhanilan
Copy link
Author

dhanilan commented Jan 4, 2016

@tegeek we need to pass updateDefinition as a parameter UpdateEntityAsync(T entity, UpdateDefinition updateDefinition); . Is that fine?

@RobThree
Copy link
Owner

RobThree commented Jan 4, 2016

we need to pass updateDefinition as a parameter UpdateEntityAsync(T entity, UpdateDefinition updateDefinition);

Preferrably not because it breaks the abstraction / leaks implementation specifics. MongoRepository should work this out internally (if at all required; I'd rather update the entire entity). I think MongoRepository should always work with (or at least default to) entities as a whole. Which also brings me to #23; I don't think that's a good idea unless we provide specific methods for it.

@dhanilan
Copy link
Author

dhanilan commented Jan 4, 2016

Okay thanks @RobThree . I will use ReplaceOneAsync for Update. (Update entire document).

@dhanilan
Copy link
Author

dhanilan commented Jan 4, 2016

Committed the same . Please review them .
Have added the test case also.
Thanks

public virtual async Task<T> UpdateAsync(T entity)
{
if (entity.Id == null)
await this.AddAsync(entity);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't possible to use UpdateOneAsync here and sending Update options upsert = true. I've not looked into the new API but can't we build UpdateDefintion from entity (which means update everything) as default?

@dhanilan
Copy link
Author

dhanilan commented Jan 4, 2016

@tegeek won't that be complicated to build UpdateDefinition for the generic class T? May be loop through the properties of the class?please advice.

@dhanilan
Copy link
Author

dhanilan commented Jan 4, 2016

Something like this?

       UpdateDefinition<T> updateDefinition= Builders<T>.Update.Set(x=>x.Id, entity.Id);
        var props = typeof(T).GetType().GetProperties();
        foreach (var propertyInfo in props)
        {
            if (propertyInfo.CanRead)
                updateDefinition.Set(propertyInfo.Name ,propertyInfo.GetValue(entity,null));
        }
        this.collection.UpdateMany(GetIDFilter(entity.Id), updateDefinition);

@RobThree
Copy link
Owner

RobThree commented Jan 4, 2016

To be honest: that seems like a horrible hack. I can't imagine we'd have to iterate over all properties to create an updatedefinition? I'm not (yet) very familiar with the 2.0+ driver but I'm pretty sure there must be a (much) better way. The sync (note: not async) methods can do fine without, I can't imagine the async variants would require such different approach.

Again: I'll take a look / give it a try a.s.a.p.

@dhanilan
Copy link
Author

dhanilan commented Jan 5, 2016

Yea. I never wanted to do that way. Meanwhile shall update you if I found better approach.
Thanks

@brgrz
Copy link

brgrz commented May 19, 2016

When will this merge? just started to work on implementing async then something told me to go check PRs and sure enough here it is.

@dhanilan Why are the GetByID async versions missing?
@RobThree Shouldn't we be using SingleOrDefault() with GetByID()
@RobThree @dhanilan @tegeek The Update() doing Add() if Entity hasn't got an ID violates SRP for update. Saving new entity through Update() should throw an exception.

Please respond.

@matt-lethargic
Copy link

As above, it's been months now, is this still maintained?

@tegeek
Copy link
Collaborator

tegeek commented Feb 2, 2017

MongoRepository is still maintained but old mongo c# driver. The version 2 of driver has different things and it seems none of us has used/explored how to fit those things in MongoRepository.

@f12358
Copy link

f12358 commented Feb 2, 2017 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants