Open cochicde opened 6 years ago
You are absolutely right. We will fix it tomorrow in the dev branch.
I'm unsure about this. The Orchestrator currently can throw 23 different DataNotFoundExceptions (or just 10 if we don't count the management interfaces), each having a meaningful message towards the requester system (or person), why the request failed. I think that is useful information to give to the requester system.
In the case of the SR it is a bit different, since the query service is used by the Orchestrator and Gatekeeper only, and it can have only 1 reason to return an empty response.
I think the DataNotFoundExceptions are something internal to the orchestrator and as a client I don't care why it didn't find anything, but I just need to know that there aren't any rules for my system. As a client I cannot guess if the orchestrator would send me some information that no one asked. The missing of rules is not always a problem. In case it's a problem, then the information should go the logger but not back to the client which is not expecting any type of information more than the response with "multiple" rules (altough I think it should state "0 or more" rules)
The error message has a predictable format (https://github.com/arrowhead-f/core-java/blob/master/common/src/main/java/eu/arrowhead/common/exception/ErrorMessage.java) just as the normal messages, and error messages are always sent with 4XX or 5XX HTTP status codes.
I think clients should check for the status code (any status code starting with 2 is a successful request, others not), and parse the response based on the status code. Error messages are only sent when the status code is not 2XX.
The errors are logged already, but that will not help the requester system to know why it did not get served. It can be that orchestration store rules are not there, or it could be because of authorization rules were missing, or another core system was unavailable (that would be another type of exception), and many other reasons (https://github.com/arrowhead-f/core-java/blob/master/common/src/main/java/eu/arrowhead/common/exception/ExceptionType.java).
Well I disagree there. If as a client I should check something that some specific implementation does, then everytime the implementation changes, I need to change my client and I cannot then use the same client in other implementations. This means that I need to know the orchestrator here implemented to implement my client. Then if I want to implement some client for other system, for example the event handler, I would need to go into the code, test it first to understand how it behaves and then implement my client. I don't see the point of having a defined interface if then I need to adapt my code to every implementation. If the SD and the IDD don't match, then the framework makes no sense.
You are right Jose. The first issue is that the documentation must be in line with the implementation. We are trying to keep this, naturally. The coherence between the documents, and with the implementation could use some touch, indeed. However, we inherited some definitions, parts, and some of the parts we wrote could use some touch as well.
The second thing is related to the interface itself. As of now, the the Orchestration SD and IDD contains the two types of response payload: OrchestrationResponse or DATA_NOT_FOUND_EXCEPTION. These payloads differ, and can be identified by the response code (200 for resp and 400+ for exceptions).
I think app systems need the exceptions as well, maybe not in the current verbose way, but they should be aware why their request failed (and what failsafes need to be triggered). A simple, empty response would not allow for such notifications. This is a deeper design issue, maybe we should even merge back this exception type information to the OrchestrationWarnings field. Need to think about it.
"The 4xx (Client Error) class of status code indicates that the client seems to have erred."
That there's no matching system is hardly a client error - afterall the client is using the correct uri for the orch. I do agree with Zoltan that it would be nice to include a more verbose reason for this, f.ex. "You don't have permission to access any of the matching systems, please contact your cloud administrator". This will greatly help new Arrowhead users. The reason could be given in a separate json field in the response.
May a 204 could be applicable in this situation: "The 204 (No Content) status code indicates that the server has successfully fulfilled the request and that there is no additional content to send in the response payload body."
I think that 4xx/5xx primary applies to cases where the orch service fails - but that works exactly as it should.
The problem with 204, that some libraries automatically assume there is no response body at all when seeing that status code.
Yea, true, wouldn't go well with the reason message...
when requesting information for a system to the empty orchestrator using the orchestrator/orchestration interface I get:
{ "errorMessage": "No Orchestration Store entries were found for consumer Deposit1", "errorCode": 404, "exceptionType": "DATA_NOT_FOUND", "origin": "http://localhost:8440/orchestrator/orchestration", "documentation": "https://github.com/arrowhead-f/core-java/tree/master/documentation" }
According to the SD of the orchestrator: "The response to a Service Request is the Orchestration Response, which can contain several Orchestration Forms", so when the the are no rules for the requested system, the orchestrator should return { "response" : [] }
the same way the serviceRegistry returns { "serviceQueryData": [] }
when no service is found.
I think the error messages should be returned only if the request is not properly formed