atuttle / Taffy

:candy: The REST Web Service framework for ColdFusion and Lucee
http://taffy.io
Other
226 stars 117 forks source link

Add new isFlushed & addHeader functions #404

Closed JamoCA closed 2 years ago

JamoCA commented 3 years ago

Adding headers post-flush causes a CFError. Sometimes a flush is automatically performed when the generated content exceeds the max buffer settings. Instead of throwing an error when adding non-status headers, I thought it was more critical to return the content without erroring.

JamoCA commented 3 years ago

The isFlushed function may need to be updated. I made some changes and here's where I maintain that UDF. https://gist.github.com/JamoCA/575182d2b2378ad57334e080d9a31209

boolean function isFlushed(){
    var response = false;
    var headers = {};
    if (StructKeyExists(request, "isFlushedAlready") AND isValid("boolean", request.isFlushedAlready) and request.isFlushedAlready){
        return true;
    }
    headers = GetHttpRequestData(false).headers;
    if (StructKeyExists(headers, "Expect") or not len(CGI.HTTP_Connection)){
        response = true;
    } else if (StructKeyExists(Server, "lucee")){
        response = getPageContext().getHttpServletResponse().isCommitted() or getPageContext().getHttpServletResponse().isTreatAsCommitted();
    } else {
        response = getPageContext().getResponse().isCommitted() or getPageContext().getFusionContext().getResponse().isOutputAutoFlushed();
    }
    request.isFlushedAlready = response;
    return response;
}
timmixell commented 3 years ago

@JamoCA we've actually run into this very issue, but I have not had time to dig into it. I'm not sure why the build is failing, but I'm also not seeing unit tests in support of your changes. I believe changes that don't have accompanying tests are generally frowned upon (I know from experience).

I'm not sure what @atuttle or @pfreitag would expect in the scenario where this comes up, but I will approve this once you've got tests in place, and the build is clearing.

Thanks!

atuttle commented 3 years ago

Test cases for new code are strongly preferred. It does look as if the testing infrastructure might need a tune-up though. It may have rotted into a non-functioning state.