Skip to content

[HWC-API] API always returns HTTP 200 even on failure + patient data getting logged at INFO level #153

@sharma-sugurthi

Description

@sharma-sugurthi

so i was going through the controller layer in HWC-API trying to understand how the API responses work, and i noticed something that feels off.

every controller method returns String instead of ResponseEntity. which means the actual HTTP status is always 200, whether the request worked or failed. the error details are just stuffed inside the JSON body.

here's what it looks like in GeneralOPDController.java:

public String saveBenGenOPDNurseData(@RequestBody String requestObj, ...) {
    OutputResponse response = new OutputResponse();
    try {
        response.setResponse(genOPDRes);
    } catch (Exception e) {
        response.setError(5000, "Unable to save data");
    }
    return response.toString();  // always 200
}

this same pattern is in every controller i checked,generalOPD, ncdscreening, cancerscreening, all of them. the client always gets 200 and has to dig into the body to figure out if things actually failed.the funny part is, OutputResponse.java already has a method called toStringWithHttpStatus() that returns proper ResponseEntity with correct status codes. but none of the controllers use it, they all call toString() instead.

also noticed a couple of things in OutputResponse.java while i was there:

  • BAD_REQUEST is set to 404, but 404 is Not Found. Bad Request is 400
  • SWYMED_EXCEPTION and TM_EXCEPTION both use error code 5010 so theres no way to tell them apart the other thing i noticed,and this one feels more important given that AMRIT is a healthcare platform,is that every controller logs the full raw request body at INFO level:
logger.info("Request object for GeneralOPD nurse data saving :" + requestObj);

these request bodies have patient vitals, medical history, examination details, prescriptions. INFO level logs are usually always on in production, so all this patient data is probably sitting in log files or getting shipped to whatever log aggregator they use.

both of these come from the same root issue honestly,heres no @ControllerAdvice or any kind of centralized exception handling. every controller method has its own try-catch doing the exact same thing. thats why the pattern is so consistent across all 20+ controllers.

i'd like to take a shot at fixing this. my plan would be:

  • add a @ControllerAdvice global exception handler
  • refactor one controller (probably GeneralOPDController) to use ResponseEntity and the existing toStringWithHttpStatus() method as a proof of concept
  • remove the raw request body logging and replace it with something that doesnt dump full patient data

not trying to rewrite everything in one go, just start with one controller and see if the approach works.
happy to get reviews and replies???

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions