[SVCS-678] Add noop for export (Functionality Only) #334

NyanHelsing · 2018-04-26T17:55:36Z

Ticket

https://openscience.atlassian.net/browse/SVCS-678

Purpose

What

When MFR receives an export request where the input and output formats are the same, it should simply return the original file unmodified... unless there's another reason to export the file (i.e. image scaling).

Why

A no-op is a simple operation to support, why not do it?

Acceptance criteria

A PR to support

QA notes

Devs will test this. Dev, please add a list of affected and unaffected file types.

this affects all file types. Any file type may now be exported. The extension of the file must match exactly the extension of the export, or we respond saying we cant export it. (i.e .jpg and .100*100.jpg are not the same, and the 100*100.jpg will trigger a actual file conversion)

Build an MFR export url requesting to export a file of type foo as type foo. This should start a download of the raw file without modification. Exception: image files should respect scaling parameters.

coveralls · 2018-04-26T18:16:30Z

Coverage decreased (-0.2%) to 71.117% when pulling 072c3e0 on birdbrained:ft/noop-only into 681a0ce on CenterForOpenScience:develop.

cslzchen

Simple but elegant fix 🎆 with a few 🐞 .

Be careful about case sensitivity. I had the doubt, which was proven during local testing.
Don't forget to await on async methods/functions.
Be more specific on logs, which help us debug.

Back to Add'l Dev 🔥 @birdbrained ~

cslzchen · 2018-04-26T18:01:49Z

mfr/server/handlers/export.py

        """Export a file to the format specified via the associated extension library"""

+        # File is already in the requested format
+        if self.metadata.ext == ".{}".format(self.format):


Double check and .lower() the case if necessary.

Here is the result during my testing with extra logger.info():

Wrong

Exported with conversion: .PDF .pdf

Correct

Exported with conversion: .JPG .2400x2400.jpeg

cslzchen · 2018-04-26T18:09:25Z

mfr/server/handlers/export.py


+        # File is already in the requested format
+        if self.metadata.ext == ".{}".format(self.format):
+            await self.write_stream(self.provider.download())


FYI, my first thought is to add a try here but then I realize the failure should be thrown if download fails.

download() needs to be awaited. Here is the failure:

mfr_1 | WARNING:root:No '/root/.cos/mfr-test.json' configuration file found mfr_1 | [2018-04-26 18:38:51,921][WARNING][root]: No '/root/.cos/waterbutler-test.json' configuration file found mfr_1 | [2018-04-26 18:38:51,930][INFO][raven.contrib.tornado.AsyncSentryClient]: Raven is not configured (logging is disabled). Please see the documentation for more information. mfr_1 | [2018-04-26 18:38:51,935][INFO][mfr.server.app]: Listening on 0.0.0.0:7778 mfr_1 | [2018-04-26 18:39:05,366][ERROR][tornado.application]: [User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36] Uncaught exception GET /export?url=http://192.168.168.167:5000/8d9yv/?action=download%26mode=render%26direct%26public_file=False&initialWidth=848&childId=mfrIframe&parentTitle=OSF%20%7C%20test.pdf&parentUrl=http%3A%2F%2Flocalhost%3A5000%2F8d9yv%2F&format=pdf (172.18.0.1) mfr_1 | Traceback (most recent call last): mfr_1 | File "/usr/local/lib/python3.5/site-packages/tornado/web.py", line 1445, in _execute mfr_1 | result = yield result mfr_1 | File "/usr/local/lib/python3.5/site-packages/tornado/gen.py", line 1008, in run mfr_1 | value = future.result() mfr_1 | File "/usr/local/lib/python3.5/site-packages/tornado/concurrent.py", line 232, in result mfr_1 | raise_exc_info(self._exc_info) mfr_1 | File "<string>", line 3, in raise_exc_info mfr_1 | File "/usr/local/lib/python3.5/site-packages/tornado/gen.py", line 282, in wrapper mfr_1 | yielded = next(result) mfr_1 | File "<string>", line 6, in _wrap_awaitable mfr_1 | File "/code/mfr/server/handlers/export.py", line 63, in get mfr_1 | await self.write_stream(self.provider.download()) mfr_1 | File "/code/mfr/server/handlers/core.py", line 140, in write_stream mfr_1 | chunk = await stream.read(settings.CHUNK_SIZE) mfr_1 | AttributeError: 'coroutine' object has no attribute 'read' mfr_1 | [2018-04-26 18:39:05,374][INFO][tornado.access]: 172.18.0.1 - - [26/Apr/2018:18:39:05 +0800] "GET /export?url=http://192.168.168.167:5000/8d9yv/?action=download%26mode=render%26direct%26public_file=False&initialWidth=848&childId=mfrIframe&parentTitle=OSF%20%7C%20test.pdf&parentUrl=http%3A%2F%2Flocalhost%3A5000%2F8d9yv%2F&format=pdf HTTP/1.1" - - "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36"

