open-traffic-generator / snappi

Open Traffic Generator SDK in Python and Go
MIT License
69 stars 7 forks source link

GoSnappi HTTP: captureController GetCapture does not recognise StatusCode200 with packets = nil #144

Closed rudranil-das closed 2 years ago

rudranil-das commented 2 years ago

A response to GetCapture API call with StatusCode200 but no packets is a valid use-case (e.g. due to a capture-filter which does not result in any packet to be captured). In this case GetCaptureResponse from concrete implementation would return with

result := gosnappi.NewGetCaptureResponse()
result.SetStatusCode200(nil)  \\ no captured bytes

But with above, captureController GetCapture sends out a 500 response through

httpapi.WriteDefaultResponse(w, http.StatusInternalServerError)

That is because the HasStatusCode200 of returned result (GetCaptureResponse) results false due to nil-check

[snippet from func (ctrl *captureController) GetCapture...]

        result := ctrl.handler.GetCapture(item, r)  //concrete implementation return result (GetCaptureResponse) 
    if result.HasStatusCode200() {   // returns false due to nil-check
        httpapi.WriteByteResponse(w, 200, result.StatusCode200())
        return
    }
    if result.HasStatusCode400() {
        httpapi.WriteJSONResponse(w, 400, result.StatusCode400())
        return
    }
    if result.HasStatusCode500() {
        httpapi.WriteJSONResponse(w, 500, result.StatusCode500())
        return
    }
    httpapi.WriteDefaultResponse(w, http.StatusInternalServerError)  // 500 response returned from here

This impacts a couple of TCs in ixia-c sanity e.g. http://gitlab.it.keysight.com/athena/tests/blob/main/py/validation/test_api_payload.py (TC: test_get_capture_for_port_not_in_flow)

alakendu commented 2 years ago

We will return HTTP code specify within model. At the same time call 200-OK as default.

result := ctrl.handler.GetCapture(item, r)
if result.HasStatusCode400() {
    httpapi.WriteJSONResponse(w, 400, result.StatusCode400())
    return
}
if result.HasStatusCode500() {
    httpapi.WriteJSONResponse(w, 500, result.StatusCode500())
    return
}
httpapi.WriteByteResponse(w, 200, result.StatusCode200())
jkristia commented 2 years ago

@alakendu I disagree with this. its is not OK to return OK as default. If nothing is set, then that is an error. If one particular return model does not work properly, then modify the model. Do not change the controller code for this

jkristia commented 2 years ago

@rudranil-das in this particular case, do not set nil, but empty array. Empty array indicates success, but nothing was captured

alakendu commented 2 years ago

According to the suggestion of @jkristia we are passing empty array from Athena controller. So no change in our servergen logic.

This should address in https://github.com/open-traffic-generator/snappi/releases/tag/v0.7.1