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

Fix for #4479 REST filter #12641

Merged
merged 32 commits into from Oct 8, 2017
Merged

Conversation

klimov-paul
Copy link
Member

Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues #4479

Implemented REST filter.
Format inspired from the ones used by ElasticSearch and MongoDB.

@klimov-paul klimov-paul added this to the 2.0.x milestone Sep 30, 2016
@klimov-paul klimov-paul mentioned this pull request Sep 30, 2016
4 tasks
@klimov-paul
Copy link
Member Author

Added support for request via body content at IndexAction.
For the test you can use command like following:

curl -XGET http://domain.com/api/item?_format=json -H content-type:application/json -d '{"filter":{"id": 1}}'

@samdark
Copy link
Member

samdark commented Oct 4, 2016

Is there a reason to use $ in keys?

@klimov-paul
Copy link
Member Author

Is there a reason to use $ in keys?

This is a practice borrowed from ElasticSearch and MongoDB. In particular I can provide following reasons:

  • keywords with $ are unlikely to be missplaced for a search attribute or value: and, eq, lte and so on can be names of attribute or actual values.
  • keys with $ are easy to notice while reading the filter in JSON format
  • JSON documents can be automatically sorted by some reading tools (firebug console, for example), using $ ensures keywords will appear at the top.

@klimov-paul
Copy link
Member Author

Is there a reason to use $ in keys?

Also note that all operators are configurable: you can change thier keywords to whatever you like.

@samdark
Copy link
Member

samdark commented Oct 5, 2016

Yes, I've noticed that. I'm concerned about defaults.

@niksamokhvalov
Copy link

A good format. It seems to abandon the $ of the impossible, though the format and complicated. This symbol needs to clearly define the terms and and or.

Here's what I don't like: applications, simultaneously, using forms and REST API for the same model, will have to make several implementations of the filters (searchModel and dataFilter models). In fact, we want to implement the same behavior, but we still need to write it twice.

@klimov-paul
Copy link
Member Author

klimov-paul commented Oct 14, 2016

will have to make several implementations of the filters (searchModel and dataFilter models). In fact, we want to implement the same behavior, but we still need to write it twice.

You can reuse a search model created for the regular 'frontend' listing in REST even without this fix via IndexAction::prepareDataProvider:

class SearchModel extends \yii\base\Model
{
    // ...

    public function search($params, $formName = null)
    {
        $query = Item::find();

        // add conditions that should always apply here

        $dataProvider = new ActiveDataProvider([
            'query' => $query,
        ]);

        $this->load($params, $formName); // remember that you can pass `formName` directly to the `load()` method

        if (!$this->validate()) {
            $query->where('0=1');
            return $formName === '' ? $this : $dataProvider;
        }
    }

class ItemController extends \yii\rest\ActiveController
{

    // ...

    public function actions()
    {
        return [
            'index' => [
                'class' => 'yii\rest\IndexAction',
                'modelClass' => $this->modelClass,
                'prepareDataProvider' => function () {
                    $searchModel = new SearchModel();
                    return $searchModel->search(Yii::$app->request->queryParams, '');
                },
            ],
        ];
    }

With such code: search at the regular 'frontend' will be performed via following URL:

domain.com/items?ItemSearch[name]=foo&ItemSearch[status]=1

while for the REST - URL will be following:

domain.com/api/items?name=foo&status=1

There is no need to change something to implement simpliest search in the REST. I have alreay told this here:
#4479 (comment)

This PR introduces sophisticated search condition builder, which can be used for creating 'database-like' search interface.
It may be useful in case you are splitting your application into a mirco services, which interact with each other via REST.

@tunecino
Copy link
Contributor

@klimov-paul shouldn't that SearchModel replace the old one here ? also what do you think about adding an interface for it ? (see related discussion in #11470)

@klimov-paul
Copy link
Member Author

shouldn't that SearchModel replace the old one here ?

Unlikely, because it more complex and thus will confuse newcomers.
You can create your own Gii CRUD template, if you want to use it by default.

what do you think about adding an interface for it ?

I am unsure if search interface can be made generic enough. It still may need extra options.

@klimov-paul
Copy link
Member Author

Refactored:

