scalars: set data download filenames on backend, not a tag
#3782
Conversation
… filename and support format=json explicitly
tensorboard/backend/http_util.py
Outdated
| @@ -184,6 +189,8 @@ def Respond( | |||
| headers.append(("X-Content-Type-Options", "nosniff")) | |||
| if content_encoding: | |||
| headers.append(("Content-Encoding", content_encoding)) | |||
| if filename: | |||
| headers.append(("Content-Disposition", 'filename="%s"' % filename)) | |||
There was a problem hiding this comment.
Want attachment; filename=…?
There was a problem hiding this comment.
So I specifically left out attachment since with the current behavior it's possible in Chrome to open the .json file in the browser in a new tab if you want (the .csv is always forcibly downloaded; I think this is content-type specific browser behavior). As a user I like it when it's possible to do this rather than requiring that I always download the file, so that's why I did it this way.
Setting the download attribute in the HTML tag but omitting attachment means that the behavior depends on whether you just click the link (download) or click to open in a new tab (browser dependent, but may stay in the browser), so that seemed like a nice option to preserve.
That said I agree it's overall a bit less consistent to reason about, so if you prefer I'm happy to add attachment.
There was a problem hiding this comment.
That’s fine with me (I also like that behavior). In that case, let’s fix
the newly added docs to not mention Content-Disposition: attachment?
tensorboard/backend/http_util.py
Outdated
| @@ -184,6 +189,8 @@ def Respond( | |||
| headers.append(("X-Content-Type-Options", "nosniff")) | |||
| if content_encoding: | |||
| headers.append(("Content-Encoding", content_encoding)) | |||
| if filename: | |||
| headers.append(("Content-Disposition", 'filename="%s"' % filename)) | |||
There was a problem hiding this comment.
Can we use werkzeug.http.quote_header_value(filename) here?
Callers need to sanitize the logical filename, but shouldn’t need to
know about the RFC 2616 quoted-string grammar.
There was a problem hiding this comment.
Ooh yes, thanks for pointing that out. Done and test added.
| @@ -127,7 +121,7 @@ def index_impl(self, experiment=None): | |||
|
|
|||
| return result | |||
|
|
|||
| def scalars_impl(self, tag, run, experiment, output_format): | |||
| def scalars_impl(self, tag, run, experiment): | |||
There was a problem hiding this comment.
Looks like legit test failure in the custom scalars plugin due to the
removal of this parameter.
There was a problem hiding this comment.
Thanks, will fix shortly.
| mapping[ord(char)] = "_" | ||
| for char in allowed_chars: | ||
| mapping[ord(char)] = char | ||
| print(mapping[ord(" ")]) |
|
|
||
| This discards all characters that are not ASCII letters, digits, or | ||
| punctuation, and then converts any remaining characters that are not in | ||
| [-_=,A-Za-z0-9] to "_". |
There was a problem hiding this comment.
Doc comment includes comma as allowed character, but code doesn’t?
There was a problem hiding this comment.
Great catch, thanks. Updated the code to match docs.
| "punct___________-_____=______________uation", | ||
| ) | ||
| check("w h\ti\nt\re\x0bs\x0cpace", "whitespace") | ||
| check("emo\U0001F95Dji", "emoji") |
There was a problem hiding this comment.
sadly github probably doesn't allow sending arbitrary emoji to their comment react API
|
Update: I'm withdrawing this PR because after some discussion we decided that this isn't a great approach, since in the long term we want the flexibility to be able to generate the data to download wholly on the frontend (e.g. so that it can more easily reflect client-side filtering done by the user, or see also the discussion in #1845) and because it also didn't fix other cases of this problem - e.g. in the custom scalars download URLs (which we'd also have had to change) or in the scalars dashboard "Download chart as SVG" option which also uses a dynamic filename (the tag name) and is generated client-side, making it ineligible for this approach to sidestepping the problem. Instead, after some discussion with the Polymer folks, it turns out they were fine changing Resin's policy for the Note I already broke the still-useful test cleanup from this PR out into #3793. |
Scalar chart data download URLs use custom filenames that include the user's run and tag names, for ease of identification when downloading from multiple charts. This PR moves that custom filename support from the frontend, where it used the
downloadattribute on<a>tags, to the backend, where we use theContent-Dispositionheader.The intent is to sidestep issues with trying to sanitize the strings going into the
downloadattribute so that they aren't intercepted by Polymer Resin; the easiest way I determined to do that was to stop binding to the attribute entirely. It's a little less elegant in that if we had multiple download widgets for various charts, now each backend location must set the filename rather than having it centralized in the UI component, but right now it's only used in one place anyway and it doesn't seem too onerous even if we had multiple places.In addition, I actually still implemented some sanitization of the attribute, to eliminate or replace with
_all characters other than[-_=A-Za-z0-9]. Chrome's auto-downloading behavior already seems to do something fairly similar (using_to replace/, for instance) but it seemed safest to ensure we're generating innocuous filenames in the first place.Lastly, the scalar plugin tests for this were a mess, so I unrolled and flattened them a fair bit and added two new tests that explicitly test the
format=URL behavior end-to-end.(The first commit on this PR is split out as #3781 since it includes an unrelated behavior change.)