U4-9291 - TypedSearch should have an overload specifying pager values, page size, page index, and with an out param to return the total number of results

Created by Shannon Deminick 15 Dec 2016, 06:39:45 Updated by Sebastiaan Janssen 07 Feb 2018, 11:52:48

Subtask of: U4-9609

1 Attachments

Comments

Tim Payne 02 Nov 2017, 12:05:04

I'll look at this, as part of UK Hack Fest.


Tim Payne 02 Nov 2017, 13:59:04

PR Submitted:

https://github.com/umbraco/Umbraco-CMS/pull/2286


Stephan 02 Nov 2017, 16:25:38

all good, merged


Shannon Deminick 02 Nov 2017, 23:29:42

Sorry not quite, paging with Examine and Lucene needs to be done in a specific way otherwise you'll be allocating every result to memory without realizing it. I've just blogged about this so you can borrow this code to update the implementation: http://bit.ly/2zaSee4

Cheers!


Tim Payne 07 Nov 2017, 09:22:59

Ahhh, wasn't aware of the fact it didn't support take properly, was going off another example. I'll sort that out ASAP and submit a new PR!

:)


Tim Payne 07 Nov 2017, 10:16:55

OK, I've had a look at this and modified the method for the TypedSearch method that you pass a search criteria to, as that accepts the max results value. HOWEVER, the method that just accepts the search text does not have a method that allows you to pass in the maxResults parameter to it.

How do you want me to proceed with this @Shandem? I'm guessing it's not trivial to update Examine in core? I'm happy to have a crack at submitting a PR for adding support for the maxResults in Examine if you want? What would be the process for then getting that into the Core to allow us to pass the maxResults to that method?

I MIGHT be able to fiddle the UmbracoExamine searcher to work without having to update the main Examine repo, but that'd be a bit of a hack I guess?


Shannon Deminick 08 Nov 2017, 02:40:32

We don't need to update anything in examine - it already supports what is required. The method that just accepts the 'text' parameter converts this to criteria under the covers, so we just need to convert this to criteria ourselves.


Sebastiaan Janssen 08 Nov 2017, 08:24:59

I've reverted this for now so we can put the proper fix in place.


Tim Payne 09 Nov 2017, 15:13:55

@shandem, are you talking about doing this in the UmbracoExamine Searcher class? So add an extra parameter to the Search(text) method (or an override) and convert to a criteria there and use the other method? I tried adding it all on the PublishedContentSearcher, but that wasn't possible because several of the methods I'd need (GetAllSearchFields for example) are internal to the searcher. Let me know and I'll sort and resubmit the PR.


Shannon Deminick 15 Dec 2017, 06:28:31

@AttackMonkey

I've created a PR for this here which used all of your old stuff and then i just cleaned some things up: https://github.com/umbraco/Umbraco-CMS/pull/2351

I'm using reflection to get at an underlying API to get all fields which is how Examine does this query normally. We can expose a method publicly in examine in the future but it's no big deal right now. I've also add checks for calling into these skip/take methods when calling from the non-skip take methods to ensure we check for the 0 and 0 values to use the native APIs.

It would be most appreciated if you could review and test these new methods :) Let us know how you go and we'll get it merged in.


Warren Buckley 02 Jan 2018, 10:39:11

Happy New Year @AttackMonkey Have you had a chance to look at @Shandem PR in regards to testing the new methods work as you might expect them to?


Warren Buckley 12 Jan 2018, 13:15:40

I get a YSOD thrown @Shandem - see attached screenshot, when using the following snippet put directly into a razor view/template

    int totalRecords = 0;
    var results = Umbraco.ContentQuery.TypedSearch(1, 100, out totalRecords, "home");

    foreach (var result in results)
    {
        var x = result.Name;
        
    }


Shannon Deminick 15 Jan 2018, 05:13:01

@warren.buckley oops, have pushed a fix. The compiler wasn't complaining about the mistake because there's some dynamics used.

I've re-tested with this and it's working:

@{
    int totalRecords = 0;
    var results = Umbraco.ContentQuery.TypedSearch(1, 100, out totalRecords, "test");
}
    
<ul>
@foreach (var result in results)
{
    <li>@result.GetPropertyValue("examineScore") - @result.Name (@result.Id)</li>
}
</ul>


Warren Buckley 15 Jan 2018, 09:40:19

Re-tested and all works fine now. Merged in


Tim Payne 07 Feb 2018, 10:30:16

Gah, sorry guys, I didn't get the alerts for this and have only just seen it as part of the release notes and I didn't get chance to test!


Sebastiaan Janssen 07 Feb 2018, 11:52:48

@AttackMonkey No worries, it happens! We got it all tested out so you just need to enjoy the updates and feel great about getting it all kickstarted! #h5yr!


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions:

Due in version: 7.8.0

Sprint: Sprint 76

Story Points: 1

Cycle: