? ? ? ? Pending

User tests: Successful: Unsuccessful:

avatar pjdevries
pjdevries
22 May 2021

Pull Request for Issue #34100

Summary of Changes

Allow filtering of data sets by passing filter[name] parameters in the request query, similar to the page[] parameters to set offset and limit. Parameters passed in this manner are set in the model state before the query is executed. This allows for simple, straight forward filtering on parameters accessed by any specific ListModel::getListQuery() implementation.

Testing Instructions

Call any known list endpoint with an appropriate filter parameter. For example:

{{base_path}}/api/index.php/v1/content/article?filter[category_id]=10
{{base_path}}/api/index.php/v1/content/categories?filter[extension]=com_content
etc.

Documentation Changes Required

yes

avatar pjdevries pjdevries - open - 22 May 2021
avatar pjdevries pjdevries - change - 22 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2021
Category Unit Tests Repository Administration
avatar joomdonation
joomdonation - comment - 23 May 2021

I think it is not right to set model states in view class like in this PR. If we want to support that, the right place is in displayList method of controller, see https://github.com/joomla/joomla-cms/blob/4.0-dev/api/components/com_content/src/Controller/ArticlesController.php#L48-L79 for an example. Maybe instead of add everything from filter input to model state like the current code, we should allows defining the states and it's data type.

There should be some kind of filter applied to data instead of use raw input data like in the current code

I don't have experience with our API yet. Maybe @wilsonge can give some feedback here?

avatar pjdevries
pjdevries - comment - 23 May 2021

It was my intention to provide a simple, straight forward solution to make webservices more usable. It can always be improved later.

EDIT:
Forget what I said :) I guess @joomdonation is right and the displayList method of the controller is the better place. The problem with the current implementation, appears to be consistency though. If we look at Joomla\Component\Content\Api\Controller\ArticlesController::displayList() and Joomla\Component\Categories\Api\Controller\CategoriesController::displayList(), we see different ways of obtaining filter parameters from the request.

How about moving my code from Joomla\CMS\MVC\View\JsonApiView::displayList() to Joomla\CMS\MVC\Controller\ApiController::displayList()? It would provide consistency and at the same time allow for filtering on all filter parameters implmented in the model, without having to implement each and every of those parameter in all the models specifically.

avatar pjdevries
pjdevries - comment - 23 May 2021

@joomdonation is also right about filtering logic already being present in (some) controllers' displayList implementations. I missed that completetly. Maybe it's time to update the webservice docs with this information :)

avatar joomdonation
joomdonation - comment - 23 May 2021

@pjdevries Moving the logic to ApiController::displayList() should be right direction. But maybe we should define list of model states which we want to populate data base on data from filter variable in the request and it's data type (filter) as a property of the class.

avatar pjdevries
pjdevries - comment - 23 May 2021

@joomdonation Do you mean specifically for each model? Isn't that somewhat redundant? After all, the model itself does that already. Or am I misunderstanding what you mean?

avatar joomdonation
joomdonation - comment - 23 May 2021

@pjdevries What I don't like from your propose code is that we set everything from filters array into model states. Also, the data is un-filtered, not like in the code https://github.com/joomla/joomla-cms/blob/4.0-dev/api/components/com_content/src/Controller/ArticlesController.php#L48-L79

(Note that filter.author_id is filtered into INT data type)

avatar pjdevries
pjdevries - comment - 23 May 2021

@joomdonation I am not sure why setting everything from the filters array into the model state is a bad thing, but you are absolutely right about the filtering.

So what you propose is a list of mappings between query parameters and model states with allowed types in each controller, which can be processed in Joomla\CMS\MVC\Controller\ApiController::displayList(). Is that correct?

avatar joomdonation
joomdonation - comment - 23 May 2021

@pjdevries Yes. That's what I wanted to propose. But I never good at API design, so maybe you should ping @wilsonge and ask him for what to do.

avatar wilsonge
wilsonge - comment - 23 May 2021

So what you propose is a list of mappings between query parameters and model states with allowed types in each controller, which can be processed in Joomla\CMS\MVC\Controller\ApiController::displayList()

As @joomdonation said the correctly - we need to have filtering code in place - and the correct place for this is in the Controller anyhow rather than the view. A mapping of query name to state property + filter type should be fine (and probably would be more maintainable).

