User tests: Successful: Unsuccessful:
Pull Request for Issue #34100
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.
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.
yes
Status | New | ⇒ | Pending |
Category | ⇒ | Unit Tests Repository Administration |
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.
@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 :)
@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.
@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?
@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)
@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?
@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.
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
.
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
?
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.
@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.
@pjdevries To me, it's the right method to get the filtered data:
But as usual, I'm still not good with API yet.
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
@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:
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.@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 :)
@pjdevries No worry. But Joomla! uses Form not only for UIElement, but also filter https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Form/Form.php#L1109 and validate https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Form/Form.php#L1171 data
@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 ;)
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).
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.
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.
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.
@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.
@wilsonge, @alikon & @joomdonation I created a new PR #34288 based on our discussion here and will close this one.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-05-30 13:36:27 |
Closed_By | ⇒ | pjdevries | |
Labels |
Added:
?
?
?
?
|
Category | Unit Tests Repository Administration | ⇒ | Libraries |
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?