View Issue Details

IDProjectCategoryView StatusLast Update
0025407mantisbtapi restpublic2024-05-22 19:13
Reportereternalstorms Assigned Todregad  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformiMac 5k, 2017OSmacOSOS Version10.14.2 (18C54)
Product Version2.18.0 
Target Version2.26.3Fixed in Version2.26.3 
Summary0025407: Resetting version fields to empty is not possible
Description

After having set a "fixed_in_version" value for an issue upon resolving it, when I'd like to re-open that issue, I'd also like to reset "fixed_in_version" to an empty value, because obviously, it wasn't fixed if I had to reopen the issue.
However, it doesn't seem to be possible right now.

I tried passing an empty dictionary
"fixed_in_version" : {}

a null object:
"fixed_in_version" : "<null">

a null-value dictionary:
"fixed_in_version" : {"id":"<null>"}
"fixed_in_version" : {"name":"<null>"}

or leaving the dictionary's values empty:
"fixed_in_version" : {"id":""}
"fixed_in_version" : {"name":""}

nothing worked.

For other fields (like handler, or status) using an empty dictionary:
"handler" : {}
resets the value to empty, or an in Mantis pre-defined value.

Additional Information

MantisBT 2.18.0

TagsNo tags attached.

Activities

eternalstorms

eternalstorms

2019-02-05 01:14

reporter   ~0061406

or am I just missing something?

dregad

dregad

2024-04-19 10:00

developer   ~0068841

I can reproduce the behavior. It applies not only to fixed_in_version, but to the other version fields as well (version, target_version).

Problem seems to be in mc_issue_update() function, which is ignoring null versions instead of unsetting the field.

dregad

dregad

2024-04-20 19:59

developer   ~0068843

PR https://github.com/mantisbt/mantisbt/pull/1997

vboctor

vboctor

2024-04-20 20:58

manager   ~0068847

@dregad I looked at the PR in 0025407:0068843 - the lack of providing the field shouldn't reset the field's value to blank. The update behavior should use a PATCH semantics. The right way to empty a version field is to explicitly set its code 0.

dregad

dregad

2024-04-21 05:02

developer   ~0068848

the lack of providing the field shouldn't reset the field's value to blank

It does not. Consider this test issue

$ curl -X GET -H "Authorization: $TOKEN" http://localhost/mantis/api/rest/issues/$ID |jq ".issues[] | {version, fixed_in_version, target_version}"

{
  "version": {
    "id": 6,
    "name": "v1.0.2"
  },
  "fixed_in_version": {
    "id": 7,
    "name": "v1.0.3"
  },
  "target_version": {
    "id": 7,
    "name": "v1.0.3"
  }
}

If I update the issue as follows

curl -X PATCH -H "Authorization: $TOKEN" -H "Content-type: application/json" http://localhost/mantis/api/rest/issues/$ID -d '{ "fixed_in_version": null }' |jq ".issues[] | {version, fixed_in_version, target_version}"

{
  "version": {
    "id": 6,
    "name": "v1.0.2"
  },
  "fixed_in_version": null,
  "target_version": {
    "id": 7,
    "name": "v1.0.3"
  }
}

Note that version and target_version, which are not specified in the payload, are not modified.

The update behavior should use a PATCH semantics

PATCH is indeed what is being used here, I'm not sure why you're mentioning that. Can you clarify ?

The right way to empty a version field is to explicitly set its code 0.

With a payload like {"fixed_in_version: {"id": 0}}, the API currently, returns HTTP 400 Version '0' does not exist in project 'Test Project'.; my PR does not change that behavior.

I think my proposed fix

  • works,
  • provides more flexibility to the user, by allowing multiple ways to unset the version (it can be done with "version": null, "version": {}, "version": {"id": null} and "version": {"name": null})
  • is therefore easier to use while still respecting the semantics of a PATCH request (i.e. no change if field is not specified).

Based on the above, I don't think it's beneficial to be more strict.

I'm OK to adapt the code, so that {"id": 0} also unsets the version if you want.

vboctor

vboctor

2024-04-21 11:19

manager   ~0068850

@dregad I may have misinterpreted the code relating to the patch semantics.

  • It would be great to add a comment in the added function to explain the semantics.
  • We should allow resetting the value in a way that doesn't change the schema - i.e. { "id": 0 } or { "name": "" }. -- i.e. avoid empty object or making fields nullable.
  • It would good if we add a test case for this.
dregad

dregad

2024-04-21 12:48

developer   ~0068851

It would be great to add a comment in the added function to explain the semantics

The anonymous functions I introduced are only here to reduce code duplication, they are not actually changing the semantics. The meaningful change here, is the removal of the if( $t_<version> != 0 ).

But I can add a comment if you want.

We should allow resetting the value in a way that doesn't change the schema - i.e. { "id": 0 } or { "name": "" }. -- i.e. avoid empty object or making fields nullable.

Can you clarify your intention ? Is that

  1. in addition to the currently working payloads I described earlier (which is what I would recommend), or
  2. do you mean to restrict the API to only allow { "id": 0 } and { "name": "" }, and throw BAD REQUEST for the other cases ?

It would good if we add a test case for this

OK will do once you have answered the question above.

dregad

dregad

2024-04-21 13:43

developer   ~0068852

It would be great to add a comment in the added function to explain the semantics

But I can add a comment if you want.

Actually the PATCH semantics are handled in the parent function rest_issue_update(), which merges the REST payload with the issue data from the DB.

But I'll add PHPDoc for the anonymous functions anyway.

And also fix a small bug I found while analyzing this ;-)

