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 an Identifiable trait for DocumentTypes #301

Closed
KodrAus opened this issue Jan 23, 2018 · 3 comments
Closed

Add an Identifiable trait for DocumentTypes #301

KodrAus opened this issue Jan 23, 2018 · 3 comments

Comments

@KodrAus
Copy link
Member

KodrAus commented Jan 23, 2018

I find myself passing along a document with its id a lot and feel like we should do a better job of making this ergonomic. It could be as simple as collecting an id method on the DocumentType trait, but I'd like to work out any design implications of dropping everything on the one trait.

It could look like:

trait DocumentType {
    fn id(&self) -> Option<Cow<str>> {
        None
    }
}

So you could supply an id when deriving DocumentType using an attribute:

#[derive(ElasticType)]
#[elastic(id = "self.id()")]
struct MyType { ... }

impl MyType {
    fn id(&self) -> &str {
        self.id
    }
}

Which generates a DocumentType impl like:

fn id(&self) -> Option<Cow<str>> {
    Some(id_expr)
}

For any document_ requests, we can then infer the id:

fn document_update<TDoc>(index: Index<'static>, doc: TDoc) {
    let ty = TDoc::name();
    let id = doc.id();

    UpdateRequest { index, ty, id }
}

The problem with this is that if doc.id() is None, then we need to figure out an id for the request. This is where a separate trait with stricter requirements might come in handy:

trait Identifiable {
    fn id(&self) -> Cow<str>;
}

With the same set of attributes we derive DocumentType + Identifiable, and requests like document_update will require DocumentType + Identifiable. This would be a bigger breaking change, but means we could have DocumentTypes without necessarily being Identifiable. We could use a combination of the two traits, so we might get an id off DocumentType but must get an id off Identifiable.

@KodrAus
Copy link
Member Author

KodrAus commented Jan 23, 2018

We could also add a #[elastic(id)] attribute to a field on the document struct to make it act as an id (conflicts with #[elastic(id = "")] on the struct itself.

The thing I don't like is requiring more work than just #[derive(ElasticType)] before you can start indexing and updating documents, or effectively the same problem, but instead of failing to compile you just get weird results at runtime.

@KodrAus
Copy link
Member Author

KodrAus commented Jan 23, 2018

We could check the custom derive for an #[id] somewhere and panic with a useful message if it's not present? And add a generate_id attribute or something to generate an id if the doc simply doesn't have one.

@KodrAus
Copy link
Member Author

KodrAus commented May 6, 2018

Actually, just having DocumentType::id() -> Option<Cow<str>> and Identifiable::id() -> Cow<str>, and deriving the Identifiable trait when #[elastic(id)] is present should be enough here.

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

No branches or pull requests

1 participant