krzysztof-gzocha/searcher

Searcher core

searcher's Issues

ResultCollection should not throw exceptions when results are null

ResultCollection should not accept such results.
It should work properly at all cost.
If wrong type is given just do not accept it and use empty array and results

Integration with external libs should probably sit in external repositories.

If I understood it properly this library is supposed to be datasource agnostic - it can work with Doctrine ORM but it might also work with Elasticsearch if you provide an appropriate adapter. Yet in this repository we can find files like searcher/src/KGzocha/Searcher/FilterImposer/AbstractQueryBuilderFilterImposer.php with direct dependency on Doctrine ORM.

You could solve it the way Gaufrette team did it. You keep only interfaces/abstracts here and create separate repositories with Doctrine ORM implementation, Doctrine ODM implementation, Elastic Search implementation and so on. Your composer file can suggest those dependencies without requiring those.

Add AlwaysAppliedCriteria which will just return true in shouldBeApplied()

In this way it will be possible to add CriteriaBuilders which could add constant values to the query, like for example aggregation into ElasticSearch query.

To do:

• Implement the criteria
• Add them to the docs

Improve Searcher core class

Have a look at https://github.com/krzysztof-gzocha/searcher/blob/master/src/KGzocha/Searcher/Searcher.php#L60

This can be improved. Result of it can be calculated outside of foreach and passed to internal function. There is no need to call it on each foreach

Should there be method AbstractQueryBuilderFilterImposer::join() ?

Method join() in class AbstractQueryBuilderFilterImposer is checking if there already is such join in passed (as a parameter) QueryBuilder and if it is, it will do nothing - in other case it will create new join.
Is it correct way? Is there better way/implementation to check it?

Situation
Two different FilterImposers are adding conditions to entity that is accessible via some relationship, so it needs to perform JOIN operation. If both would just do joining with the same alias, then Doctrine would throw an exception because there already is such join. In case both of them would just do the join with different (unique) alias, then we would have two different joins which is a mistake, right?

Example
Searching for entity Person that lives in some country for longer than some period. There is entity Address. There is one-to-many relationship between Person and Address.
Code in CountryFilterImposer:

$queryBuilder->join('p.adresses', 'address')->andWhere(/** some condition */);

Code in TimePeriodFilterImposer:

$queryBuilder->join('p.adresses', 'address')->andWhere(/** some condition */);

Both needs to call join() somehow, because they are not aware of each other and can be called separately.

Workaround
Create all possible joins in QueryBuilder before passing it to SearchingContext

Add Doctrine SearchingContext adapter for enabling caching

Idea

For now SearchingContexts are working pretty well, but if user wants to make use of Doctrine's caching mechanism then he needs to implement it on his own (which is not that hard). Target of this issue is to create an adapter/wrapper/anything that will make queriers inside SearchingContexts cacheable.

Create AbstractCriteriaBuilder for ElasticSearch searching context

Create AbstractCriteriaBuilder for ElasticSearch searching context, so users won't need to do it them selfs and each CriteriaBuilder that should work with ElasticSearch query could just extend this abstract builder.

Elastic\AbstractCriteriaBuilder should only implement method supportsSearchingContext()

Implement and use CellCollection instead of an array in chain-searching

Current implementation

Right now chain searching is requiring at least 2 cells to be provided as array to ChainSearch class and this class is responsible for validation of this array, but it should not care about it that much.

Ideas

• Create CellCollection which will return cells only if all of it's elements are valid. Collection should extend AbstractCollection and should have it's own interface, which will extends \Countable and \IteratorAggregate
• Remove name from CellInterface and have NamedCellCollection which will extend regular CellCollection (with shared interface) just like we have it with Criteria for two thing: consistency and cell does not need to be aware of it's name, but name might be helpful to extract single cell from the collection (or to build service for some frameworks).

Important

• Think if it's possible to keep backward compatibility
• Solution should be fully covered with unit tests
• Humbug's MSI factor should show 100%
• Scrutinizer's score should not be decreased

Integrate with POMM Project

Integrate Searcher (and SearcherBundle if needed) with POMM project.

Links to start with:

• http://www.pomm-project.org/
• https://github.com/pomm-project/ModelManager

How to use it?

There is no usage example here. We can find one on Symfony Bundle page but Symfony integration shouldn't be more important than the library.

Make use of .gitattributes

Idea

Make use of .gitattributes as described here to exclude not required files when running composer isntall --prefer-dist.

Better documentation

Requirements

