Fix Partial success HTTP status codes #3054

[Anjali-NEC]

Fix issue #3054

[fgalan]

Changing response codes is a decision that need to be done with care (it may have effect in backward compatibility). Thus, we are going to put this PR on hold while we can assure there is no problem in existing deployments.

Thanks @Anjali-NEC for your contribution!

[Anjali-NEC]

@fgalan Gentle Reminder !!

[fgalan]

This shouldn't be 2xx as there is no partial success in this case for the creation of an entity with duplicated attributes.

(NOTE: probably 422 is also wrong as according to "Error Response" rules at https://fiware.github.io/specifications/ngsiv2/stable this is a case in which the error is caused by request itself (i.e. they do not depend on the NGSIv2 server status) so 400 should be used instead. But this is a lateral issue)

[fgalan]

This shouldn't be 2xx as there is no partial success in this case for the update of an entity with duplicated attributes.

(NOTE: probably 422 is also wrong as according to "Error Response" rules at https://fiware.github.io/specifications/ngsiv2/stable this is a case in which the error is caused by request itself (i.e. they do not depend on the NGSIv2 server status) so 400 should be used instead. But this is a lateral issue)

[fgalan]

@fgalan Gentle Reminder !!

I'm still concerned by the backward compatibility implications of changing a 4xx code to a 2xx. From one 4xx to another 4xx is not a big problem, as the client logic use to deal with 4xx error conditions on the same way. However, fro 4xx to 2xx could break some client response processing logic.

Anyway, I propose to add a test to this PR with the following steps in order to gain more insight on this and help us to decide what to do next:

1. Create entity E with attributes A=1 and B=2
2. Strict append update E-A=10, we should see 422 response (as operation completely fails in this case)
3. GET E to check A=1 and B=2 have not changed
4. Strict append update E-A=10,C=30, we should see 207 response (as operation partially success in this case)
5. GET E to check partial result C=30 appended (but A=1 and B=2 untouched)

And also this one:

1. Create entity E with attributes A=1 and B=2
2. Patch update E-C=30, we should see 422 response (as operation completely fails in this case)
3. GET E to check A=1 and B=2 have not changed
4. Patch update E-A=10,C=30, we should see 207 response (as operation partially success in this case)
5. GET E to check partial result A=10 (but B=2 unchanged and C not added)

[Anjali-NEC]

I have added test case in c585d01 But currently it shows 207 Multi-Status in both the cases when operation completely fails and operation partially success.

[fgalan]

I have added test case in c585d01 But currently it shows 207 Multi-Status in both the cases when operation completely fails and operation partially success.

Having a look to your implementation, it seems you have just blindly replaced 422 by 207 in all cases. Note that issue #3054 is not about it. Issue #3054 is about the usage of the new code in some circumstances, i.e. when partial success has take place. But if the operation completely fails, then 422 usage has to be kept.

Please review implementation to adapt to this behaviour. The tests described my me above should help.

[Anjali-NEC]

Having a look to your implementation, it seems you have just blindly replaced 422 by 207 in all cases. Note that issue #3054 is not about it. Issue #3054 is about the usage of the new code in some circumstances, i.e. when partial success has take place. But if the operation completely fails, then 422 usage has to be kept.

Please review implementation to adapt to this behaviour. The tests described my me above should help.

Sorry for this I haven't understand this issue clearly. Thanks you for your guidance. Now I have reverted the changes in my latest commit 92e0a2b and added code for 207, when partial success has take place. but currently my code is working only in case of Strict append but now working in case of patch/update. I am looking into it and will update the PR soon.

[Anjali-NEC]

@fgalan sir,

I have tried to implement the code for 207 Multi-Status in case of partial update but I am getting same response in both the situation (when operation completely fails and operation partially success). I haven't found suitable solution for this.

Could you please provide some suggestion how we can get different HTTP code in both the cases.

[fgalan]

I have tried to implement the code for 207 Multi-Status in case of partial update but I am getting same response in both the situation (when operation completely fails and operation partially success). I haven't found suitable solution for this.

Could you please provide some suggestion how we can get different HTTP code in both the cases.

I don't know exactly which difficulties you have, so I can only provide generic advice: follow the code logic during an update operation (a debugger with breadkpoint feature could help, this may be useful), identify the cases of partial updates and implement the desired behaviour without breaking existing functionality (i.e. breaking existing .ftest).

[kzangeli]

Could you please provide some suggestion how we can get different HTTP code in both the cases

Seems to me you simply need to set the appropriate value for ciP->httpStatusCode.

Look at restReply, a few lines before it ends:

  MHD_queue_response(ciP->connection, ciP->httpStatusCode, response);

The second parameter to MHD_queue_response is the HTTP status code.
I believe that's what you're looking for.

Ah, I just checked, HttpStatusCode doesn't support a 207 ...
You'll have to add an SccMultiStatus constant to the enum HttpStatusCode in src/lib/rest/HttpStatusCode.h,
and most probably add a "case SccMultiStatus" in a few places for the compiler to like it.

• httpStatusCodeString() has a "default", so, the compiler won'y complain. Please add it anyway.

[Anjali-NEC]

Could you please provide some suggestion how we can get different HTTP code in both the cases

Seems to me you simply need to set the appropriate value for ciP->httpStatusCode.

Look at restReply, a few lines before it ends:

  MHD_queue_response(ciP->connection, ciP->httpStatusCode, response);

The second parameter to MHD_queue_response is the HTTP status code. I believe that's what you're looking for.

Ah, I just checked, HttpStatusCode doesn't support a 207 ... You'll have to add an SccMultiStatus constant to the enum HttpStatusCode in src/lib/rest/HttpStatusCode.h, and most probably add a "case SccMultiStatus" in a few places for the compiler to like it.

• httpStatusCodeString() has a "default", so, the compiler won'y complain. Please add it anyway.

Added SccMultiStatus constant to the enum HttpStatusCode in src/lib/rest/HttpStatusCode.h and also added a "case SccMultiStatus" in src/lib/rest/HttpStatusCode.h but still not getting the 207 SccMultiStatus response in case of partially update.

[fgalan]

I don't know exactly which difficulties you have, so I can only provide generic advice: follow the code logic during an update operation (a debugger with breadkpoint feature could help, this may be useful), identify the cases of partial updates and implement the desired behaviour without breaking existing functionality (i.e. breaking existing .ftest).

@Anjali-NEC could you tell us the status on this PR? Did you follow my advice above?

It would be great to know it, given in the next FIWARE TSC Orion is going to be discussed.

[Anjali-NEC]

I don't know exactly which difficulties you have, so I can only provide generic advice: follow the code logic during an update operation (a debugger with breadkpoint feature could help, this may be useful), identify the cases of partial updates and implement the desired behaviour without breaking existing functionality (i.e. breaking existing .ftest).

@Anjali-NEC could you tell us the status on this PR? Did you follow my advice above?

It would be great to know it, given in the next FIWARE TSC Orion is going to be discussed.

Currently I am putting this PR on hold as I am busy on another issue.

[chandradeep11]

I don't know exactly which difficulties you have, so I can only provide generic advice: follow the code logic during an update operation (a debugger with breadkpoint feature could help, this may be useful), identify the cases of partial updates and implement the desired behaviour without breaking existing functionality (i.e. breaking existing .ftest).

@Anjali-NEC could you tell us the status on this PR? Did you follow my advice above?

It would be great to know it, given in the next FIWARE TSC Orion is going to be discussed.

@fgalan
We apology for putting the issue on hold for long period. @Anjali-NEC was working on other high priority issue (#4149 ). Currently she is on holiday due to her personal urgency.
Now e have planned to resume working on this ticket in mid of Jan.

Please let us know which ticket has high priority to you in below PR:
#4186 , #4003 , #4097 and #4090
Based on the priority, we will plan our work on open tickets accordingly

[fgalan]

Please let us know which ticket has high priority to you in below PR:
#4186 , #4003 , #4097 and #4090
Based on the priority, we will plan our work on open tickets accordingly

These four ones are minor issues. Of course, Orion would be a bit better if they get completed, but they are not actually very important. In this sense, none of them has more priority than other, they can be considered equally minor.

On the contrary, I'd suggest to address another more important issues. The important issues are exposed in the Orion roadmap document, which I recommend you to check whenever you consider to pick a new issue.

In this sense, these two ones seems to be more appealing that the former four, specially the first one:

• Multivalued entity type #3638. Note this @Anjali-NEC was already working on this issue in the past so it should be easier than a completely new issue for him
• id/type patterns for batch update operation #2389

Thanks!

[chandradeep11]

In this sense, these two ones seems to be more appealing that the former four, specially the first one:

• Multivalued entity type #3638. Note this @Anjali-NEC was already working on this issue in the past so it should be easier than a completely new issue for him
• id/type patterns for batch update operation #2389

Thanks!

Thanks you for sharing the priority of PRs and also suggesting to work on two issues #3638 and #2389.
As informed you, currently @Anjali-NEC is working on ticket #4149 . This ticket is required urgently for one of our requirement where we are using Orion.
I request you to kindly provide your suggestion or review points on our approach which is provided on ticket #4149. It will help us to close this ticket soon.
Once this ticket (#4149) will be completed then we will pick the tickets suggested by you.

[fgalan]

I'm sorry we don't have time to provide feedback on #4149 in a timely manner taking into account the urgency it has for you and that it is not part of the roadmap. Issues that are in the roadmap are the ones in which focus our feedback and review effort.

Thus, go ahead with the implementation of #4149 without waiting for our feedback and we will see the PR when it comes. At that moment we have another feedback point.

[fgalan]

Overpassed by PR #4266.