sdn-sense / sense-o-py-client

Other
0 stars 3 forks source link

Normalize the request return structure #23

Closed xi-yang closed 1 year ago

xi-yang commented 3 years ago

The API server returns mix of structures, including HTTP native error, our wrapped error and plain text.

We should leave the StackV code alone for now and do some translation on the client end to normalize the return structure for both successful and failed requests. Users will then have a common (presumably JSON) structure to work with.

Need to come up a design first.

xi-yang commented 2 years ago

The REST error return a body along with code. For example, code 403 may have the below body.

{
    "method": "operate",
    "code": "access_error",
    "exception": {
        "message": "Refused access to INSTANCE 79a27776-84b3-4e88-8e3b-1b9f958d3c47 due to: INSTANCE_LOCKED",
        "type": "net.maxgigapop.mrs.rest.api.common.AccessException"
    }
}

We need to expose the exception message to client output. This may involve modifying RequsetWrapper code.

R-Jimenez commented 2 years ago

Shouldn't be too terrible. What kind of reporting were you looking to give back? Just passing along the exception message if it exists? And did you want me to handle this issue?

xi-yang commented 2 years ago

Exception message like the above. I think it is a good idea that you take on modifying the sense/client/requestwrapper.py code to expose the exception message from API response body. Then use that as a tool to test the if the changes in StackV API layer has sufficiently exposed the backend error instead of just throwing IOException etc.

I can help with some of the tests too.

R-Jimenez commented 2 years ago

Sounds good, I'll reboot my python environment and get on this. Shouldn't be too difficult.

R-Jimenez commented 2 years ago

All set. Access control now properly returns the exception message if it's present and proper JSON.

xi-yang commented 2 years ago

We still see null pointer error message for querying an instance that does not exist. Need to find out where the error is originated.

Screen Shot 2021-12-01 at 4 48 52 PM
R-Jimenez commented 2 years ago

Access check was relying on others to verify the uuid was present, but it makes more sense to do that in the query since we have to pull from DB anyways, so there is now a third reason NOT_FOUND that you can get from the standardized access exception response: Refused access to INSTANCE 106ad6f0-5442-4e2f-bca9-80b124b9edbf due to: NOT_FOUND

This should circumvent the null exception as well as the resource won't continue if this response gets sent back up.