GoSnappi: HTTP configuration controller SetConfig issue

[rudranil-das]

Found in Snappi release: Dev implementation of HTTP Server implementation as in https://github.com/open-traffic-generator/snappi/tree/gosnappi_server_tmp/gosnappi

I am elaborating the issues with reference to SetConfig in configuration_controller.go as in snippet below,

func (ctrl *configurationController) SetConfig(w http.ResponseWriter, r *http.Request) {
    var item gosnappi.Config
    if r.Body != nil {
        body, _ := ioutil.ReadAll(r.Body)
        if body != nil {
            item = gosnappi.NewConfig()
            err := item.FromJson(string(body))
            if err != nil {
                item = nil
            }
        }
    }
    result := ctrl.handler.SetConfig(item, r)
    if result.HasStatusCode200() {
        httpapi.WriteAnyResponse(w, 200, result.StatusCode200())
        return
    }
    if result.HasStatusCode400() {
        httpapi.WriteAnyResponse(w, 400, result.StatusCode400())
        return
    }
    if result.HasStatusCode500() {
        httpapi.WriteAnyResponse(w, 500, result.StatusCode500())
        return
    }
    httpapi.WriteDefaultResponse(w, http.StatusInternalServerError)
}

Issue 1: result.StatusCode200() [or 400/500] are ResponseWarning/ResponseError both of whom are interfaces. So the following does not work [json.Marshal in httpapi.WriteAnyResponse fails to write warnings/errors]

httpapi.WriteAnyResponse(w, 200, result.StatusCode200())

I think Msg() on StatusCode should be passed on instead (which works for me, please check), i.e.

httpapi.WriteAnyResponse(w, 200, result.StatusCode200().Msg()) 

Issue 2: There is no error-handling in the following block,

         if r.Body != nil {
        body, _ := ioutil.ReadAll(r.Body)
        if body != nil {
            item = gosnappi.NewConfig()
            err := item.FromJson(string(body))
            if err != nil {
                item = nil
            }
        }
    }
    result := ctrl.handler.SetConfig(item, r)

• ioutil.ReadAll can reurn error and so does item.FromJson.
• In both of these cases, there is no point the workflow to reach to ctrl.handler.SetConfig, it should return from error generating block with StatusCode400 since it would be a gosnappi unmarshal error.
• Since the actual error is captured here, this is the suitable place to return instead of propagating further.

• Another point in this context as incoming request-binding is part of gosnappi controller APIs now. This is related to open issue https://github.com/open-traffic-generator/openapiart/issues/247 due to which we use a small workaround in ixia-c controller we currently use (without gosnappi, binding incoming request with http echo server). I am providing current ixia-c controller bind snippet as mentioned below (workaround is the if len(str) == 0 block)

  func BindRequest(c echo.Context, in CanToFromJson) status.Status {
  req := c.Request()
  st := status.NewStatus()
  
  body, err := ioutil.ReadAll(req.Body)
  if err != nil {
      st.AppendErr(err, status.ValidationError) //ignore st, it should be httpapi.WriteAnyResponse with StatusCode400 for you
      return st
  }
  
  str := string(body)
  // TODO: Use "{}" instead of empty string due to:
  // https://github.com/open-traffic-generator/openapiart/issues/247
  if len(str) == 0 {
      str = "{}"
  }
  if err := in.FromJson(str); err != nil {
      st.AppendErr(err, status.ValidationError) //ignore st, it should be httpapi.WriteAnyResponse with StatusCode400 for you
      return st
  }
  
  return st
  }

Note: For both the issues, same fix needs to be done for all the APIs in all the controllers (config/control/metrics/...)

[alakendu]

@rudranil-das This is the update for Issue-1. It looks like our generator code not parse "$ref". Therefore it will not fill with proper write method. I have fix that issue and current code looks like

func (ctrl *configurationController) SetConfig(w http.ResponseWriter, r *http.Request) {
    var item gosnappi.Config
    if r.Body != nil {
        body, _ := ioutil.ReadAll(r.Body)
        if body != nil {
            item = gosnappi.NewConfig()
            err := item.FromJson(string(body))
            if err != nil {
                item = nil
            }
        }
    }
    result := ctrl.handler.SetConfig(item, r)
    if result.HasStatusCode200() {
        httpapi.WriteJSONResponse(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)
}

At the time of debugging I found issue related to []Byte response. So I also incorporate that

result := ctrl.handler.GetCapture(item, r)
if result.HasStatusCode200() {
    httpapi.WriteByteResponse(w, 200, result.StatusCode200())
    return
}

func WriteByteResponse(w http.ResponseWriter, statuscode int, data []byte) (int, error) {
    w.WriteHeader(statuscode)
    w.Header().Set("Content-Type", "application/octet-stream; charset=UTF-8")
    return w.Write(data)
}

please review. I have updated generated file in the same branch (https://github.com/open-traffic-generator/snappi/tree/gosnappi_server_tmp/gosnappi)

[alakendu]

This issue FromJson() accepts empty string but does not initialize objects to defaults resulting in nil pointer error on accessing getter openapiart#247 already fix and should available.

[alakendu]

issue-2: This would be the snippet for this fix

if r.Body != nil {
    body, readError := ioutil.ReadAll(r.Body)
    if body != nil {
        item = gosnappi.NewConfig()
        err := item.FromJson(string(body))
        if err != nil {
            ctrl.responseSetConfig400(w, err)
            return
        }
    } else {
        ctrl.responseSetConfig400(w, readError)
        return
    }
} else {
    bodyError := errors.New("Request do not have any body")
    ctrl.responseSetConfig400(w, bodyError)
    return
}

func (ctrl *configurationController) responseSetConfig400(w http.ResponseWriter, rsp_err error) {
    result := gosnappi.NewSetConfigResponse()
    result.StatusCode400().SetErrors([]string{rsp_err.Error()})
    httpapi.WriteJSONResponse(w, 400, result.StatusCode400())
}

[alakendu]

@jkristia please review those two approach to fix this issue. Note: We will add a restriction in bundler to have 400 and 500 response within model./ add default 400 and 500 response at the time of bundling.

[alakendu]

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