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

Allow calling SourceInclude without manually specifiying all properties #1737

Closed
jeroenheijmans opened this issue Jan 19, 2016 · 3 comments
Closed

Comments

@jeroenheijmans
Copy link

I've asked a question on Stack Overflow, because I wanted to use SourceInclude to make Nest (1.x) not retrieve all document fields from Elasticsearch.

To reiterate the idea I had, consider the following code:

[ElasticType(Name = "productDoc")]
public class Product
{
    [ElasticProperty(Name = "doc_id")]
    public string Id { get; set; }

    [ElasticProperty(Name = "fullname")]
    public string Name { get; set; }

    [ElasticProperty(Name = "desc")]
    public string Description { get; set; }

    // Et cetera...
}

public class SearchRepo
{
    public Product RetrieveProduct(string id)
    {
        IElasticClient client = new ElasticClient();

        var getResponse = client.Get<Product>(p => p
            .Id(id)
            .SourceInclude(
                i => i.Id,
                i => i.Name,
                i => i.Description
                // Et cetera...
            )
        );

        return getResponse.Source;
    }
}

What realistically would happen next (e.g. in a few weeks), is this:

  • Someone wants to retrieve one more property from Elasticsearch, e.g. ProductNumber;
  • This person adds this property to Product with a proper ElasticProperty;
  • This person then runs the Get query, but to his/her surprise the property remains empty;

The problem is that this person (probably me) will have to remember to add that property to the SourceInclude call. And programmers are pretty bad at remembering to do things that are not automated...

The thing is, my code above is not DRY: I'm specifying twice which properties I want to be mapped. Once in the Product, and once in the SourceInclude call. The first one is the logical place (e.g. if I do a MultiGet somewhere with Product I want to reuse the explicit mappings for SourceInclude) for me.

I'm looking for guidance. Am I actually formulating a proper feature request? Or would that feature be bloat for Nest and should I roll my own "typedPathLookups" method? Or am I missing an obvious better solution?

@russcam
Copy link
Contributor

russcam commented Jan 19, 2016

Hey @jeroenheijmans, some kind of convention sounds like it would be interesting but it feels specific to your particular use case; NEST tries to stay close to the Elasticsearch API and expose it in .NET using .NET idioms (well, C#!) without unduly layering on top so I don't think this belongs in NEST.

Maybe an approach where properties of the POCO that should be returned in GET requests are decorated with a specific attribute and write an extension method to GetDescriptor<T> that iterates over the properties, passing each one that has the attribute applied into the .SourceInclude() method? Since the attributed properties are not going to change at runtime, you would only need to iterate once and cache the collection.

@russcam russcam added the Vote label Jan 19, 2016
@Mpdreamz
Copy link
Member

@jeroenheijmans zooming back out to your initial comment:

because I wanted to use SourceInclude to make Nest (1.x) not retrieve all document fields from Elasticsearch.

Elasticsearch returns documents from a single_source field which is a single byte copy and actually much faster then doing source includes which has to parse each _source and write a modified json on the response.

This is very much unlike SQL where selecting the minimum number of columns is good practice.

If you do want to go with SourceInclude(...) and make sure that stays in sync I would refactor it to a call to a single static method that will e.g SourceInclude(PersonSourceIncludes)

@jeroenheijmans
Copy link
Author

Thank you @russcam and @Mpdreamz for quick and insightful response.

I understand now that (and why) if I want to DRY out speccing which fields are SourceIncluded I need to write a utility method for that myself.

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

No branches or pull requests

3 participants