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 not all issues returned via REST API. #1464

Closed
wants to merge 1 commit into from

Conversation

obmsch
Copy link
Contributor

@obmsch obmsch commented Feb 11, 2019

Currently an implicit current/default filter is applied. And therefore, the
returned issue list might lack a lot of issues assigned to the project. For
the default filter that's actually only 'closed' issues, for the current that
might be all (any filter with empty results).

  • Add FILTER_STANDARD_ANY
  • Add filter_create_any to filter_api
  • Handle FILTER_STANDARD_ANY in filter_standard_get
  • Use in issues_rest, if the request doesn't contain a filter_id

Fixes #25102

- Add FILTER_STANDARD_ANY
- Add filter_create_any to filter_api
- Handle FILTER_STANDARD_ANY in filter_standard_get
- Use in issues_rest, if the request doesn't contain a filter_id

Fixes #25102
@cproensa
Copy link
Contributor

If we consider filter_standard_get() as a constructor to retrieve a prebuilt filter, this may be a reasonable approach, and avoid dealing directly with the filter array low level details.

  • I wonder if "any" is the best name for it. I'd propose a name that reflects better that it's an "empty" filter. in the sense that it's not applying any filter criteria.

  • I'm not sure that it's good to have an explicit project_id initialized for that filter. If you look at a default filter, it is initialized with FILTER_PROPERTY_PROJECT_ID => array( META_FILTER_CURRENT ).
    I tend to think that project_id should be set by the consumer.

  • Why initialize FILTER_PROPERTY_HANDLER_ID? It's not currently set by default, iirc.

  • Consider looking also to force a value for _view_type (in the consumer code, not in filter_standard_get), as simple/advanced has different behavior in including subprojects. I guess that a consistent behaviour is expected for the soap/rest api, regardless of the configured simple-only/advanced-only, etc.

I have been slowly changing the filter api, and one of the future plans i had in mind is separate the actual concepts of "empty" and "default" filter. Having your addition in filter_standard_get() as a single point to get an actual "empty" filter is good for future refactoring.
At a later point we could refactor that once we have a real "filter_get_empty()", "new Filter()", or whatever.

So in summary:
This new standard filter entry should override the properties of a "default" filter to make it truly an empty filter, and in our current state of the code, that means to "undo" the artificial defaults like "hide status".
The consumer is then responsible to apply properties as needed.

My suggestions may not be accurate or can lead to other issues, so pleas don't follow them blindly. Let's just make a starting point for a common approach that fits in.

@obmsch
Copy link
Contributor Author

obmsch commented Feb 12, 2019

I choosed 'any' along the lines of the other standard filters ( 'assigned', 'unassigned', 'reported', 'monitored' ),
which return issues having that attribute. And this new filter should return any/all issues. That said, I am open
to choose something else. But 'empty' sounds a bit odd in this context. Sure the filter has no restrictions and
'empty' from this POW makes a lot of sense, but 'filter_create_empty' (define( 'FILTER_STANDARD_EMPTY', 'empty' ); ) at least looks weird from a callers POW (Am I looking for 'empty' issues, whatever those are?).
Or to rephrased it: I want all issues, even the 'empty' ones. A name on that, and I am all yours.

For now there's only one caller for 'filter_standard_get': mc_filter_get_issues(), that's soap (also used called from rest).
So changing the api, and putting the spec of filter (additional) restrictions on the caller/consumer side shouldn't be the problem. But fiddling with low level filter attributes there, makes no sense at all.
As long as we don't provide something like '$filter->SetProject', '$filter->SetHandler', ... that's a no go.

  • Initializing FILTER_PROPERTY_HANDLER_ID here is in fact not needed. I wasn't sure and really got lost in 'filter_ensure_valid_filter' and all the checks.

And regarding 'view_type', that's WebUI, any requester (soap, rest, ...) should't bother at all, we should
extend 'view_type' with: null, none, ...

At least this is a change, fixing a real issue without breaking BC. Anything else could be applied on top.

@cproensa
Copy link
Contributor

I choosed 'any' along the lines of the other standard filters ( 'assigned', 'unassigned', 'reported', 'monitored' ), which return issues having that attribute. And this new filter should return any/all issues
Or to rephrased it: I want all issues, even the 'empty' ones.

Good point, i buy it.
An "empty" filter (empty of properties) should return "any" issue.

For now there's only one caller for 'filter_standard_get': mc_filter_get_issues(), that's soap

filter_standard_get is a wrapper for filter_create_xxxx, which are also user from "my view"

As long as we don't provide something like '$filter->SetProject', '$filter->SetHandler', ... that's a no go

Right now there's no other way to customize a filter, other that directly setting filter[xxx] properties. It's nothing more than an associative array.

