short circuit the mapping operations for invalid path/hash

assignment-client/src/assets/AssetServer.cpp

@@ -604,7 +604,7 @@ bool AssetServer::deleteMappings(const AssetPathList& paths) {
 }
 
 bool AssetServer::renameMapping(const AssetPath& oldPath, const AssetPath& newPath) {
-    if (oldPath[0] != '/' || newPath[0] != '/') {
+    if (!isValidPath(oldPath) || !isValidPath(newPath)) {
         qWarning() << "Cannot perform rename with invalid paths - both should have leading forward slashes:"
             << oldPath << "=>" << newPath;
 

interface/resources/qml/AssetServer.qml

@@ -33,7 +33,8 @@ Window {
     HifiConstants { id: hifi }
 
     property var scripts: ScriptDiscoveryService;
-    property var assetMappingsModel: Assets.proxyModel;
+    property var assetProxyModel: Assets.proxyModel;
+    property var assetMappingsModel: Assets.mappingModel;
     property var currentDirectory;
     property alias currentFileUrl: fileUrlTextField.text;
 
@@ -44,7 +45,10 @@ Window {
         property alias directory: root.currentDirectory
     }
 
-    Component.onCompleted: reload()
+    Component.onCompleted: {
+        assetMappingsModel.errorGettingMappings.connect(handleGetMappingsError)
+        reload()
+    }
 
     function doDeleteFile(path) {
         console.log("Deleting " + path);
@@ -103,7 +107,7 @@ Window {
 
     function canAddToWorld() {
         var supportedExtensions = [/\.fbx\b/i, /\.obj\b/i];
-        var path = assetMappingsModel.data(treeView.currentIndex, 0x100);
+        var path = assetProxyModel.data(treeView.currentIndex, 0x100);
 
         return supportedExtensions.reduce(function(total, current) {
             return total | new RegExp(current).test(path);
@@ -114,6 +118,12 @@ Window {
         print("reload");
         Assets.mappingModel.refresh();
     }
+
+    function handleGetMappingsError() {
+        errorMessage("There was a problem retreiving the list of assets from your Asset Server.\n"
+                     + "Please make sure you are connected to the Asset Server and try again. ");
+    }
+
     function addToWorld() {
         var url = assetMappingsModel.data(treeView.currentIndex, 0x102);
         if (!url) {
@@ -124,7 +134,7 @@ Window {
     }
 
     function copyURLToClipboard() {
-        var path = assetMappingsModel.data(treeView.currentIndex, 0x103);
+        var path = assetProxyModel.data(treeView.currentIndex, 0x103);
         if (!path) {
             return;
         }
@@ -132,7 +142,7 @@ Window {
     }
 
     function renameFile() {
-        var path = assetMappingsModel.data(treeView.currentIndex, 0x100);
+        var path = assetProxyModel.data(treeView.currentIndex, 0x100);
         if (!path) {
             return;
         }
@@ -156,12 +166,12 @@ Window {
         });
     }
     function deleteFile() {
-        var path = assetMappingsModel.data(treeView.currentIndex, 0x100);
+        var path = assetProxyModel.data(treeView.currentIndex, 0x100);
         if (!path) {
             return;
         }
 
-        var isFolder = assetMappingsModel.data(treeView.currentIndex, 0x101);
+        var isFolder = assetProxyModel.data(treeView.currentIndex, 0x101);
         var typeString = isFolder ? 'folder' : 'file';
 
         var object = desktop.messageBox({
@@ -195,7 +205,7 @@ Window {
         var fileUrl = fileUrlTextField.text
         var addToWorld = addToWorldCheckBox.checked
 
-        var path = assetMappingsModel.data(treeView.currentIndex, 0x100);
+        var path = assetProxyModel.data(treeView.currentIndex, 0x100);
         var directory = path ? path.slice(0, path.lastIndexOf('/') + 1) : "";
         var filename = fileUrl.slice(fileUrl.lastIndexOf('/') + 1);
 
@@ -297,7 +307,7 @@ Window {
             HifiControls.Tree {
                 id: treeView
                 height: 400
-                treeModel: assetMappingsModel
+                treeModel: assetProxyModel
                 colorScheme: root.colorScheme
                 anchors.left: parent.left
                 anchors.right: parent.right

libraries/networking/src/AssetClient.cpp

@@ -138,6 +138,7 @@ bool haveAssetServer() {
     
     return true;
 }
+
 GetMappingRequest* AssetClient::createGetMappingRequest(const AssetPath& path) {
     return new GetMappingRequest(path);
 }
@@ -159,8 +160,8 @@ RenameMappingRequest* AssetClient::createRenameMappingRequest(const AssetPath& o
 }
 
 AssetRequest* AssetClient::createRequest(const AssetHash& hash) {
-    if (hash.length() != SHA256_HASH_HEX_LENGTH) {
-        qCWarning(asset_client) << "Invalid hash size";
+    if (isValidHash(hash)) {
+        qCWarning(asset_client) << "Invalid hash";
         return nullptr;
     }
 
@@ -364,9 +365,10 @@ bool AssetClient::getAssetMapping(const AssetPath& path, MappingOperationCallbac
         _pendingMappingRequests[assetServer][messageID] = callback;
 
         return true;
+    } else {
+        callback(false, AssetServerError::NoError, QSharedPointer<ReceivedMessage>());
+        return false;
     }
-
-    return false;
 }
 
 bool AssetClient::getAllAssetMappings(MappingOperationCallback callback) {
@@ -386,9 +388,10 @@ bool AssetClient::getAllAssetMappings(MappingOperationCallback callback) {
         _pendingMappingRequests[assetServer][messageID] = callback;
 
         return true;
+    } else {
+        callback(false, AssetServerError::NoError, QSharedPointer<ReceivedMessage>());
+        return false;
     }
-
-    return false;
 }
 
 bool AssetClient::deleteAssetMappings(const AssetPathList& paths, MappingOperationCallback callback) {
@@ -414,9 +417,10 @@ bool AssetClient::deleteAssetMappings(const AssetPathList& paths, MappingOperati
         _pendingMappingRequests[assetServer][messageID] = callback;
 
         return true;
+    } else {
+        callback(false, AssetServerError::NoError, QSharedPointer<ReceivedMessage>());
+        return false;
     }
-
-    return false;
 }
 
 bool AssetClient::setAssetMapping(const QString& path, const AssetHash& hash, MappingOperationCallback callback) {
@@ -439,9 +443,10 @@ bool AssetClient::setAssetMapping(const QString& path, const AssetHash& hash, Ma
         _pendingMappingRequests[assetServer][messageID] = callback;
 
         return true;
+    } else {
+        callback(false, AssetServerError::NoError, QSharedPointer<ReceivedMessage>());
+        return false;
     }
-
-    return false;
 }
 
 bool AssetClient::renameAssetMapping(const AssetPath& oldPath, const AssetPath& newPath, MappingOperationCallback callback) {
@@ -465,9 +470,10 @@ bool AssetClient::renameAssetMapping(const AssetPath& oldPath, const AssetPath&
 
         return true;
 
+    } else {
+        callback(false, AssetServerError::NoError, QSharedPointer<ReceivedMessage>());
+        return false;
     }
-
-    return false;
 }
 
 bool AssetClient::uploadAsset(const QByteArray& data, UploadResultCallback callback) {

libraries/networking/src/MappingRequest.cpp

@@ -30,6 +30,13 @@ GetMappingRequest::GetMappingRequest(const AssetPath& path) : _path(path) {
 
 void GetMappingRequest::doStart() {
 
+    // short circuit the request if the path is invalid
+    if (!isValidPath(_path)) {
+        _error = MappingRequest::InvalidPath;
+        emit finished(this);
+        return;
+    }
+
     auto assetClient = DependencyManager::get<AssetClient>();
 
     assetClient->getAssetMapping(_path, [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer<ReceivedMessage> message) {
@@ -89,11 +96,26 @@ void GetAllMappingsRequest::doStart() {
     });
 };
 
-SetMappingRequest::SetMappingRequest(const AssetPath& path, const AssetHash& hash) : _path(path), _hash(hash) {
+SetMappingRequest::SetMappingRequest(const AssetPath& path, const AssetHash& hash) :
+    _path(path),
+    _hash(hash)
+{
+
 };
 
 void SetMappingRequest::doStart() {
+
+    // short circuit the request if the hash or path are invalid
+    auto validPath = isValidPath(_path);
+    auto validHash = isValidHash(_hash);
+    if (!validPath || !validHash) {
+        _error = validPath ? MappingRequest::InvalidPath : MappingRequest::InvalidHash;
+        emit finished(this);
+        return;
+    }
+
     auto assetClient = DependencyManager::get<AssetClient>();
+
     assetClient->setAssetMapping(_path, _hash, [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer<ReceivedMessage> message) {
         if (!responseReceived) {
             _error = NetworkError;
@@ -119,7 +141,18 @@ DeleteMappingsRequest::DeleteMappingsRequest(const AssetPathList& paths) : _path
 };
 
 void DeleteMappingsRequest::doStart() {
+
+    // short circuit the request if any of the paths are invalid
+    for (auto& path : _paths) {
+        if (!isValidPath(path)) {
+            _error = MappingRequest::InvalidPath;
+            emit finished(this);
+            return;
+        }
+    }
+
     auto assetClient = DependencyManager::get<AssetClient>();
+
     assetClient->deleteAssetMappings(_paths, [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer<ReceivedMessage> message) {
         if (!responseReceived) {
             _error = NetworkError;
@@ -142,14 +175,23 @@ void DeleteMappingsRequest::doStart() {
 };
 
 RenameMappingRequest::RenameMappingRequest(const AssetPath& oldPath, const AssetPath& newPath) :
-_oldPath(oldPath),
-_newPath(newPath)
+    _oldPath(oldPath),
+    _newPath(newPath)
 {
 
 }
 
 void RenameMappingRequest::doStart() {
+
+    // short circuit the request if either of the paths are invalid
+    if (!isValidPath(_oldPath) || !isValidPath(_newPath)) {
+        _error = InvalidPath;
+        emit finished(this);
+        return;
+    }
+
     auto assetClient = DependencyManager::get<AssetClient>();
+
     assetClient->renameAssetMapping(_oldPath, _newPath, [this, assetClient](bool responseReceived,
                                                                             AssetServerError error,
                                                                             QSharedPointer<ReceivedMessage> message) {

libraries/networking/src/MappingRequest.h

@@ -26,6 +26,8 @@ public:
         NotFound,
         NetworkError,
         PermissionDenied,
+        InvalidPath,
+        InvalidHash,
         UnknownError
     };
 

libraries/script-engine/src/AssetMappingsScriptingInterface.cpp

@@ -29,87 +29,117 @@ void AssetMappingsScriptingInterface::setMapping(QString path, QString hash, QJS
     auto assetClient = DependencyManager::get<AssetClient>();
     auto request = assetClient->createSetMappingRequest(path, hash);
 
-    connect(request, &SetMappingRequest::finished, this, [this, callback](SetMappingRequest* request) mutable {
-        QJSValueList args { uint8_t(request->getError()) };
+    if (request) {
+        connect(request, &SetMappingRequest::finished, this, [this, callback](SetMappingRequest* request) mutable {
+            QJSValueList args { uint8_t(request->getError()) };
 
+            callback.call(args);
+
+            request->deleteLater();
+
+        });
+        
+        request->start();
+    } else {
+        // not connected to an Asset Server, return network error
+        QJSValueList args { uint8_t(MappingRequest::NetworkError) };
         callback.call(args);
-
-        request->deleteLater();
-         
-    });
-
-    request->start();
+    }
 }
 
 void AssetMappingsScriptingInterface::getMapping(QString path, QJSValue callback) {
     auto assetClient = DependencyManager::get<AssetClient>();
     auto request = assetClient->createGetMappingRequest(path);
 
-    connect(request, &GetMappingRequest::finished, this, [this, callback](GetMappingRequest* request) mutable {
-        QJSValueList args { uint8_t(request->getError()), request->getHash() };
+    if (request) {
+        connect(request, &GetMappingRequest::finished, this, [this, callback](GetMappingRequest* request) mutable {
+            QJSValueList args { uint8_t(request->getError()), request->getHash() };
 
+            callback.call(args);
+
+            request->deleteLater();
+
+        });
+        
+        request->start();
+    } else {
+        // not connected to an Asset Server, return network error
+        QJSValueList args { uint8_t(MappingRequest::NetworkError) };
         callback.call(args);
-
-        request->deleteLater();
-         
-    });
-
-    request->start();
+    }
 }
 
 void AssetMappingsScriptingInterface::deleteMappings(QStringList paths, QJSValue callback) {
     auto assetClient = DependencyManager::get<AssetClient>();
     auto request = assetClient->createDeleteMappingsRequest(paths);
 
-    connect(request, &DeleteMappingsRequest::finished, this, [this, callback](DeleteMappingsRequest* request) mutable {
-        QJSValueList args { uint8_t(request->getError()) };
+    if (request) {
+        connect(request, &DeleteMappingsRequest::finished, this, [this, callback](DeleteMappingsRequest* request) mutable {
+            QJSValueList args { uint8_t(request->getError()) };
 
+            callback.call(args);
+
+            request->deleteLater();
+
+        });
+        
+        request->start();
+    } else {
+        // not connected to an Asset Server, return network error
+        QJSValueList args { uint8_t(MappingRequest::NetworkError) };
         callback.call(args);
-
-        request->deleteLater();
-         
-    });
-
-    request->start();
+    }
 }
 
 void AssetMappingsScriptingInterface::getAllMappings(QJSValue callback) {
     auto assetClient = DependencyManager::get<AssetClient>();
     auto request = assetClient->createGetAllMappingsRequest();
 
-    connect(request, &GetAllMappingsRequest::finished, this, [this, callback](GetAllMappingsRequest* request) mutable {
-        auto mappings = request->getMappings();
-        auto map = callback.engine()->newObject();
+    if (request) {
+        connect(request, &GetAllMappingsRequest::finished, this, [this, callback](GetAllMappingsRequest* request) mutable {
+            auto mappings = request->getMappings();
+            auto map = callback.engine()->newObject();
 
-        for (auto& kv : mappings ) {
-            map.setProperty(kv.first, kv.second);
-        }
+            for (auto& kv : mappings ) {
+                map.setProperty(kv.first, kv.second);
+            }
 
-        QJSValueList args { uint8_t(request->getError()), map };
+            QJSValueList args { uint8_t(request->getError()), map };
 
+            callback.call(args);
+
+            request->deleteLater();
+            
+        });
+        
+        request->start();
+    } else {
+        // not connected to an Asset Server, return network error
+        QJSValueList args { uint8_t(MappingRequest::NetworkError) };
         callback.call(args);
-
-        request->deleteLater();
-         
-    });
-
-    request->start();
+    }
 }
 
 void AssetMappingsScriptingInterface::renameMapping(QString oldPath, QString newPath, QJSValue callback) {
     auto assetClient = DependencyManager::get<AssetClient>();
     auto request = assetClient->createRenameMappingRequest(oldPath, newPath);
 
-    connect(request, &RenameMappingRequest::finished, this, [this, callback](RenameMappingRequest* request) mutable {
-        QJSValueList args { uint8_t(request->getError()) };
+    if (request) {
+        connect(request, &RenameMappingRequest::finished, this, [this, callback](RenameMappingRequest* request) mutable {
+            QJSValueList args { uint8_t(request->getError()) };
+
+            callback.call(args);
+
+            request->deleteLater();
+
+        });
         
+        request->start();
+    } else {
+        // not connected to an Asset Server, return network error
+        QJSValueList args { uint8_t(MappingRequest::NetworkError) };
         callback.call(args);
-        
-        request->deleteLater();
-        
-    });
-    
-    request->start();
+    }
 }
 
 
@@ -134,92 +164,102 @@ void AssetMappingModel::refresh() {
     auto assetClient = DependencyManager::get<AssetClient>();
     auto request = assetClient->createGetAllMappingsRequest();
 
-    connect(request, &GetAllMappingsRequest::finished, this, [this](GetAllMappingsRequest* request) mutable {
-        auto mappings = request->getMappings();
-        auto existingPaths = _pathToItemMap.keys();
-        for (auto& mapping : mappings) {
-            auto& path = mapping.first;
-            auto parts = path.split("/");
-            auto length = parts.length();
+    if (request) {
+        connect(request, &GetAllMappingsRequest::finished, this, [this](GetAllMappingsRequest* request) mutable {
+            if (request->getError() == MappingRequest::NoError) {
+                auto mappings = request->getMappings();
+                auto existingPaths = _pathToItemMap.keys();
+                for (auto& mapping : mappings) {
+                    auto& path = mapping.first;
+                    auto parts = path.split("/");
+                    auto length = parts.length();
 
-            existingPaths.removeOne(mapping.first);
+                    existingPaths.removeOne(mapping.first);
 
-            QString fullPath = "/";
+                    QString fullPath = "/";
 
-            QStandardItem* lastItem = nullptr;
+                    QStandardItem* lastItem = nullptr;
 
-            // start index at 1 to avoid empty string from leading slash
-            for (int i = 1; i < length; ++i) {
-                fullPath += (i == 1 ? "" : "/") + parts[i];
+                    // start index at 1 to avoid empty string from leading slash
+                    for (int i = 1; i < length; ++i) {
+                        fullPath += (i == 1 ? "" : "/") + parts[i];
 
-                auto it = _pathToItemMap.find(fullPath);
-                if (it == _pathToItemMap.end()) {
-                    qDebug() << "prefix not found: " << fullPath;
-                    auto item = new QStandardItem(parts[i]);
-                    bool isFolder = i < length - 1;
-                    item->setData(isFolder ? fullPath + "/" : fullPath, Qt::UserRole);
-                    item->setData(isFolder, Qt::UserRole + 1);
-                    item->setData(parts[i], Qt::UserRole + 2);
-                    item->setData("atp:" + fullPath, Qt::UserRole + 3);
-                    if (lastItem) {
-                        lastItem->setChild(lastItem->rowCount(), 0, item);
-                    } else {
-                        appendRow(item);
+                        auto it = _pathToItemMap.find(fullPath);
+                        if (it == _pathToItemMap.end()) {
+                            qDebug() << "prefix not found: " << fullPath;
+                            auto item = new QStandardItem(parts[i]);
+                            bool isFolder = i < length - 1;
+                            item->setData(isFolder ? fullPath + "/" : fullPath, Qt::UserRole);
+                            item->setData(isFolder, Qt::UserRole + 1);
+                            item->setData(parts[i], Qt::UserRole + 2);
+                            item->setData("atp:" + fullPath, Qt::UserRole + 3);
+                            if (lastItem) {
+                                lastItem->setChild(lastItem->rowCount(), 0, item);
+                            } else {
+                                appendRow(item);
+                            }
+
+                            lastItem = item;
+                            _pathToItemMap[fullPath] = lastItem;
+                        }
+                        else {
+                            lastItem = it.value();
+                        }
                     }
 
-                    lastItem = item;
-                    _pathToItemMap[fullPath] = lastItem;
+                    Q_ASSERT(fullPath == path);
                 }
-                else {
-                    lastItem = it.value();
+
+                // Remove folders from list
+                auto it = existingPaths.begin();
+                while (it != existingPaths.end()) {
+                    auto item = _pathToItemMap[*it];
+                    if (item->data(Qt::UserRole + 1).toBool()) {
+                        it = existingPaths.erase(it);
+                    } else {
+                        ++it;
+                    }
                 }
-            }
 
-            Q_ASSERT(fullPath == path);
-        }
+                for (auto& path : existingPaths) {
+                    Q_ASSERT(_pathToItemMap.contains(path));
+                    qDebug() << "removing existing: " << path;
 
-        // Remove folders from list
-        auto it = existingPaths.begin();
-        while (it != existingPaths.end()) {
-            auto item = _pathToItemMap[*it];
-            if (item->data(Qt::UserRole + 1).toBool()) {
-                it = existingPaths.erase(it);
+                    auto item = _pathToItemMap[path];
+
+                    while (item) {
+                        // During each iteration, delete item
+                        QStandardItem* nextItem = nullptr;
+
+                        auto parent = item->parent();
+                        if (parent) {
+                            parent->removeRow(item->row());
+                            if (parent->rowCount() > 0) {
+                                // The parent still contains children, set the nextItem to null so we stop processing
+                                nextItem = nullptr;
+                            } else {
+                                nextItem = parent;
+                            }
+                        } else {
+                            removeRow(item->row());
+                        }
+                        
+                        _pathToItemMap.remove(path);
+                        //delete item;
+                        
+                        item = nextItem;
+                    }
+                    //removeitem->index();
+                }
             } else {
-                ++it;
+                emit errorGettingMappings(uint8_t(request->getError()));
             }
-        }
+        });
 
-        for (auto& path : existingPaths) {
-            Q_ASSERT(_pathToItemMap.contains(path));
-            qDebug() << "removing existing: " << path;
-
-            auto item = _pathToItemMap[path];
-
-            while (item) {
-                // During each iteration, delete item
-                QStandardItem* nextItem = nullptr;
-
-                auto parent = item->parent();
-                if (parent) {
-                    parent->removeRow(item->row());
-                    if (parent->rowCount() > 0) {
-                        // The parent still contains children, set the nextItem to null so we stop processing
-                        nextItem = nullptr;
-                    } else {
-                        nextItem = parent;
-                    }
-                } else {
-                    removeRow(item->row());
-                }
-
-                _pathToItemMap.remove(path);
-                //delete item;
-
-                item = nextItem;
-            }
-            //removeitem->index();
-        }
-    });
-
-    request->start();
+        request->start();
+    } else {
+        qDebug() << "NO CONNECTED ASSET SERVER";
+        // not connected to an Asset Server, emit network error
+        emit errorGettingMappings(uint8_t(MappingRequest::NetworkError));
+    }
 }

libraries/script-engine/src/AssetMappingsScriptingInterface.h

@@ -42,6 +42,9 @@
 
      bool isKnownMapping(QString path) const { return _pathToItemMap.contains(path); };
 
+ signals:
+     void errorGettingMappings(uint8_t error);
+
  private:
      QHash<QString, QStandardItem*> _pathToItemMap;
  };
@@ -65,6 +68,7 @@ public:
     Q_INVOKABLE void deleteMapping(QString path, QJSValue callback) { deleteMappings(QStringList(path), callback); }
     Q_INVOKABLE void getAllMappings(QJSValue callback);
     Q_INVOKABLE void renameMapping(QString oldPath, QString newPath, QJSValue callback);
+
 protected:
     QSet<AssetRequest*> _pendingRequests;
     AssetMappingModel _assetMappingModel;

libraries/script-engine/src/AssetScriptingInterface.cpp

@@ -34,7 +34,7 @@ void AssetScriptingInterface::uploadData(QString data, QScriptValue callback) {
 
     QObject::connect(upload, &AssetUpload::finished, this, [this, callback](AssetUpload* upload, const QString& hash) mutable {
         if (callback.isFunction()) {
-            QString url = "atp://" + hash;
+            QString url = "atp:" + hash;
             QScriptValueList args { url };
             callback.call(_engine->currentContext()->thisObject(), args);
         }
@@ -43,7 +43,7 @@ void AssetScriptingInterface::uploadData(QString data, QScriptValue callback) {
 }
 
 void AssetScriptingInterface::downloadData(QString urlString, QScriptValue callback) {
-    const QString ATP_SCHEME { "atp://" };
+    const QString ATP_SCHEME { "atp:" };
 
     if (!urlString.startsWith(ATP_SCHEME)) {
         return;