vboctor

vboctor

2024-04-21 15:02

manager   ~0068853

@dregad

do you mean to restrict the API to only allow { "id": 0 } and { "name": "" }, and throw BAD REQUEST for the other cases ?

yes, only empty the field when id = 0 or name = ''. So that is the option I'm suggesting. Note that different languages can serialize json structures differently and generate null values when fields not set, etc. So it would be good to be consistent with the schema and provide standard way to do such behavior.

Actually the PATCH semantics are handled in the parent function rest_issue_update(), which merges the REST payload with the issue data from the DB.

Just keep in mind that such APIs are typically used from REST and SOAP APIs.

dregad

dregad

2024-04-22 03:55

developer   ~0068855

only empty the field when id = 0 or name = ''

I think this makes things unnecessarily strict, but fine, I'll do as you say.

This will require changes in rest_issue_update() to validate input before passing the payload on to mc_issue_update() for processing, to ensure we have no regression on SOAP API.

different languages can serialize json structures differently and generate null values when fields not set

I would be very surprised if that were the case. Do you have evidence to support that statement ?
In particular, if I'm reading this right, the generate null values when fields not set bit, would amount to "inventing" data that does not exist...

would be good to be consistent with the schema

Pardon my ignorance, but you have referred to this schema several times and I'm not aware of its existence... Is it something that you manage in Postman ? Can you please point me to where I can find it ?

keep in mind that such APIs are typically used from REST and SOAP APIs

I see no problem here, as SOAP API does not have the notion of a patch request. mc_issue_update() expects a whole Issue as payload, and if you call it with missing or null fields, they will be cleared. It is the client's responsibility to call mc_issue_get() first to retrieve current values, which is precisely what rest_issue_update() does.

dregad

dregad

2024-05-12 09:52

developer   ~0068929

Last edited: 2024-05-12 09:53

@vboctor I finally managed to get back to this.

do you mean to restrict the API to only allow { "id": 0 } and { "name": "" }, and throw BAD REQUEST for the other cases ?

yes, only empty the field when id = 0 or name = ''. So that is the option I'm suggesting. Note that different languages can serialize json structures differently and generate null values when fields not set, etc. So it would be good to be consistent with the schema and provide standard way to do such behavior.

I analysed the API's current behavior with regards to specifying a version when adding a new issue (i.e. POST /issues). Your original implementation (see MantisBT master 8df25e99) currently accepts the following:

  • "version": null => not set, no error
  • "version": "" => not set, no error
  • "version": <integer> => not set, no error INCONSISTENT
  • "version": <string> => version set if given name is valid, bad request if not
  • "version": {} => not set, no error
  • "version": {"id": 0} => not set, no error
  • "version": {"id": <number>} => version set if given id is valid, bad request if not
  • "version": {"name": null} => not set, no error
  • "version": {"name": ""} => not set, no error
  • "version": {"name": <string>} => version set if given name is valid, bad request if not
  • "version": {"xxx": <string>} => not set, no error

Based on this, I believe that the way I currently implemented the PATCH /issues endpoint is correct and consistent with the existing behavior. Restricting the possibility of clearing the version to only "version": {"id": 0} would introduce an inconsistency.

I have updated the PR, to document the anonymous functions (PHPDoc) and add PHPUnit tests which reflect the current behavior.

I propose to merge as-is to resolve this issue.

If you still feel that the REST API should be more restrictive (per your note 0025407:0068853) then you can open another issue to track that, considering that this change should be applied globally (POST & PATCH /issues), .

Independently from that, I noted the following inconsistent behavior, which successfully (and incorrectly IMO) clears the version:

  • {"version": 99999} (invalid or non-existing version Id), should probably be BAD REQUEST instead
  • {"version": {"xxx":"yyy"}}, not sure what the correct behavior should be (BAD REQUEST)

I propose to fix these separately (added @TODO notes in the Test script).

I would still be interested in having your answer to this question:

would be good to be consistent with the schema

Pardon my ignorance, but you have referred to this schema several times and I'm not aware of its existence... Is it something that you manage in Postman ? Can you please point me to where I can find it ?

Related Changesets

MantisBT: master-2.26 cb0e07f3

2024-04-19 13:45

dregad


Details Diff
REST: allow clearing version fields

Until now it was not possible to set an issue's version, target_version
and fixed_in_version to blank.

Fixes 0025407
Affected Issues
0025407
mod - api/soap/mc_issue_api.php Diff File

MantisBT: master-2.26 bddcc92a

2024-04-21 18:08

dregad


Details Diff
Allow id 0 and name '' to clear version

The following Version payloads can now be used to unset Version fields:

- `"version": {"id": 0}`
- `"version": {"name": ""}`

Prior to this, it was only possible with `"version": null`,
`"version": ""` and `"version": {}`.

Fixes 0025407
Affected Issues
0025407
mod - api/soap/mc_api.php Diff File
mod - api/soap/mc_issue_api.php Diff File

MantisBT: master-2.26 9f6f7350

2024-05-12 09:55

dregad


Details Diff
Add PHPUnit test to update Issue Version via REST

At the moment the tests are reflecting the API's current behavior, which
may not be the intended result. See discussion in issue 0025407.
Affected Issues
0025407
add - tests/rest/RestIssueUpdateVersion.php Diff File