-
Notifications
You must be signed in to change notification settings - Fork 599
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 new features to MongoDbStorage provider #613
Add new features to MongoDbStorage provider #613
Conversation
…ompletely opt-in for new features
Getting the security bits on, will try to poke at this during the week - on first pass I see some extra .ToList() and not sure where the Linq usage is - will grab and poke at those. |
Thanks! |
Bump @NickCraver . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks mostly good, but the drop of data gives me a lot of pause here. IMO, we should error if the index already exists (and doesn't match what we want?) or just not drop at all and add a method for a user to explicitly do this.
Having a drop in the main path is asking for data loss/trouble and that's the only change I'd like to see here: removing that from the critical path unless explicitly and knowingly opted in to by the user. Thoughts on how to better make that crystal clear?
Along the same lines - do we need to error or drop when the index has what we need? Or can we check that and carry on?
if (indexNames.Contains(indexName)) | ||
{ | ||
indexManager.DropOne(indexName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not expect a method like this (regardless of name) to do a drop. Instead, probably best to have an option for "drop existing" that's very explicitly opt-in. The current overall path (callers to this aren't clear) could unexpectedly lose data for users.
This makes data loss opt-in rather than automatic when options change :)
Looks like the message changed over versions, so use the better method anyway.
AppVeyor is limited to MongoDB 4.0 from 5 years ago which behaves differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current looks good - thanks!!
* New features: * Decimal fields can now be serialized as `NumberDecimal`s instead of raw strings. * Indexes can be automatically created instead of having to manually call the `WithIndexCreation` method. * MiniProfiler sessions can be configured to be automatically expired (deleted) after a certain time period has elapsed. * The `MongoDbStorageOptions` class has been added to allow for setting the above options. * MongoDB C# driver has been updated to the latest version, and obsoleted code elements updated accordingly. * All new features are opt-in; full backwards-compatibility (including binary) is retained. Co-authored-by: Ian Kemp <ian.kemp@capitalontap.com> Co-authored-by: Nick Craver <nrcraver@gmail.com>
NumberDecimal
s instead of raw strings.WithIndexCreation
method.MongoDbStorageOptions
class has been added to allow for setting the above options.