Add explicit error message (SOAP response) #1007
Conversation
|
Thanks @sfraigneau, for this PR! I marked it as a part of #895 because only deals with Soap mocks. That's a great first step! |
There was a problem hiding this comment.
Thanks a lot for the PR. I had a few comments and questions I have on status code... Looking forward to getting your opinions on this!
Also I have to figure out how to remove those 3 commits that were already approved into another PR now merged. Do you have any idea?
| if (action == null || action.length() > 0) { | ||
| action = operationName; | ||
| } |
There was a problem hiding this comment.
I don't get this condition purpose... At this point of flow, we haven't found rOperation using the action so we're looking at operation extracted from the body payload.
I think we could directly set action = operationName no?
There was a problem hiding this comment.
As the requested action in header takes precedence over the requested action in the body, I return the header one if the client asks for action in both header and body.
I set the action variable only in case the client asks for action in body only
There was a problem hiding this comment.
But action == null || action.length() > 0 is only true if action is null or not empty, right?
So we have 2 cases here:
- client didn't ask for action in header (action is null because not found in header)
- client did ask for action in header (action is not empty as found in header).
Are you sure it's a ||? I don't see why we couldn't just have action = operationName... sorry.
There was a problem hiding this comment.
Yes you're right, the condition was wrong. It should be :
action == null || action.length() == 0
I changed the way to check by using hasText from Spring utils. It is more readable.
| if (wsdlResources.isEmpty()) { | ||
| return new ResponseEntity<String>( | ||
| String.format("The service %s with version %s does not have a wsdl!", serviceName, version), | ||
| HttpStatus.NOT_FOUND); |
There was a problem hiding this comment.
Agree with the check but we need to have a decision on status code.
There was a problem hiding this comment.
Could be a 400 here. My point was that the wsdl of the service is part of the resource and it does not exist
There was a problem hiding this comment.
I wonder if 412 wouldn't be more appropriate: a validation precondition set by the client cannot be fulfilled...
There was a problem hiding this comment.
412 is good for me
There was a problem hiding this comment.
Can you thus change it to 412 above? Then I think we'll be good to merge!
I forgot to sign-ff my commit and after force push, these commits appear in the PR. No idea why |
Maybe rebasing your branch on |
Signed-off-by: sfraigneau <[email protected]>
Signed-off-by: sfraigneau <[email protected]>
| if (wsdlResources.isEmpty()) { | ||
| return new ResponseEntity<String>( | ||
| String.format("The service %s with version %s does not have a wsdl!", serviceName, version), | ||
| HttpStatus.NOT_FOUND); |
There was a problem hiding this comment.
Can you thus change it to 412 above? Then I think we'll be good to merge!
Signed-off-by: sfraigneau <[email protected]>
|
Thanks a lot @sfraigneau ! |
Description
Return an explicit message when an error occurs while mocking soap response
Note : The error message is returned as the response body like others. Using Spring exception handler may be more appropriate.
Related issue(s)
#895