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

Updated dependencies, rewritten code utilizing mongodb::IndexModel #98

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

styrowolf
Copy link

I have updated dependencies and rewritten the code to utilize mongodb::IndexModel instead of wither::IndexModel, which is just a placeholder. The tests have been updated and is passing too.

Copy link
Owner

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this work! Just a few items that caught my eye. Keep me posted.

tokens.extend(quote!(wither::IndexModel::new(#keys, #options)));
match &self.options {
Some(opts) => {
tokens.extend(quote!(wither::IndexModel::builder().keys(#keys).options(wither::mongodb::bson::from_document::<wither::mongodb::options::IndexOptions>(#opts).unwrap()).build()));
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer not to use unwrap at all. No way to just use a doc!{} here in the .options(...) bit? If not, then we should likely use an intermediate type to hide this complexity, but which will results in the correct MongoDB IndexModel. Thoughts?

Copy link
Author

@styrowolf styrowolf Jan 19, 2023

Choose a reason for hiding this comment

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

.options(...) takes IndexOptions as a parameter, so I don't think there is a straightforward way to use a doc!{} there.

Preferably, we should have the macro abort with an error if deserializing the doc!{} to IndexOptions does not work. However, I cannot think of a way (not very knowledgeable about macros) to do that, as that would require executing the from_document(...) function at compile-time.

Another solution would be to change the #options tokens to some expression that returns a IndexOptions instead of doc!{}. But that means changing the public API. For comparison,

The old way of defining options is:

// Define a model. Simple as deriving a few traits.
#[derive(Debug, Model, Serialize, Deserialize)]
#[model(index(keys = r#"doc!{"email": 1}"#, options = r#"doc!{"unique": true}"#))]
struct User {
    /// The ID of the model.
    #[serde(rename = "_id", skip_serializing_if = "Option::is_none")]
    pub id: Option<ObjectId>,
    /// The user's email address.
    pub email: String,
}

The new way would be:

// Define a model. Simple as deriving a few traits.
#[derive(Debug, Model, Serialize, Deserialize)]
#[model(index(keys = r#"doc!{"email": 1}"#, options = r#"IndexOptions::builder().unique(true).build()"#))]
struct User {
    /// The ID of the model.
    #[serde(rename = "_id", skip_serializing_if = "Option::is_none")]
    pub id: Option<ObjectId>,
    /// The user's email address.
    pub email: String,
}

Please let me know your thoughts on this.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh nice ... that is not a bad idea at all. Path of least resistance, perhaps. I think that is worth experimentation. As long as none of the tests fail and can be effectively adapted, then I would say that we have a clean win, and it will effortlessly stay up-to-date with upstream changes in the driver.

Good call. Keep me posted! Sorry for the late response :)

wither/src/model.rs Outdated Show resolved Hide resolved
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