or am I just missing something?

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.

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

@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.

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.

@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.

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.

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 ;-)

@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.

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.