Fix cat empty file (issue #510)

[albu-diku]

Address attempting to concatenate strings to a an empty byte string in the file_format output function.

The added tests here make use of changes to migwsgi that make it possible to exercise the entire output path. Bring those changes over because having to write them again has no benefit.

[jonasbardino]

I think this fix needs core adjustments and thorough testing in actual deployment. In that relation please also remember that the file_format function and output_format of file is not just used in cat.py but also e.g. in showfreezefile.py and showvgridprivatefile.py in addition to head.py and tail.py.

The added unit test appears to only cover the specific plain text error case and AFAICT none of the tests cover the important default case of serving binary data for file output. I would consider that an essential addition to avoid regressions in the basic use cases.

[albu-diku]

I think this fix needs core adjustments and thorough testing in actual deployment. In that relation please also remember that the file_format function and output_format of file is not just used in cat.py but also e.g. in showfreezefile.py and showvgridprivatefile.py in addition to head.py and tail.py.

The added unit test appears to only cover the specific plain text error case and AFAICT none of the tests cover the important default case of serving binary data for file output. I would consider that an essential addition to avoid regressions in the basic use cases.

Ok, I was hoping to iterate upon it off the back of your feedback. Perhaps this should have been a draft, but wasn’t sure it’d then get looked over. Feel I need to stress I was just tying to help here.

I have reworked this based on a pre-existing render_text flag but could use confirmation that it can be relied upon for this purpose - would seem it would only work correctly if codepaths triggering errors reliably reset the output content type. The negative test case confirms it is done in the error case, but if you have any suggestions for further belts and braces let me know. The positive case confirms a successful response with the example GIF data and content type of image/gif.

[jonasbardino]

I think this fix needs core adjustments and thorough testing in actual deployment. In that relation please also remember that the file_format function and output_format of file is not just used in cat.py but also e.g. in showfreezefile.py and showvgridprivatefile.py in addition to head.py and tail.py.
The added unit test appears to only cover the specific plain text error case and AFAICT none of the tests cover the important default case of serving binary data for file output. I would consider that an essential addition to avoid regressions in the basic use cases.

Ok, I was hoping to iterate upon it off the back of your feedback. Perhaps this should have been a draft, but wasn’t sure it’d then get looked over. Feel I need to stress I was just tying to help here.

Help is welcome of course. Yet, when I find a PR with a title of 'Fix something' with no markers or mention of it being work-in-progress or not fully tested, it's very difficult not to expect it to be merge-ready. PR titles and Descriptions are usually helpful to clarify what to expect.
I've also added a couple of new labels to help in such situations. Please have a look at the updated https://github.com/ucphhpc/migrid-sync/wiki/CodeTestingGuide and remember that PRs like this, which modify those core modules require deployment testing as described there.

[jonasbardino]

I think this fix needs core adjustments and thorough testing in actual deployment. In that relation please also remember that the file_format function and output_format of file is not just used in cat.py but also e.g. in showfreezefile.py and showvgridprivatefile.py in addition to head.py and tail.py.
The added unit test appears to only cover the specific plain text error case and AFAICT none of the tests cover the important default case of serving binary data for file output. I would consider that an essential addition to avoid regressions in the basic use cases.

...

I have reworked this based on a pre-existing render_text flag but could use confirmation that it can be relied upon for this purpose - would seem it would only work correctly if codepaths triggering errors reliably reset the output content type. The negative test case confirms it is done in the error case, but if you have any suggestions for further belts and braces let me know. The positive case confirms a successful response with the example GIF data and content type of image/gif.

I have a bunch of other matters to tend to now but will return to have a look next time I can find some time. In the meantime I'd urge you to look into signing of your git commits (see our internal wiki) and proceed with the mandatory deployment testing.

[albu-diku]

It was not my intention to cause frustration with this work. I don't think I currently have the bandwidth for this either when also taking care of the large active review, thus I will at minimum drop this to a draft and pick this up when I have more spare capacity.