Skip to content

Bypass blob storage in UploadArtifact(Closes #1704) #1717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brollb merged 7 commits into from
Jul 31, 2020
Merged

Bypass blob storage in UploadArtifact(Closes #1704) #1717

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
14e5667
Bypass blob storage in UploadArtifact(Closes #1704)
umesh-timalsina May 21, 2020
7df7dab
WIP- Fix eslinting issues
umesh-timalsina May 21, 2020
36cf80d
WIP-Refactor CustomConfigEntries, Address PR suggestions
umesh-timalsina Jul 28, 2020
03eb659
Merge remote-tracking branch 'origin/master' into 1704-bypass-blob-st…
umesh-timalsina Jul 28, 2020
71a7bf1
WIP- add spinner; fix unused imports
umesh-timalsina Jul 28, 2020
430674b
WIP- Gracefully handle dialog errors, rename BrowserAssetWidget->File…
umesh-timalsina Jul 29, 2020
5529cf6
Fix widget name in components.json, Use toast to show errors
umesh-timalsina Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 43 additions & 9 deletions src/common/viz/ConfigDialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ define([
'js/Dialogs/PluginConfig/PluginConfigDialog',
'deepforge/utils',
'text!js/Dialogs/PluginConfig/templates/PluginConfigDialog.html',
'text!./CustomConfigWidgets.json',
'css!./ConfigDialog.css'
], function(
Q,
PluginConfigDialog,
utils,
pluginConfigDialogTemplate,
CustomWidgets
) {
var SECTION_DATA_KEY = 'section',
ATTRIBUTE_DATA_KEY = 'attribute',
Expand All @@ -20,36 +22,56 @@ define([
DESCRIPTION_BASE = $('<div class="desc muted col-sm-8"></div>'),
SECTION_HEADER = $('<h6 class="config-section-header">');

const CUSTOM_WIDGETS = JSON.parse(CustomWidgets);

var ConfigDialog = function(client) {
PluginConfigDialog.call(this, {client: client});
this._widgets = {};
this.imported = this._registerCustomWidgets(CUSTOM_WIDGETS);
};

ConfigDialog.prototype = Object.create(PluginConfigDialog.prototype);

ConfigDialog.prototype._registerCustomWidgets = async function(customWidgets) {
const self = this;

const promises = customWidgets.map(widgetInfo => {
return new Promise((resolve, reject) => {
require([widgetInfo.path], function(customWidget) {
const targetType = widgetInfo.type;
self._propertyGridWidgetManager
.registerWidgetForType(targetType, customWidget);
resolve();
}, reject);
});
});

return Promise.all(promises);
};

ConfigDialog.prototype.show = async function(pluginMetadata, options={}) {
const deferred = Q.defer();

this._pluginMetadata = pluginMetadata;
const prevConfig = await this.getSavedConfig();
await this.imported;
this._initDialog(pluginMetadata, prevConfig, options);

this._dialog.on('shown', () => {
this._dialog.find('input').first().focus();
});

this._btnSave.on('click', event => {
this.submit(deferred.resolve);
this._btnSave.on('click', async event => {
await this.submit(deferred.resolve);
Copy link
Contributor

@brollb brollb Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

event.stopPropagation();
event.preventDefault();
});

//save&run on CTRL + Enter
this._dialog.on('keydown.PluginConfigDialog', event => {
this._dialog.on('keydown.PluginConfigDialog', async event => {
if (event.keyCode === 13 && (event.ctrlKey || event.metaKey)) {
event.stopPropagation();
event.preventDefault();
this.submit(deferred.resolve);
await this.submit(deferred.resolve);
}
});
this._dialog.modal('show');
Expand All @@ -63,6 +85,7 @@ define([
this._divContainer = this._dialog.find('.modal-body');
this._saveConfigurationCb = this._dialog.find('.save-configuration');
this._modalHeader = this._dialog.find('.modal-header');
this._modalFooter = this._dialog.find('.modal-footer');
this._saveConfigurationCb.find('input').prop('checked', true);

// Create the header
Expand All @@ -85,8 +108,9 @@ define([
};
};

ConfigDialog.prototype.submit = function (callback) {
const config = this._getAllConfigValues();
ConfigDialog.prototype.submit = async function (callback) {
this._btnSave.attr('disabled', true);
const config = await this._getAllConfigValues();
const saveConfig = this._saveConfigurationCb.find('input')
.prop('checked');

Expand Down Expand Up @@ -159,9 +183,9 @@ define([
return response.status < 399 ? await response.json() : null;
};

ConfigDialog.prototype._getAllConfigValues = function () {
ConfigDialog.prototype._getAllConfigValues = async function () {
var settings = {};

const basedir = `${this._client.getActiveProjectId()}/artifacts`;
Copy link
Contributor

@brollb brollb Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be in the BrowserAssetWidget

Object.keys(this._widgets).forEach(namespace => {
settings[namespace] = {};

Expand All @@ -170,6 +194,16 @@ define([
});
});

// Second pass necessary to resolve value from functions
for (const namespace of Object.keys(this._widgets)){
for(const name of Object.keys(this._widgets[namespace])){
if(this._widgets[namespace][name].beforeSubmit){
await this._widgets[namespace][name].beforeSubmit(basedir, settings);
settings[namespace][name] = this._widgets[namespace][name].getValue();
}
}
}

return settings;
};

Expand Down
7 changes: 7 additions & 0 deletions src/common/viz/CustomConfigWidgets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[
Copy link
Contributor

@brollb brollb Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
"name" : "BrowserAssetWidget",
Copy link
Contributor

@brollb brollb Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a minor point, I would probably rename this since all widgets are only used in the browser.

"type": "userAsset",
"path" : "deepforge/viz/widgets/BrowserAssetWidget"
}
]
155 changes: 155 additions & 0 deletions src/common/viz/widgets/BrowserAssetWidget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
/*globals define, $, WebGMEGlobal*/
define([
'js/Controls/PropertyGrid/Widgets/WidgetBase',
'js/logger',
'deepforge/storage/index'
], function (WidgetBase,
Logger,
Storage) {
'use strict';

const BTN_ATTACH = $('<a class="btn btn-mini btn-dialog-open"><i class="glyphicon glyphicon-file"/></a>'),
INPUT_FILE_UPLOAD = $('<input type="file" />'),
ASSET_WIDGET_BASE = $('<div class="asset-widget" />'),
ASSET_LINK = $('<a class="local-download-link" href="" target="_blank"></a>');

class BrowserAssetWidget extends WidgetBase {
Copy link
Contributor

@brollb brollb Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@umesh-timalsina umesh-timalsina Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@brollb brollb Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

constructor(propertyDesc) {
super(propertyDesc);
if (propertyDesc.readOnly) {
this._alwaysReadOnly = true;
}
this.file = null;

this.logger = Logger.create('deepforge:viz:widgets:BrowserAssetWidget',
WebGMEGlobal.gmeConfig.client.log);

this.parentEl = ASSET_WIDGET_BASE.clone();
this.el.append(this.parentEl);

this.assetLink = ASSET_LINK.clone();
this.parentEl.append(this.assetLink);

this.fileDropTarget = this.parentEl;

this.btnAttach = BTN_ATTACH.clone();
this.parentEl.append(this.btnAttach);

this.fileUploadInput = INPUT_FILE_UPLOAD.clone();

this.attachFileDropHandlers();
this.updateDisplay();
}

attachFileDropHandlers() {
this.btnAttach.on('click', e => {
e.stopPropagation();
e.preventDefault();

this.fileUploadInput.click();
});

this.fileUploadInput.on('change', e => {
e.stopPropagation();
e.preventDefault();
this.fileSelectHandler(e.originalEvent);
});

this.fileDropTarget.on('dragover', e => {
e.stopPropagation();
e.preventDefault();
});

this.fileDropTarget.on('dragenter', e => {
e.stopPropagation();
e.preventDefault();
this.fileDropTarget.addClass('hover');
});

this.fileDropTarget.on('dragleave', e => {
e.stopPropagation();
e.preventDefault();
this.fileDropTarget.removeClass('hover');
});

this.fileDropTarget.on('drop', e => {
e.stopPropagation();
e.preventDefault();
this.fileDropTarget.removeClass('hover');
this.fileSelectHandler(e.originalEvent);
});
}

fileSelectHandler(event){
const files = event.target.files || event.dataTransfer.files;
if(files){
this.setFile(files[0]);
}
}

setFile(file) {
this.file = file;
this.assetLink.text(this.file.name);
this.assetLink.attr('href', URL.createObjectURL(this.file));
}

detachFileHandlers(){
this.fileUploadInput.off('change');

this.fileDropTarget.off('dragover');
this.fileDropTarget.off('dragenter');
this.fileDropTarget.off('dragleave');
this.fileDropTarget.off('drop');

this.btnAttach.off('click');
}

updateDisplay() {
if(this.file){
this.setFile(this.file);
}
super.updateDisplay();
}

async beforeSubmit(basedir, config) {
const storageConfig = findByKey(config, 'storage');
Copy link
Contributor

@brollb brollb Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const backendId = this.resolveBackendId(storageConfig);
if(backendId){
const backend = Storage.getBackend(backendId);
const tgtStorageClient = await backend.getClient(this.logger, storageConfig.config);
const path = `${basedir}/${this.file.name}`;
const dataInfo = await tgtStorageClient.putFile(path, this.file);
this.setValue({name: this.file.name, dataInfo: dataInfo});
}
}

resolveBackendId(config){
if(config.name){
const storageId = Storage.getAvailableBackends()
.map(id => Storage.getStorageMetadata(id))
.filter(metadata => metadata.name === config.name)
.map(metadata => metadata.id);
return storageId.pop();
}
}

destroy() {
this.detachFileHandlers();
super.destroy();
}
}

function findByKey(obj, key) {
if(Object.keys(obj).includes(key)){
return obj[key];
} else {
for(const childKey of Object.keys(obj)){
if(typeof obj[childKey] === 'object'){
return findByKey(obj[childKey], key);
}
}
}
}

return BrowserAssetWidget;
});
17 changes: 2 additions & 15 deletions src/plugins/UploadArtifact/UploadArtifact.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,12 @@ define([
*/
UploadArtifact.prototype.main = async function (callback) {
const config = this.getCurrentConfig();
const hash = config.dataHash;
const {name, dataInfo} = config.assetInfo;
const baseName = config.dataTypeId;

try {
this.ensureCompatibleMeta();
const name = await this.getAssetNameFromHash(hash) ||
baseName[0].toLowerCase() + baseName.substring(1);
const assetInfo = await this.transfer(hash, config.storage, name);
await this.createArtifact({type: baseName, name: name, data: assetInfo});
await this.createArtifact({type: baseName, name: name, data: dataInfo});
await this.save(`Uploaded "${name}" data`);
this.result.setSuccess(true);
callback(null, this.result);
Expand All @@ -69,16 +66,6 @@ define([
}
};

UploadArtifact.prototype.transfer = async function (hash, storage, name) {
const filename = `${this.projectId}/artifacts/${name}`;
const gmeStorageClient = await Storage.getBackend('gme').getClient(this.logger);
const dataInfo = gmeStorageClient.createDataInfo(hash);
const content = await gmeStorageClient.getFile(dataInfo);

const {id, config} = storage;
const dstStorage = await Storage.getBackend(id).getClient(this.logger, config);
return await dstStorage.putFile(filename, content);
};

UploadArtifact.prototype.getAssetNameFromHash = async function(hash){
Copy link
Contributor Author

@umesh-timalsina umesh-timalsina May 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One Question on this is should we remove any dot extensions from the file? Should it happen on BrowserAssetWidget?

Copy link
Contributor

@brollb brollb Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably leave the file as-is.

Copy link
Contributor

@brollb brollb Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to remove the getAssetNameFromHash function now.

const metadata = await this.blobClient.getMetadata(hash);
Expand Down
8 changes: 4 additions & 4 deletions src/plugins/UploadArtifact/metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
{
"id": "UploadArtifact",
"name": "Upload Artifact",
"version": "0.2.0",
"version": "0.3.0",
"description": "",
"icon": {
"class": "glyphicon glyphicon-cloud-upload",
"src": ""
},
"disableServerSideExecution": false,
"disableServerSideExecution": true,
"disableBrowserSideExecution": false,
"writeAccessRequired": true,
"configStructure": [
{
"name": "dataHash",
"name": "assetInfo",
"displayName": "Data to upload",
"description": "",
"value": "",
"valueType": "asset",
"valueType": "userAsset",
"readOnly": false
},
{
Expand Down