• Fix README.md file to correspond to proper naming conventions used in the code
• Fix PHPDocs across the project. Add proper comments.
• Double check for misleading variable names
• Write better documentation (maybe using GitHub pages or readthedocs) that will show exactly what service is responsible for. (In progress in #55)

Additional

Idea

Create example application for searching as a proof of concept (maybe hosted on Heroku).
(Help wanted)

Searcher class improvements

Ideas

• Move Searcher class to \KGzocha\Searcher
• Remove wrapping results automatically in ResultCollection and add WrappedSearcher, because Searcher might be use to fetch number, not array of results, so user needs to have a possibility to change this behaviour

Create adapters for SearchingContexts to allow pagination.

Idea

The idea is to create adapter (or any other easy method) to allow end user to implement pagination of results.

Paginators:

• https://packagist.org/packages/pagerfanta/pagerfanta
• https://packagist.org/packages/knplabs/knp-paginator-bundle will be done in searcher-bundle repo
• https://packagist.org/packages/illuminate/pagination
• https://packagist.org/packages/zendframework/zend-paginator

Other approach

User can still implement his own FilterImposer and use existing OrderFilterModel and PaginationFilterModel to each the same goal.

Implement exception structure

Implement core exception like SearcherException, which can be later extended for example in bridge between searcher and symfony. Name of the exception itself should help end user to find the problem.

Integrate with ElasticSearch

Create ElasticaSearchingContext that can be used to integrate searcher with ElasticSearch.

Link to start with: https://packagist.org/packages/ruflin/elastica.

Implement searching context for scrolling in Elastica

Issue

Right now we only have QuerySearchingContext for Elastica which is using \Elastica\Search::search() method, which has limitation of 10 000 results and developer needs to add special scroll flags in order to fetch all the results at once.

Idea

There are two possible ways:

1. Implement ScrollQuerySearchingContext which will extend QuerySearchingContext, but will use \Elastica\Search::scroll() method instead of search()
2. Refactor existing QuerySearchingContext, so it can accept options, which will be passed to elastica before searching

Describe example SearchingContext's service declaration for Symfony2

Idea

• Add to the documentation working example of how to construct most common SearchingContext in krzysztof-gzocha/searcher-bundle
• Add to the documentation working example of how to construct KnpPaginatorAdapter in for searcher integration with Symfony

Add shared pre-build capabilities to CriteriaBuilders

Idea

In some cases it might be useful to create two separate CriteriaBuilders, that will share some common logic in order to build JOIN's for example. Even if the logic will be extracted to 3rd service it will still be run twice if both CriteriaBuilders will run.

Point of this issue would be to provide solution to somehow map what should be run before those CriteriaBuilders and if it was run before, do not allow to call it again.

In this way both CriteriaBuilders will be allowed to specify some service which will do JOINs, both can be called and logic that will actually add those JOINs will be called just once.

Requirement

This can NOT break backward capabilities.

Current work around

Current implementation is allowing developer to perform JOIN only if there is no such join in the Query, but this works only for Doctrine ORM. This code can be found in AbstractORMCriteriaBuilder, but it's not easy to use and will work only with Doctrine ORM. The code is as well marked as one of the worst methods in whole library according to Scrutinizer CI

Problem right now in pseudo-code

CriteriaBuilder1:

buildCriteria($criteria, $searchingContext)
{
   $searchingContext
      ->getQueryBuilder()
      ->join('p.user', 'u')          // this is the same
      ->where('u.name = :name')
      ->setParameter(':name', $criteria->getText());
}

CriteriaBuilder2:

buildCriteria($criteria, $searchingContext)
{
   $searchingContext
      ->getQueryBuilder()
      ->join('p.user', 'u')              // this is the same
      ->where('u.age = :age')     // this is changed
      ->setParameter(':age', $criteria->getNumber());
}

So now in current implementation if both CriteriaBuilders will be called then join will be added 2 times, which shouldn't happen. That's why there is AbstractORMCriteriaBuilder::join, so CriteriaBuilders can be changed and look like this:

CriteriaBuilder1:

// Needs to extend AbstractORMCriteriaBuilder
buildCriteria($criteria, $searchingContext)
{
   $this
      ->join($searchingContext->getQueryBuilder(), 'p.user', 'u', Join::INNER_JOIN)
      ->where('u.name = :name')
      ->setParameter(':name', $criteria->getText());
}

CriteriaBuilder2:

// Needs to extend AbstractORMCriteriaBuilder
buildCriteria($criteria, $searchingContext)
{
    $this
      ->join($searchingContext->getQueryBuilder(), 'p.user', 'u', Join::INNER_JOIN)
      ->where('u.age = :age')
      ->setParameter(':age', $criteria->getNumber());
}

Those joins can be for example moved to separate classes

Criteria constructors

Is there a reason the default Criteria don't have a constructor to set the value?

The Complete example does have one and use it (new HeightCriteria(200)), which seems to make it more easy (instead of (new HeightCriteria)->setHeight(200);)

I can understand it might not be ideal for DI (with the Symfony bundle or something), but a default with null shouldn't hurt?

(Just checking if this is deliberate or not)

Improve tests according to Humbug log

In this moment humbug is showing ~70% of MSI, so it means not all tests are highest quality.
This issue is about increase this number to 90% at least (aiming for 100%)

70 mutations were generated:
      50 mutants were killed
       9 mutants were not covered by tests
      11 covered mutants were not detected
       0 fatal errors were encountered
       0 time outs were encountered

Metrics:
    Mutation Score Indicator (MSI): 71%
    Mutation Code Coverage: 87%
    Covered Code MSI: 82%

Migrate completely to PHP 7

Issue

Migrate whole library to PHP 7.

To do

• Add declare(strict_types=1) in all .php files
• Add strong type hinting
• Make use of \Generator when iterating though something.
• Require only PHP 7 or higher in composer.json
• Upgrade dev dependencies

Add possibility to pass all CriteriaCollection across all Transformers inside ChainSearcher

Idea

Right now transformers have access only to the results from previous search, but it some scenarios it might be useful to pass also all CriteriaCollection that were used in previous cells.

[Insight] User specific files should not appear in .gitignore - in .gitignore, line 3

in .gitignore, line 3

.idea/ is user-specific and should not appear in a project .gitignore. Consider adding it to the user global .gitignore instead.

bin/
vendor/
.idea/
coverage/
coverage.clover
ocular.phar
composer.lock

---

Posted from SensioLabsInsight

Rename things to make more sense

I think Imposer and Model names are too generic. Maybe let's use Criteria and CriteriaBuilder?

Ex:

interface SearchCriteriaInterface
{

}

class BedroomCriteria implements SearchCriteriaInterface
{
    public function getMin()
    {

    }

    public function getMax()
    {

    }
}

interface SearchCriteriaBuilderInterface
{
    public function build(SearchCriteriaInterface $criteria);
}

class BedroomCriteriaBuilder implements SearchCriteriaBuilderInterface
{
    /**
     * @var
     */
    protected $criteria;

    public function build(SearchCriteriaInterface $criteria)
    {
        $qb = $this->qb->where('bedroom', $criteria->getMin());
    }

    public function allowsCriteria(SearchCriteriaInterface $criteria)
    {
        return $criteria instanceof BedroomCriteria;
    }
}

Create ResultsCollection which will be returned from every SearchingContext

Problem

Currently each SearchingContext may return different things (arrays, collections, etc).

Idea

The idea is to have single, common result object which will encapsulate the results, so end user will never need to worry about what SearchingContext is used.

Idea: SearcherChain

Idea

Create a mechanism that will allow to use results from first sub-searching to be used as (additional) criteria in next sub-searching, so together it will create one single search. Each sub-search could have different searching context, so it would allow developer to perform first search on MySQL, then use results to search for MongoDB and even use results from MongoDB to search for files on S3 bucket.

Real world example

ElasticSearch searching for documents with theirs statistics calculated by aggregations.
Since aggregations can not be (yet) paginated in ElasticSearch it self you would need to fetch all buckets on your own and count them. Solution using SearcherChain would be:

1. Fetch results in first sub-search,
2. Map results to (additional) criteria - for example extract just IDs,
3. Pass original and additional criteria and results to next sub-search and fetch statistics,
4. Eventually compose results from first search with statistics into single collection

Must have

1. Unlimited number of sub-searches,
2. Each sub-search can have different searching context,
3. Interface for service responsible for mapping first results as criteria for another sub-search.

Nice to have

1. Mechanism for marking sub-search to be skipped, so whole process will go from first sub-search directly to third one.

Status

• Code introduced in #67
• Full usage documentation #68

Write missing unit tests

Classes that requires unit tests:

• ODMBuilderSearchingContext
• CriteriaBuilder/Doctrine/*
• CriteriaBuilder/Elastica/*

Implement abstract Collection

Idea

Implement abstract collection from which every other collection can extend.

Requirements

Implements \Countable, \IteratorAggregrate

Add default FilterModels

For example:

• IntegerFilterModel
• NumberFilterModel
• RangeFilterModel
• CoordinatesFilterModel
• BooleanFilterModel

and think about additional ones