And here is the correct behavior after my temporary fix:

mfr_1 | WARNING:root:No '/root/.cos/mfr-test.json' configuration file found mfr_1 | [2018-04-26 18:52:16,412][WARNING][root]: No '/root/.cos/waterbutler-test.json' configuration file found mfr_1 | [2018-04-26 18:52:16,423][INFO][raven.contrib.tornado.AsyncSentryClient]: Raven is not configured (logging is disabled). Please see the documentation for more information. mfr_1 | [2018-04-26 18:52:16,432][INFO][mfr.server.app]: Listening on 0.0.0.0:7778 mfr_1 | Exported with no conversion: .pdf mfr_1 | [2018-04-26 18:52:53,903][INFO][tornado.access]: 172.18.0.1 - - [26/Apr/2018:18:52:53 +0800] "GET /export?url=http://192.168.168.167:5000/8d9yv/?action=download%26mode=render%26direct%26public_file=False&initialWidth=848&childId=mfrIframe&parentTitle=OSF%20%7C%20test.pdf&parentUrl=http%3A%2F%2Flocalhost%3A5000%2F8d9yv%2F&format=pdf HTTP/1.1" - - "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36"

cslzchen · 2018-04-26T18:09:38Z

mfr/server/handlers/export.py

+        # File is already in the requested format
+        if self.metadata.ext == ".{}".format(self.format):
+            await self.write_stream(self.provider.download())
+            logger.info('Exported with no conversion.')


Add the extension name so that we know which type it is. This also helps us identify bugs easily.

cslzchen · 2018-04-26T19:23:00Z

For now, adding/updating new unit tests is not required since we will have a refactoring PR right afterwards.

NyanHelsing · 2018-04-26T21:29:44Z

Ok commits squashed I think this may be ready to go.

cslzchen

LGTM and move to RTM for @felliott 🎆

[SVCS-678] Closes: #334

cslzchen requested changes Apr 26, 2018

View reviewed changes

cslzchen added the Add'l Dev label Apr 26, 2018

NyanHelsing force-pushed the ft/noop-only branch from 194d84b to 7c32a2c Compare April 26, 2018 20:24

cslzchen approved these changes Apr 27, 2018

View reviewed changes

cslzchen added Ready To Merge and removed Add'l Dev labels Apr 27, 2018

NyanHelsing force-pushed the ft/noop-only branch from 7c32a2c to 72b4e66 Compare May 4, 2018 20:12

Add noop for export

072c3e0

NyanHelsing force-pushed the ft/noop-only branch from 72b4e66 to 072c3e0 Compare May 4, 2018 20:37

cslzchen merged commit 072c3e0 into CenterForOpenScience:develop May 4, 2018

cslzchen added a commit that referenced this pull request May 4, 2018

Merge branch 'fix/noop-export' into develop

792d7b6

[SVCS-678] Closes: #334

cslzchen removed the Ready To Merge label May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SVCS-678] Add noop for export (Functionality Only) #334

[SVCS-678] Add noop for export (Functionality Only) #334

Uh oh!

NyanHelsing commented Apr 26, 2018 •

edited by cslzchen

Loading

Uh oh!

coveralls commented Apr 26, 2018 •

edited

Loading

Uh oh!

cslzchen left a comment •

edited

Loading

Uh oh!

cslzchen Apr 26, 2018

Uh oh!

cslzchen Apr 26, 2018

Uh oh!

cslzchen Apr 26, 2018

Uh oh!

cslzchen Apr 26, 2018 •

edited

Loading

Uh oh!

cslzchen Apr 26, 2018

Uh oh!

cslzchen commented Apr 26, 2018

Uh oh!

NyanHelsing commented Apr 26, 2018

Uh oh!

cslzchen left a comment

Uh oh!

Uh oh!

[SVCS-678] Add noop for export (Functionality Only) #334

[SVCS-678] Add noop for export (Functionality Only) #334

Uh oh!

Conversation

NyanHelsing commented Apr 26, 2018 • edited by cslzchen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

Purpose

What

Why

Acceptance criteria

QA notes

Uh oh!

coveralls commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cslzchen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cslzchen Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

cslzchen Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

cslzchen Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

cslzchen Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cslzchen Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

cslzchen commented Apr 26, 2018

Uh oh!

NyanHelsing commented Apr 26, 2018

Uh oh!

cslzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NyanHelsing commented Apr 26, 2018 •

edited by cslzchen

Loading

coveralls commented Apr 26, 2018 •

edited

Loading

cslzchen left a comment •

edited

Loading

cslzchen Apr 26, 2018 •

edited

Loading