If we look at Joomla\Component\Content\Api\Controller\ArticlesController::displayList() and Joomla\Component\Categories\Api\Controller\CategoriesController::displayList(), we see different ways of obtaining filter parameters from the request.

The main difference is due to the fact that the component name for categories is in the path rather than the query - as a result the data comes from different sources. Any other category filtering (e.g. if we added filtering by category created author) would be in the same format as the code in ArticlesController.

avatar pjdevries
pjdevries - comment - 24 May 2021

Thanks for the feedback @wilsonge.

The main difference is due to the fact that the component name for categories is in the path rather than the query - as a result the data comes from different sources. Any other category filtering (e.g. if we added filtering by category created author) would be in the same format as the code in ArticlesController.

What puzzles me, is that the ArticlesController obtains the filter parameters from filter[] whereas the CategoriesController does not. However, I can still pass filter[extension] in my query and it will be processed as I expect. Apparently the filter[extension] value is set as extension directly in the input object somewhere, but I yet have to discover where that is.

Not sure I understand what you mean by '...is in the path...'. If I were to generalize filter parameter mapping as suggested above, what does this mean for the processing logic? When, i.e. for what kind of filter parameters, would I simply do the same as the ArticlesController and when do something like in the CategoriesController?

avatar joomdonation
joomdonation - comment - 24 May 2021

Actually, for each ListModel, we already have available filters defined in filter form, for example https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_content/forms/filter_articles.xml#L3-L114

So I'm thinking that we can use that existing filter form to filter data from input and set the filtered data for model states (instead of set value for individual model states at how we are doing at the moment). Right now, we do not define filter attribute for each filter form, but that could be easily added.

avatar pjdevries
pjdevries - comment - 24 May 2021

@joomdonation I'm not sure if the filter forms are the proper objects to retrieve the api request filter parameters from. Filter forms serve a different purpose.

avatar joomdonation
joomdonation - comment - 24 May 2021

@pjdevries To me, it's the right method to get the filtered data:

  • We use that form to display filter options in the view
  • Then the model get data submitted from that that filter form, populate model states to query database to get filtered items. Unfortunately, it does not use form filter API to get actual filtered data yet (so, the data set in model states are still raw data)
  • We already use Form API for filtering data while saving item, so it should not be wrong to use API to get filtered data from filter form.

But as usual, I'm still not good with API yet.

avatar wilsonge
wilsonge - comment - 24 May 2021

Apparently the filter[extension] value is set as extension directly in the input object somewhere, but I yet have to discover where that is.

That's definitely not expected!

Not sure I understand what you mean by '...is in the path...'

The URL you send category data to is like /api/v1/content/categories and we therefore hardcode the extension as an additional input parameter - see e.g. https://github.com/joomla/joomla-cms/blob/4.0-dev/plugins/webservices/content/content.php#L48-L52

Because although categories is a shared component in the API we treat it the same way we do in the UI - as associated with the content/contact etc components

avatar joomdonation
joomdonation - comment - 25 May 2021

@pjdevries https://github.com/joomla/joomla-cms/compare/4.0-dev...joomdonation:webservice_filter?expand=1 is quick code show my idea about using filter form to populate model states. It has few advantages compare to current code:

  • Automatic set model states instead of having to do it manually for each component like this https://github.com/joomla/joomla-cms/blob/4.0-dev/api/components/com_content/src/Controller/ArticlesController.php#L48-L79
  • Only populate states supported by the model (define in filter form) instead of blind populating everything from data in filter from request.
  • Use filter form to filter data. So if we want to filter data to INT? Just add filter="int" to the field. Want to filter data by array? Add filter="array" to the field.... We can use all Joomla supported filter to filter and get the data in the data type we want.
avatar pjdevries
pjdevries - comment - 25 May 2021

@joomdonation I'm sorry to say I still don't think this is the right approach. I don't think forms are the right place to define elements of an API. Form definitions define the UI, which is a different thing. If anything, forms could be generated from an API definition, but that's not how Joomla! does it :)

avatar joomdonation
joomdonation - comment - 25 May 2021
avatar pjdevries
pjdevries - comment - 25 May 2021

@joomdonation Yes it does, to retrieve the form data and form validation rules. To be honest, I think this is rather ugly and not something I would like to replicate in the web service API. But just like you, I am not an expert on these matters. Far from that. Not very well educated in design patterns either. So if there is anyone with better ideas than you or me, I love to hear from them. If not, I would like to implement it as described before in this discussion and initiated by yourself ;)