  • Extracted DataFilter::$filterControls allowing setup of control keywords to be used in filter.
  • Added DataFilter::$attributeMap supporting table joins
  • Added I18N for error messages

public function setSearchModel($model)
{
if (is_object($model)) {
if (!$model instanceof Model && !$model instanceof \Closure) {
Copy link
Member

Choose a reason for hiding this comment

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

Could be moved to the condition above.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it could not: $model may be an array or string class name, which is a valid value and should not throw an exception.

Copy link
Member

Choose a reason for hiding this comment

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

$model may be an array

Then it will not be object and no exception will be thrown

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


foreach ($model->getValidators() as $validator) {
$type = null;
if ($validator instanceof BooleanValidator) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this part to a separate protected method to make it easier to adjust for custom validators.

Copy link
Member Author

Choose a reason for hiding this comment

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

Disagreed.
It is meant to override a whole detectSearchAttributeTypes() ot provide some sophosticated type detecttion mechanism. Particular implementation may not process validators at all.

Copy link
Member

Choose a reason for hiding this comment

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

Adding custom validator does not lead to any sophisticated type detection logic. It is still a map: class to type. I don't think that overriding the whole method (including logic) is a right way for filter configuration.

Moreover, current approach increases formal cyclomatic complexity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Repeat once more: method detectSearchAttributeTypes() as a whole determines the search attribute types. The actual algorithm of its detection may be different.
For example: you may want to get types from DB table schema, or may want to parse PHPDoc searching for type-hints.
It is only a single particular implementation, which relies on model validators.

If I add extra method for validators processing, it will become useless and never will be invoked in case detectSearchAttributeTypes() rewritten to use DB table schema.

I don't think that overriding the whole method (including logic) is a right way for filter configuration.

How is method override related to filter configuration? You can set $searchAttributeTypes via setSearchAttributeTypes() to whatever you like. Method detectSearchAttributeTypes() invoked only in case there are $searchAttributeTypes explicitly configured.

public $attributeMap = [];

/**
* @var array list of error messages responding to invalid filter structure, in format: `[errorKey => message]`.
Copy link
Member

Choose a reason for hiding this comment

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

array|\Closure

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@samdark
Copy link
Member

samdark commented Oct 1, 2017

Code and user API looks good to me. @klimov-paul should it be described in the guide?

@klimov-paul
Copy link
Member Author

should it be described in the guide?

Extra docs will not hurt. But I can not afford to waste any more time on this.
If you suppose guide section to be essential - someone else should take care of it.

@samdark
Copy link
Member

samdark commented Oct 5, 2017

OK.

@samdark samdark mentioned this pull request Oct 5, 2017
@samdark
Copy link
Member

samdark commented Oct 5, 2017

@yiisoft/reviewers need your help to:

  1. Ensure overall code is good. I think it is but I could be wrong.
  2. Ensure phpdoc is clear.
  3. Ensure English in phpdoc is OK.

Thanks.

Copy link
Member

@SilverFire SilverFire left a comment

Choose a reason for hiding this comment

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

Good otherwise

*
* @see DataFilter
*
* @since 2.0.10
Copy link
Member

Choose a reason for hiding this comment

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

2.0.13

@samdark samdark merged commit 744b87f into yiisoft:master Oct 8, 2017
@samdark
Copy link
Member

samdark commented Oct 8, 2017

Merged. Thanks for solving it.

@samdark
Copy link
Member

samdark commented Oct 8, 2017

Docs will be handled separately but for 2.0.13 as well.

@samdark samdark mentioned this pull request Oct 24, 2017
@klimov-paul klimov-paul deleted the 4479-rest-filter branch January 10, 2018 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants