Add support for filtering in FindUsers request

[jessegeens]

This PR adds support to a Filter type in the FindUsers request, to filter for specific user types. The old filter parameter has been renamed to the more accurate query.

[glpatcern]

Are we sure we want to introduce a breaking change?

[diocas]

I think is a good time to do it. I would argue this is the proper way to implement the thing. But if you prefer we can revert. Let's tag the people from own/openCloud affected by this to see what they suggest. If it's ok to keep or we revert and get other names (even if more confusing ones)

[jessegeens]

@butonic What do you think? If you want we can revert this change and keep the original filter name; otherwise you guys might get a breaking change in the API (rename filter -> query)

[glpatcern]

cc @micbar : this is a breaking change, but we can make it non-breaking at the expense of clarity. Both options are on the table so far.

[rhafer]

@jessegeens @glpatcern Sorry I haven't seen this earlier. But wouldn't it be better, and also backwards compatible, to introduce a new Request (e.g. FilterUsers) instead of breaking the existing one?

BTW, I was just looking into similar enhancement. I'd like to do queries like "displayName contains the substring 'xyz'". Any suggestions to how this could fit into the Filter? The query string isn't just good enough for that.

[glpatcern]

Hi @rhafer, creating a new Request is also a breaking change, in the sense that compilation breaks when a new Request/Response is introduced but not implemented. What I thought, at the expense of clarity, was to introduce the new Filter with a different name, such as filterBy.

Yet, many other CS3 APIs have filter with type Filter, and this was clearly overlooked at the beginning. So the breaking change does make sense after all.

[rhafer]

Hi @rhafer, creating a new Request is also a breaking change, in the sense that compilation breaks when a new Request/Response is introduced but not implemented.

Actually (talking about the go implementation here) if the userprovider would properly embed the UnimplementedUserAPIServer struct in it's service struct it should not result in compile errors. That was invented exactly for this case AFAIK. But yeah currently we're not using that.

What I thought, at the expense of clarity, was to introduce the new Filter with a different name, such as filterBy.

Yet, many other CS3 APIs have filter with type Filter, and this was clearly overlooked at the beginning.

Agreed.

So the breaking change does make sense after all.

Well, I'd just preferred if this would have been done in a none breaking manner. Additionally the implementation of FindUser() is getting somewhat clumsy IMO.
E.g. what is the expected behavior if both, Query and Filter are present? What if none is there?

[glpatcern]

Actually (talking about the go implementation here) if the userprovider would properly embed the UnimplementedUserAPIServer struct in it's service struct it should not result in compile errors. That was invented exactly for this case AFAIK. But yeah currently we're not using that.

Good to know, we should adopt that too - There are a few pending renames that would improve clarity but were pushed back to avoid to break backwards compatibility.

So the breaking change does make sense after all.

Well, I'd just preferred if this would have been done in a none breaking manner. Additionally the implementation of FindUser() is getting somewhat clumsy IMO. E.g. what is the expected behavior if both, Query and Filter are present? What if none is there?

Yes, I'd have preferred a non-breaking change as well. Now, if we agree to go for a breaking change nonetheless for the sake of clarity and consistency, we could even drop query and base everything on filters. I'd have to check how much the query is really used.

[glpatcern]

@rhafer Answering to my question, query is actually used as a generic query in several places. The logic is that Filter applies as post-processing after the query is performed: if no filter, return all results from query - see e.g. https://github.com/cs3org/reva/pull/5291/files#diff-d254163de2082008c31583d083be7d0564eb87bf795d036cc4d90b652e7ba2ac

Now, query is the former REQUIRED filter, so the behavior when none is present was and remains to return a Bad Request.

BTW a similar approach should be implemented for the Group filters, that would need a similar breaking change. Shall we bundle them and get the API fixed for good? I'd even have another breaking PR concerning OCM in the same vein!

[rhafer]

@rhafer Answering to my question, query is actually used as a generic query in several places. The logic is that Filter applies as post-processing after the query is performed: if no filter, return all results from query - see e.g. https://github.com/cs3org/reva/pull/5291/files#diff-d254163de2082008c31583d083be7d0564eb87bf795d036cc4d90b652e7ba2ac

But can't those cases where Query it still used be turned into a Filter? I find the behavior of still having Query required and use Filter just as an additional "post Query" filter really un-intuitive.

Now, query is the former REQUIRED filter, so the behavior when none is present was and remains to return a Bad Request.

BTW a similar approach should be implemented for the Group filters, that would need a similar breaking change. Shall we bundle them and get the API fixed for good? I'd even have another breaking PR concerning OCM in the same vein!

I'd be still in favor of introducing a new Request Type for that. The 2nd best option would IMO be to get rid of the Query Parameter completely and only use Filter (maybe turn Query it into a specific FilterType). But I really don't like to have both Query and Filter in one Request.

[glpatcern]

I'd be still in favor of introducing a new Request Type for that. The 2nd best option would IMO be to get rid of the Query Parameter completely and only use Filter (maybe turn Query it into a specific FilterType). But I really don't like to have both Query and Filter in one Request.

OK, let me give it a try on a new PR and we continue the discussion there. A caveat going in this direction is that we'll introduce the possibility to return CODE_RESOURCE_EXHAUSTED (i.e. server is rate-limiting) to signal a too large response, which could happen if no filter is given at all. This was already possible in the past with a too wide query, so it makes sense to have such return value explicitly.