What i mean with fiddling the low level, is:
filter_standard_get should override the properties that filter_ensure_valid initializes as default, to achieve a filter that effectively returns "any" issues. That means, resetting hide-status property, and probably more.
That's the rationale of your PR, and it's the correct approach.

What else should be overriden?
project id, i'd say not. Because then it won't match "any" issues, only issues in that project. When using META_FILTER_CURRENT, the filter is scoped to the current project at runtime. that's why i said, that the soap api should specify the project. If i recall correctly, that's already done by overriding the current project setting, to correctly apply any access, config, etc api call to the project used in the context.

filter_standard_get should be neutral about the project property.
But, If we look at other filter_create_xxx functions, they are explicitly setting current project as project_id, and that also doesn't make sense to me (but that out of the scope of this PR)

I suggest to not modify filter_standard_get, just let the 'any' filter to have project_id=META_FILTER_CURRENT (it's by default), and let the soap api apply the project in context with
$g_project_override = $p_project_id; (in line: https://github.com/mantisbt/mantisbt/pull/1464/files#diff-d83d4890b372807ae66ee2cb786e4f38R197)
Give it a try, i bet it will work.

And regarding 'view_type', that's WebUI, any requester (soap, rest, ...) should't bother at all,

'_view_type' (which i hope we can get rid of in a close future) has some side effects on the project scope of the filter. See https://www.mantisbt.org/bugs/view.php?id=20238
And this property is initialized in the default filter according to some configurations that can dictate that filters are "simple" or "advanced" by default.
So my question is (because i honestly don't know): when asking the soap/rest api for the filter in project X, Should it return (A) issues only in that project, or (B) also in subprojects?
If case (A), then soap api should make sure to set project_id and view_type=advanced, otherwise, if case (B), set project_id and view_type=simple

we should extend 'view_type' with: null, none, ...

I'd rather remove it, in future iterations.

Copy link
Member

@vboctor vboctor left a comment

Choose a reason for hiding this comment

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

Thanks @obmsch for your contribution.

Just confirming some functional behaviors:

  • Get Issues for SOAP/REST will now return all issues independent of user's filter and including closed issues.
  • How does Get Issues from REST API get project id? I assume it assumed all projects, right? Haven't looked if this is applicable to SOAP API or not.
  • Does GET {{url}}/api/rest/issues?filter_id=any work like GET {{url}}/api/rest/issues?filter_id=monitored.
  • Similar to any should we have default? This would return all issues that are not closed? Similar to the filter a brand new user gets by default.

@obmsch
Copy link
Contributor Author

obmsch commented Feb 17, 2019

@vboctor notwithstanding @cproensa comments, the PRs intension was to fix an issue with rest.
And to answer your assumptions.

Get Issues for SOAP/REST will now return all issues independent of user's filter and including closed issues.

REST will, SOAP not. In contrast to REST, SOAP doesn't offer a filter on the get-issue API, and an API
change is really out of scope for this PR and my skills. At least SOAPs 'mc_filter'-API would benefit and
return the expected result on 'any'.

How does Get Issues from REST API get project id? I assume it assumed all projects, right? Haven't looked if this is applicable to SOAP API or not.

That's either through request param 'project_id' (may be 0 = ALL_PROJECTS), or defaults to ALL_PROJECTS.

$t_project_id = (int)$p_request->getParam( 'project_id', ALL_PROJECTS );

Does GET {{url}}/api/rest/issues?filter_id=any work like GET {{url}}/api/rest/issues?filter_id=monitored.

If request param 'filter_id' exists that's passed through, otherwise the filter_id is set to any.
So GET {{url}}/api/rest/issues?filter_id=any and GET {{url}}/api/rest/issues will return the same set of issues.

Similar to any should we have default? This would return all issues that are not closed? Similar to the filter a brand new user gets by default.

From an API consumers POW, I would rather have an explizit unclosed filter.

@cproensa
Copy link
Contributor

@vboctor
Can you confirm: the request for issues?project_id=xxx, should include also issues in subprojects?

In this PR: /api/rest/issues?project_id=xxx

  • If having $g_view_filters = SIMPLE_ONLY; returns issues including subprojects
  • If having $g_view_filters = ADVANCED_ONLY; returns issues only for project xxx, without subprojects.

I expect the api to have a consistent behaviour, unaffected by that config option.

Similar to any should we have default?

I'd say yes, that is a good addition.

From an API consumers POW, I would rather have an explizit unclosed filter.

At this moment, the "default" filter is equivalent to "unclosed", but that may change in the future. For example, if we add option to set a "default" filter for each user.

@vboctor
Copy link
Member

vboctor commented Feb 18, 2019

@cproensa

for the question about sub-projects and $g_view_filters

In the Web UI and REST API, we should be consistent on including sub-project issues. Simple/Advanced filters is just a way to define the filter, but shouldn't change its behavior.

Similar to any should we have default?

Seems like we agree on this. And I also was thinking of the point that you mentioned, the default filter may change over time due to MantisBT changing the default filter or providing a feature in the future where the admin can define the default filter for projects.

That is different from the user's current filter, which can be remembered per client.

@obmsch

From an API consumers POW, I would rather have an explizit unclosed filter.

As for unclosed I think it is too specific. The API should support ability to get issues matching a custom filtering criteria without having to create a well-known filter or a saved filter.

REST will, SOAP not. In contrast to REST, SOAP doesn't offer a filter on the get-issue API, and an API change is really out of scope for this PR and my skills. At least SOAPs 'mc_filter'-API would benefit and return the expected result on 'any'.

There is no desire to really innovate in SOAP API, just a bug fix applies, then that's a bonus. If a feature is added, e.g. SOAP API supports a new type of known filter "any", then we should create an issue in the tracker for it, so users know it is there. However, no desire to add new methods or functionality in SOAP. I expect we will retire the SOAP API eventually.

function filter_create_any( $p_project_id ) {
$t_filter = filter_get_default();

$t_filter[FILTER_PROPERTY_HANDLER_ID] = array( '0' => META_FILTER_ANY );
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to set there 3 criteria that just say any/none? I would expect that omitting these should deliver the same results. @cproensa ? Also should the approach be to create an array with desired criteria then call filter_ensure_valid_filter() on it? Why trigger two calls of filter_ensure_valid_filter()?

Copy link
Contributor

Choose a reason for hiding this comment

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

It has already been noted, that of those three, only FILTER_PROPERTY_HIDE_STATUS => META_FILTER_NONE is needed

Also should the approach be to create an array with desired criteria then call filter_ensure_valid_filter() on it?

Maybe filter_get_default() is not needed, and can be done with a raw array and call validate on it.
But note that this pattern is already there in other previous filter_create_xxx functions

@cproensa
Copy link
Contributor

In the Web UI and REST API, we should be consistent on including sub-project issues. Simple/Advanced filters is just a way to define the filter, but shouldn't change its behavior.

Agree, but that's just how it works since long time:

  • Simple filter: the project_id property is ignored and is run over the current project. Includes subprojects.
  • Advanced filter with project_id = meta_filter_current: same result as simple filter.
  • Advanced filter with project_id = specific_id: Does not include subprojects

The best course of action is to get rid of the simple/advanced mode, in some future.

The test i did by changing $g_view_filters between "SIMPLE_ONLY" and "ADVANCED_ONLY" shows a regression, because previously, the project_id was not explicitly set in the filter, thus the behavior is to include subprojects in both cases.
Since now the project_id is filled, the behavior is different.

Note that other named filters: assigned, monitored, etc, do fill the project_id, so this problem was already affecting them too before this PR.

The way to fix this can be (for always including subprojects)

  • Either, Do not set project_id in the filter, if the project is applied through "$g_project_override"
  • Or, Explicitly set _view_type to simple.

or for never including subprojects:

  • Always set the project_id AND set _view_type to advanced

@obmsch
Copy link
Contributor Author

obmsch commented Feb 21, 2019

To be honest, I got somewhat got lost here. The original aim of the PR was to simply fix a problem with the REST GetIssues API not returning all issues for project(n/0=ALL_PROJECTS). At least the closed ones are missing. I tried to achieve that, without changing any other behaviour.
Dropping:

  • $t_filter[FILTER_PROPERTY_HANDLER_ID] = array( '0' => META_FILTER_ANY );
  • $t_filter[FILTER_PROPERTY_STATUS] = array( '0' => META_FILTER_ANY );

I have no problem with at all. Those are only there, because I wanted to assure that I don't miss anything. This filter stuff keeps my head wheeling. @cproensa really good work.
From my point of view there are two options left:

  • I do the drops, fix possibly other (serious) issues (in the scope of this PR) and we postpone reworking
  • I am out, and anyone other picks this up

@vboctor
Copy link
Member

vboctor commented Feb 24, 2019

Merged via 4cc687a - we should open separate bugs for sub-project issues.

@vboctor vboctor closed this Feb 24, 2019
@vboctor
Copy link
Member

vboctor commented Feb 24, 2019

@cproensa I have opened a bug fix difference between simple and advanced filters for handling sub-project issues:

https://www.mantisbt.org/bugs/view.php?id=25515

@obmsch obmsch deleted the issue-25102 branch February 25, 2019 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants