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.

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