avatar joomdonation
joomdonation - comment - 25 May 2021

If not, I would like to implement it as described before in this discussion and initiated by yourself ;)

That could work, too. But the fact is that we are using backend model list to query data, so re-use existing filter form could be a choice. We will then don't have to define model states and it's da type for each controller (save us some work).

avatar pjdevries
pjdevries - comment - 25 May 2021

Apparently the filter[extension] value is set as extension directly in the input object somewhere, but I yet have to discover where that is.

That's definitely not expected!

@wilsonge It's not exactly as I said. Apparently a default value for extension filtering is set in plugin PlgWebservicesContent:

$router->createCRUDRoutes(
	'v1/content/categories',
	'categories',
	['component' => 'com_categories', 'extension' => 'com_content']
);

The problem is that method CategoriesController::getExtensionFromInput() only checks for extension, ignoring filter[extension]:

return $this->input->exists('extension') ?
			$this->input->get('extension') : $this->input->post->get('extension');

My question now is, where must this be remedied: adjust the default value or the CategoriesController::getExtensionFromInput() implementation?

Before creating a new PR, based on the 'mapping of query name to state property + filter type' idea, I have one more question. It concerns query parameter and model state names. In ArticlesController::displayList(), those names are not mapped one on one. For instance: query parameter filter[category] is mapped to model state filter.category_id, dropping _id from the query parameter. Not my preference, because I like descriptive names; what it says on the tin, so to speak, But if the general consensus is different, I would like to know what the accepted naming conventions are.

avatar wilsonge
wilsonge - comment - 25 May 2021

The problem is that method CategoriesController::getExtensionFromInput() only checks for extension, ignoring filter[extension]:

This is intended specifically for extensions. The exposed endpoints are for each extensions categories. Not for the category system as a whole (if we had that then it wouldn't have /articles or /contact in the URL). For any filters within categories (e.g. created author etc) then we'd use the filter[author] etc query strings.

avatar wilsonge
wilsonge - comment - 25 May 2021

Before creating a new PR, based on the 'mapping of query name to state property + filter type' idea, I have one more question. It concerns query parameter and model state names. In ArticlesController::displayList(), those names are not mapped one on one. For instance: query parameter filter[category] is mapped to model state filter.category_id, dropping _id from the query parameter. Not my preference, because I like descriptive names; what it says on the tin, so to speak, But if the general consensus is different, I would like to know what the accepted naming conventions are.

There's no defined conventions. I'm happy to take on board any suggestions (the format of the URLs I want to match the recommendations in the JSON API spec https://jsonapi.org/recommendations/#filtering). Obviously state definitely doesn't have to be a one to one mapping to the API property. But exactly what the string to filter on is in the URL i'm much less fussed about (although to me the filter property should match that in the body)

Generally in the context of an article - the only category property you can filter by is the category id (you can't filter articles by the category author or something weird). So to me I don't see the point of having the trailing _id prefix (even if that's what it is). But that's my personal preferences not some project level decision making.

avatar pjdevries
pjdevries - comment - 26 May 2021

@wilsonge mentioned

Generally in the context of an article - the only category property you can filter by is the category id (you can't filter articles by the category author or something weird). So to me I don't see the point of having the trailing _id prefix (even if that's what it is).

At this point in time that's absolutely true. But who is to say this will not change in the future? If we decide to be specific now, we don't have to change the API later, when the need for other category related filters arises. Also for the purpose of clarity, I think it's always a good idea to be as specific as possible and have code document itself as much as possible. When using category_id it's absolutely clear what we mean. When using category that's not necessarily the case for each and everyone.

So if nobody has any objections, I will try to be as specific as possible.

avatar pjdevries
pjdevries - comment - 30 May 2021

@wilsonge, @alikon & @joomdonation I created a new PR #34288 based on our discussion here and will close this one.

avatar pjdevries pjdevries - change - 30 May 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-05-30 13:36:27
Closed_By pjdevries
Labels Added: ? ? ? ?
avatar pjdevries pjdevries - close - 30 May 2021
avatar joomla-cms-bot joomla-cms-bot - change - 30 May 2021
Category Unit Tests Repository Administration Libraries

Add a Comment

Login with GitHub to post a comment