Bypass blob storage in UploadArtifact(Closes #1704) #1717
Conversation
| return await dstStorage.putFile(filename, content); | ||
| }; | ||
|
|
||
| UploadArtifact.prototype.getAssetNameFromHash = async function(hash){ |
There was a problem hiding this comment.
One Question on this is should we remove any dot extensions from the file? Should it happen on BrowserAssetWidget?
There was a problem hiding this comment.
I would probably leave the file as-is.
brollb
left a comment
brollb
left a comment
There was a problem hiding this comment.
This is a good start!
The biggest issue so far is that the new widget has a peer dependency in the sense that it expects there to be a "storage" config setting. This setting should be independent of others as this would be difficult to debug (and be somewhat annoying :) ).
Ideally, the current widget would actually create a storage widget itself. When getting the value, it should first get the value of the storage widget and then upload the data to the given storage. Upon a successful upload, it should return the {name, dataInfo} as you have already done.
The cleanest implementation will, unfortunately, require some refactoring of some of the custom types that I added here. I would recommend moving "dict" types to their own custom type. They can probably still be directly loaded by the ConfigDialog as they are a very generic type. Using this type, we should probably make an official type for storage which essentially creates this.
After these have been moved to their own custom widgets, the widget for user assets should be pretty easy to create by mostly just composing these existing types.
src/common/viz/ConfigDialog.js
Outdated
| this._btnSave.on('click', event => { | ||
| this.submit(deferred.resolve); | ||
| this._btnSave.on('click', async event => { | ||
| await this.submit(deferred.resolve); |
There was a problem hiding this comment.
hmm.. Does jquery support async event handlers correctly here? Maybe we should move event.stopPropagation() and event.preventDefault() to be called first.
The issue I am concerned about is that I am not sure that the underlying implementation is something like this:
let target = clickedItem;
let handled = false;
while (!handled) {
target.onclick(event);
target = target.parent();
handled = event.propagate || !!target; // assuming `stopPropagation` toggles a flag, `propagate`
}In the above case, the event would still propagate until the async function completes.
src/common/viz/ConfigDialog.js
Outdated
| ConfigDialog.prototype._getAllConfigValues = async function () { | ||
| var settings = {}; | ||
|
|
||
| const basedir = `${this._client.getActiveProjectId()}/artifacts`; |
There was a problem hiding this comment.
This should only be in the BrowserAssetWidget
| @@ -0,0 +1,7 @@ | |||
| [ | |||
| { | |||
| "name" : "BrowserAssetWidget", | |||
There was a problem hiding this comment.
As a minor point, I would probably rename this since all widgets are only used in the browser.
| return await dstStorage.putFile(filename, content); | ||
| }; | ||
|
|
||
| UploadArtifact.prototype.getAssetNameFromHash = async function(hash){ |
There was a problem hiding this comment.
I would probably leave the file as-is.
| return await dstStorage.putFile(filename, content); | ||
| }; | ||
|
|
||
| UploadArtifact.prototype.getAssetNameFromHash = async function(hash){ |
There was a problem hiding this comment.
We should be able to remove the getAssetNameFromHash function now.
| } | ||
|
|
||
| async beforeSubmit(basedir, config) { | ||
| const storageConfig = findByKey(config, 'storage'); |
There was a problem hiding this comment.
Ideally, I would actually include the storage config settings in this widget as it shouldn't reach outside of itself to look up peer config values.
| @@ -0,0 +1,7 @@ | |||
| [ | |||
There was a problem hiding this comment.
In terms of keeping this logic generic so it can more easily become a standalone library, it would be better to implement using component settings.
This would allow the configuration for it to be specified here and projects could define custom widgets - much like we are doing in this PR.
brollb
left a comment
brollb
left a comment
There was a problem hiding this comment.
A couple minor changes but it is looking good!
I also noticed that if there is an error with the upload, it isn't handled gracefully. This could be saved for another PR but also might not be bad to address here. The issue can be reproduced by switching to the "Temporary" volume pool without changing the volume from "/deepforge_data". The following error should be thrown:
Then the dialog will just hang with the loading icon:
| filename: filename, | ||
| volume: this.volume, | ||
| size: content.byteLength, | ||
| size: content.byteLength || content.size, |
There was a problem hiding this comment.
Because File or Blob doesn't have attribute byteLength but size. You can still create an ArrayBuffer during the upload from the widget, but its not needed.
| ASSET_WIDGET_BASE = $('<div class="asset-widget" />'), | ||
| ASSET_LINK = $('<a class="local-download-link" href="" target="_blank"></a>'); | ||
|
|
||
| class BrowserAssetWidget extends WidgetBase { |
There was a problem hiding this comment.
Since this always runs in the browser, it might be better to name it something like FileWidget since it appears to just be a widget that provides a file input (at least that is the core functionality).
There was a problem hiding this comment.
Okay. This should have been addressed from the initial review. By browser BrowserAssetWidget, the intention was to make it clear that it is like the AssetWidget from upstream but rather that asset should be handled in the browser rather than via BlobClient.
There was a problem hiding this comment.
ah, that makes more sense. I still have a slight preference for something like FileWidget since I think it is a little clearer but I think we can leave it if you want.
No description provided.