Merge lp:~michihenning/storage-framework/bug1644577 into lp:storage-framework/devel
- bug1644577
- Merge into devel
Proposed by
Michi Henning
| Status: | Merged |
|---|---|
| Approved by: | Michi Henning |
| Approved revision: | 97 |
| Merged at revision: | 96 |
| Proposed branch: | lp:~michihenning/storage-framework/bug1644577 |
| Merge into: | lp:storage-framework/devel |
| Diff against target: |
537 lines (+161/-118) 6 files modified
src/qt/internal/AccountImpl.cpp (+2/-1) src/qt/internal/ItemImpl.cpp (+10/-8) src/qt/internal/ItemListJobImpl.cpp (+4/-6) src/qt/internal/MultiItemListJobImpl.cpp (+1/-4) src/qt/internal/validate.cpp (+75/-63) tests/remote-client/remote-client_test.cpp (+69/-36) |
| To merge this branch: | bzr merge lp:~michihenning/storage-framework/bug1644577 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| unity-api-1-bot | continuous-integration | Approve | |
| James Henstridge | Approve | ||
|
Review via email:
|
|||
Commit message
Fix for bug 1644577, fail list job if metadata for any item is incorrect.
Always emit itemsReady(), even if list is empty.
validate() failures now log the error.
Lots of improvements to log messages to provide more detail.
Description of the change
Fix for bug 1644577, fail list job if metadata for any item is incorrect.
Always emit itemsReady(), even if list is empty.
validate() failures now log the error.
Lots of improvements to log messages to provide more detail.
To post a comment you must log in.
Revision history for this message
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:96
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
review:
Needs Fixing
(continuous-integration)
- 97. By Michi Henning
-
Review comments from James.
Revision history for this message
| Michi Henning (michihenning) wrote : | # |
Good catch, thanks!
Revision history for this message
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
FAILED: Continuous integration, rev:97
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
review:
Needs Fixing
(continuous-integration)
Revision history for this message
| unity-api-1-bot (unity-api-1-bot) wrote : | # |
PASSED: Continuous integration, rev:97
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
review:
Approve
(continuous-integration)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'src/qt/internal/AccountImpl.cpp' | |||
| 2 | --- src/qt/internal/AccountImpl.cpp 2016-11-03 06:41:00 +0000 | |||
| 3 | +++ src/qt/internal/AccountImpl.cpp 2016-11-25 04:22:53 +0000 | |||
| 4 | @@ -101,7 +101,8 @@ | |||
| 5 | 101 | { | 101 | { |
| 6 | 102 | if (md.type != ItemType::root) | 102 | if (md.type != ItemType::root) |
| 7 | 103 | { | 103 | { |
| 9 | 104 | QString msg = method + ": provider returned non-root item type: " + QString::number(int(md.type)); | 104 | QString msg = method + ": provider returned non-root item type: " + QString::number(int(md.type)) + |
| 10 | 105 | " (id = " + md.item_id + ")"; | ||
| 11 | 105 | qCritical().noquote() << msg; | 106 | qCritical().noquote() << msg; |
| 12 | 106 | throw StorageErrorImpl::local_comms_error(msg); | 107 | throw StorageErrorImpl::local_comms_error(msg); |
| 13 | 107 | } | 108 | } |
| 14 | 108 | 109 | ||
| 15 | === modified file 'src/qt/internal/ItemImpl.cpp' | |||
| 16 | --- src/qt/internal/ItemImpl.cpp 2016-11-03 03:41:24 +0000 | |||
| 17 | +++ src/qt/internal/ItemImpl.cpp 2016-11-25 04:22:53 +0000 | |||
| 18 | @@ -153,7 +153,7 @@ | |||
| 19 | 153 | { | 153 | { |
| 20 | 154 | if (md.type == ItemType::file) | 154 | if (md.type == ItemType::file) |
| 21 | 155 | { | 155 | { |
| 23 | 156 | QString msg = method + ": provider returned a file as a parent"; | 156 | QString msg = method + ": provider returned a file as a parent (id = " + md.item_id + ")"; |
| 24 | 157 | qCritical().noquote() << msg; | 157 | qCritical().noquote() << msg; |
| 25 | 158 | throw StorageErrorImpl::local_comms_error(msg); | 158 | throw StorageErrorImpl::local_comms_error(msg); |
| 26 | 159 | } | 159 | } |
| 27 | @@ -178,7 +178,8 @@ | |||
| 28 | 178 | || | 178 | || |
| 29 | 179 | (md_.type != ItemType::file && md.type == ItemType::file)) | 179 | (md_.type != ItemType::file && md.type == ItemType::file)) |
| 30 | 180 | { | 180 | { |
| 32 | 181 | QString msg = method + ": source and target item type differ"; | 181 | QString msg = method + "provider error: source and target item type differ (source id = " + |
| 33 | 182 | md_.item_id + ", target id = " + md.item_id + ")"; | ||
| 34 | 182 | qCritical().noquote() << msg; | 183 | qCritical().noquote() << msg; |
| 35 | 183 | throw StorageErrorImpl::local_comms_error(msg); | 184 | throw StorageErrorImpl::local_comms_error(msg); |
| 36 | 184 | } | 185 | } |
| 37 | @@ -203,7 +204,7 @@ | |||
| 38 | 203 | { | 204 | { |
| 39 | 204 | if (md.type == ItemType::root) | 205 | if (md.type == ItemType::root) |
| 40 | 205 | { | 206 | { |
| 42 | 206 | QString msg = method + ": impossible root item returned by provider"; | 207 | QString msg = method + ": impossible root item returned by provider (id = " + md.item_id + ")"; |
| 43 | 207 | qCritical().noquote() << msg; | 208 | qCritical().noquote() << msg; |
| 44 | 208 | throw StorageErrorImpl::local_comms_error(msg); | 209 | throw StorageErrorImpl::local_comms_error(msg); |
| 45 | 209 | } | 210 | } |
| 46 | @@ -211,7 +212,8 @@ | |||
| 47 | 211 | || | 212 | || |
| 48 | 212 | (md_.type != ItemType::file && md.type == ItemType::file)) | 213 | (md_.type != ItemType::file && md.type == ItemType::file)) |
| 49 | 213 | { | 214 | { |
| 51 | 214 | QString msg = method + ": provider error: source and target item type differ"; | 215 | QString msg = method + ": provider error: source and target item type differ (source id = " + |
| 52 | 216 | md_.item_id + ", target id = " + md.item_id + ")"; | ||
| 53 | 215 | qCritical().noquote() << msg; | 217 | qCritical().noquote() << msg; |
| 54 | 216 | throw StorageErrorImpl::local_comms_error(msg); | 218 | throw StorageErrorImpl::local_comms_error(msg); |
| 55 | 217 | } | 219 | } |
| 56 | @@ -266,7 +268,7 @@ | |||
| 57 | 266 | { | 268 | { |
| 58 | 267 | if (md.type != storage::ItemType::file) | 269 | if (md.type != storage::ItemType::file) |
| 59 | 268 | { | 270 | { |
| 61 | 269 | QString msg = method + ": impossible folder item returned by provider"; | 271 | QString msg = method + ": impossible folder item returned by provider (id = " + md.item_id + ")"; |
| 62 | 270 | qCritical().noquote() << msg; | 272 | qCritical().noquote() << msg; |
| 63 | 271 | throw StorageErrorImpl::local_comms_error(msg); | 273 | throw StorageErrorImpl::local_comms_error(msg); |
| 64 | 272 | } | 274 | } |
| 65 | @@ -318,7 +320,7 @@ | |||
| 66 | 318 | { | 320 | { |
| 67 | 319 | if (md.type == storage::ItemType::root) | 321 | if (md.type == storage::ItemType::root) |
| 68 | 320 | { | 322 | { |
| 70 | 321 | QString msg = method + ": impossible root item returned by provider"; | 323 | QString msg = method + ": impossible root item returned by provider (id = " + md.item_id + ")"; |
| 71 | 322 | qCritical().noquote() << msg; | 324 | qCritical().noquote() << msg; |
| 72 | 323 | throw StorageErrorImpl::local_comms_error(msg); | 325 | throw StorageErrorImpl::local_comms_error(msg); |
| 73 | 324 | } | 326 | } |
| 74 | @@ -379,7 +381,7 @@ | |||
| 75 | 379 | { | 381 | { |
| 76 | 380 | return; | 382 | return; |
| 77 | 381 | } | 383 | } |
| 79 | 382 | QString msg = method + ": impossible file item returned by provider"; | 384 | QString msg = method + ": impossible file item returned by provider (id = " + md.item_id + ")"; |
| 80 | 383 | qCritical().noquote() << msg; | 385 | qCritical().noquote() << msg; |
| 81 | 384 | throw StorageErrorImpl::local_comms_error(msg); | 386 | throw StorageErrorImpl::local_comms_error(msg); |
| 82 | 385 | }; | 387 | }; |
| 83 | @@ -423,7 +425,7 @@ | |||
| 84 | 423 | { | 425 | { |
| 85 | 424 | if (md.type != storage::ItemType::file) | 426 | if (md.type != storage::ItemType::file) |
| 86 | 425 | { | 427 | { |
| 88 | 426 | QString msg = method + ": impossible folder item returned by provider"; | 428 | QString msg = method + ": impossible folder item returned by provider (id = " + md.item_id + ")"; |
| 89 | 427 | qCritical().noquote() << msg; | 429 | qCritical().noquote() << msg; |
| 90 | 428 | throw StorageErrorImpl::local_comms_error(msg); | 430 | throw StorageErrorImpl::local_comms_error(msg); |
| 91 | 429 | } | 431 | } |
| 92 | 430 | 432 | ||
| 93 | === modified file 'src/qt/internal/ItemListJobImpl.cpp' | |||
| 94 | --- src/qt/internal/ItemListJobImpl.cpp 2016-10-12 07:23:15 +0000 | |||
| 95 | +++ src/qt/internal/ItemListJobImpl.cpp 2016-11-25 04:22:53 +0000 | |||
| 96 | @@ -62,16 +62,14 @@ | |||
| 97 | 62 | auto item = ItemImpl::make_item(method_, md, account_impl_); | 62 | auto item = ItemImpl::make_item(method_, md, account_impl_); |
| 98 | 63 | items.append(item); | 63 | items.append(item); |
| 99 | 64 | } | 64 | } |
| 101 | 65 | catch (StorageError const&) | 65 | catch (StorageError const& e) |
| 102 | 66 | { | 66 | { |
| 103 | 67 | // Bad metadata received from provider, validate_() or make_item() have logged it. | 67 | // Bad metadata received from provider, validate_() or make_item() have logged it. |
| 104 | 68 | error_ = e; | ||
| 105 | 68 | } | 69 | } |
| 106 | 69 | } | 70 | } |
| 112 | 70 | status_ = ItemListJob::Status::Finished; | 71 | status_ = error_.type() == StorageError::NoError ? ItemListJob::Finished : ItemListJob::Error; |
| 113 | 71 | if (!items.isEmpty()) | 72 | Q_EMIT public_instance_->itemsReady(items); |
| 109 | 72 | { | ||
| 110 | 73 | Q_EMIT public_instance_->itemsReady(items); | ||
| 111 | 74 | } | ||
| 114 | 75 | Q_EMIT public_instance_->statusChanged(status_); | 73 | Q_EMIT public_instance_->statusChanged(status_); |
| 115 | 76 | }; | 74 | }; |
| 116 | 77 | 75 | ||
| 117 | 78 | 76 | ||
| 118 | === modified file 'src/qt/internal/MultiItemListJobImpl.cpp' | |||
| 119 | --- src/qt/internal/MultiItemListJobImpl.cpp 2016-10-12 07:23:15 +0000 | |||
| 120 | +++ src/qt/internal/MultiItemListJobImpl.cpp 2016-11-25 04:22:53 +0000 | |||
| 121 | @@ -86,10 +86,7 @@ | |||
| 122 | 86 | { | 86 | { |
| 123 | 87 | status_ = ItemListJob::Status::Finished; | 87 | status_ = ItemListJob::Status::Finished; |
| 124 | 88 | } | 88 | } |
| 129 | 89 | if (!items.isEmpty()) | 89 | Q_EMIT public_instance_->itemsReady(items); |
| 126 | 90 | { | ||
| 127 | 91 | Q_EMIT public_instance_->itemsReady(items); | ||
| 128 | 92 | } | ||
| 130 | 93 | if (token.isEmpty()) | 90 | if (token.isEmpty()) |
| 131 | 94 | { | 91 | { |
| 132 | 95 | Q_EMIT public_instance_->statusChanged(status_); | 92 | Q_EMIT public_instance_->statusChanged(status_); |
| 133 | 96 | 93 | ||
| 134 | === modified file 'src/qt/internal/validate.cpp' | |||
| 135 | --- src/qt/internal/validate.cpp 2016-11-02 06:06:32 +0000 | |||
| 136 | +++ src/qt/internal/validate.cpp 2016-11-25 04:22:53 +0000 | |||
| 137 | @@ -110,69 +110,81 @@ | |||
| 138 | 110 | { | 110 | { |
| 139 | 111 | using namespace unity::storage::metadata; | 111 | using namespace unity::storage::metadata; |
| 140 | 112 | 112 | ||
| 204 | 113 | QString prefix = method + ": received invalid metadata from provider: "; | 113 | QString prefix = method + ": received invalid metadata from provider"; |
| 205 | 114 | 114 | if (!md.item_id.isEmpty()) | |
| 206 | 115 | // Basic sanity checks for mandatory fields. | 115 | { |
| 207 | 116 | if (md.item_id.isEmpty()) | 116 | prefix += " (id = " + md.item_id + ")"; |
| 208 | 117 | { | 117 | } |
| 209 | 118 | throw StorageErrorImpl::local_comms_error(prefix + "item_id cannot be empty"); | 118 | prefix += ": "; |
| 210 | 119 | } | 119 | |
| 211 | 120 | if (md.type != ItemType::root) | 120 | try |
| 212 | 121 | { | 121 | { |
| 213 | 122 | if (md.parent_ids.isEmpty()) | 122 | // Basic sanity checks for mandatory fields. |
| 214 | 123 | { | 123 | if (md.item_id.isEmpty()) |
| 215 | 124 | throw StorageErrorImpl::local_comms_error(prefix + "file or folder must have at least one parent ID"); | 124 | { |
| 216 | 125 | } | 125 | throw StorageErrorImpl::local_comms_error(prefix + "item_id cannot be empty"); |
| 217 | 126 | for (int i = 0; i < md.parent_ids.size(); ++i) | 126 | } |
| 218 | 127 | { | 127 | if (md.type != ItemType::root) |
| 219 | 128 | if (md.parent_ids.at(i).isEmpty()) | 128 | { |
| 220 | 129 | { | 129 | if (md.parent_ids.isEmpty()) |
| 221 | 130 | throw StorageErrorImpl::local_comms_error(prefix + "parent_id of file or folder cannot be empty"); | 130 | { |
| 222 | 131 | } | 131 | throw StorageErrorImpl::local_comms_error(prefix + "file or folder must have at least one parent ID"); |
| 223 | 132 | } | 132 | } |
| 224 | 133 | } | 133 | for (int i = 0; i < md.parent_ids.size(); ++i) |
| 225 | 134 | if (md.type == ItemType::root && !md.parent_ids.isEmpty()) | 134 | { |
| 226 | 135 | { | 135 | if (md.parent_ids.at(i).isEmpty()) |
| 227 | 136 | throw StorageErrorImpl::local_comms_error(prefix + "parent_ids of root must be empty"); | 136 | { |
| 228 | 137 | } | 137 | throw StorageErrorImpl::local_comms_error(prefix + "parent_id of file or folder cannot be empty"); |
| 229 | 138 | if (md.type != ItemType::root) // Dropbox does not support metadata for roots. | 138 | } |
| 230 | 139 | { | 139 | } |
| 231 | 140 | if (md.name.isEmpty()) | 140 | } |
| 232 | 141 | { | 141 | if (md.type == ItemType::root && !md.parent_ids.isEmpty()) |
| 233 | 142 | throw StorageErrorImpl::local_comms_error(prefix + "name cannot be empty"); | 142 | { |
| 234 | 143 | } | 143 | throw StorageErrorImpl::local_comms_error(prefix + "parent_ids of root must be empty"); |
| 235 | 144 | } | 144 | } |
| 236 | 145 | if (md.type == ItemType::file && md.etag.isEmpty()) // WebDav doesn't do etag for folders. | 145 | if (md.type != ItemType::root) // Dropbox does not support metadata for roots. |
| 237 | 146 | { | 146 | { |
| 238 | 147 | throw StorageErrorImpl::local_comms_error(prefix + "etag of a file cannot be empty"); | 147 | if (md.name.isEmpty()) |
| 239 | 148 | } | 148 | { |
| 240 | 149 | 149 | throw StorageErrorImpl::local_comms_error(prefix + "name cannot be empty"); | |
| 241 | 150 | // Sanity check metadata to make sure only known metadata keys appear. | 150 | } |
| 242 | 151 | QMapIterator<QString, QVariant> actual(md.metadata); | 151 | } |
| 243 | 152 | while (actual.hasNext()) | 152 | if (md.type == ItemType::file && md.etag.isEmpty()) // WebDav doesn't do etag for folders. |
| 244 | 153 | { | 153 | { |
| 245 | 154 | actual.next(); | 154 | throw StorageErrorImpl::local_comms_error(prefix + "etag of a file cannot be empty"); |
| 246 | 155 | auto known = known_metadata.find(actual.key().toStdString()); | 155 | } |
| 247 | 156 | if (known == known_metadata.end()) | 156 | |
| 248 | 157 | { | 157 | // Sanity check metadata to make sure only known metadata keys appear. |
| 249 | 158 | qWarning().noquote().nospace() << prefix << "unknown metadata key: \"" << actual.key() << "\""; | 158 | QMapIterator<QString, QVariant> actual(md.metadata); |
| 250 | 159 | } | 159 | while (actual.hasNext()) |
| 251 | 160 | else | 160 | { |
| 252 | 161 | { | 161 | actual.next(); |
| 253 | 162 | validate_type_and_value(prefix, actual, known); | 162 | auto known = known_metadata.find(actual.key().toStdString()); |
| 254 | 163 | } | 163 | if (known == known_metadata.end()) |
| 255 | 164 | } | 164 | { |
| 256 | 165 | 165 | qWarning().noquote().nospace() << prefix << "unknown metadata key: \"" << actual.key() << "\""; | |
| 257 | 166 | // Sanity check metadata to make sure that mandatory fields are present. | 166 | } |
| 258 | 167 | if (md.type == ItemType::file) | 167 | else |
| 259 | 168 | { | 168 | { |
| 260 | 169 | if (!md.metadata.contains(metadata::SIZE_IN_BYTES) || | 169 | validate_type_and_value(prefix, actual, known); |
| 261 | 170 | !md.metadata.contains(metadata::LAST_MODIFIED_TIME)) | 170 | } |
| 262 | 171 | { | 171 | } |
| 263 | 172 | QString msg = prefix + "missing key \"" + metadata::SIZE_IN_BYTES + "\" " | 172 | |
| 264 | 173 | "in metadata for \"" + md.item_id + "\""; | 173 | // Sanity check metadata to make sure that mandatory fields are present. |
| 265 | 174 | throw StorageErrorImpl::local_comms_error(msg); | 174 | if (md.type == ItemType::file) |
| 266 | 175 | } | 175 | { |
| 267 | 176 | if (!md.metadata.contains(metadata::SIZE_IN_BYTES) || | ||
| 268 | 177 | !md.metadata.contains(metadata::LAST_MODIFIED_TIME)) | ||
| 269 | 178 | { | ||
| 270 | 179 | QString msg = prefix + "missing key \"" + metadata::SIZE_IN_BYTES + "\" in metadata"; | ||
| 271 | 180 | throw StorageErrorImpl::local_comms_error(msg); | ||
| 272 | 181 | } | ||
| 273 | 182 | } | ||
| 274 | 183 | } | ||
| 275 | 184 | catch (StorageError const& e) | ||
| 276 | 185 | { | ||
| 277 | 186 | qCritical().noquote() << e.errorString(); | ||
| 278 | 187 | throw; | ||
| 279 | 176 | } | 188 | } |
| 280 | 177 | } | 189 | } |
| 281 | 178 | 190 | ||
| 282 | 179 | 191 | ||
| 283 | === modified file 'tests/remote-client/remote-client_test.cpp' | |||
| 284 | --- tests/remote-client/remote-client_test.cpp 2016-11-04 07:33:07 +0000 | |||
| 285 | +++ tests/remote-client/remote-client_test.cpp 2016-11-25 04:22:53 +0000 | |||
| 286 | @@ -504,11 +504,23 @@ | |||
| 287 | 504 | QSignalSpy ready_spy(j.get(), &ItemListJob::itemsReady); | 504 | QSignalSpy ready_spy(j.get(), &ItemListJob::itemsReady); |
| 288 | 505 | QSignalSpy status_spy(j.get(), &ItemListJob::statusChanged); | 505 | QSignalSpy status_spy(j.get(), &ItemListJob::statusChanged); |
| 289 | 506 | status_spy.wait(SIGNAL_WAIT_TIME); | 506 | status_spy.wait(SIGNAL_WAIT_TIME); |
| 295 | 507 | auto arg = status_spy.takeFirst(); | 507 | { |
| 296 | 508 | 508 | auto arg = status_spy.takeFirst(); | |
| 297 | 509 | // Bad metadata is ignored, so status is finished, and itemsReady was never called. | 509 | EXPECT_EQ(ItemListJob::Status::Error, qvariant_cast<ItemListJob::Status>(arg.at(0))); |
| 298 | 510 | EXPECT_EQ(ItemListJob::Status::Finished, qvariant_cast<ItemListJob::Status>(arg.at(0))); | 510 | } |
| 299 | 511 | EXPECT_EQ(0, status_spy.count()); | 511 | |
| 300 | 512 | if (ready_spy.count() != 1) | ||
| 301 | 513 | { | ||
| 302 | 514 | ready_spy.wait(SIGNAL_WAIT_TIME); | ||
| 303 | 515 | } | ||
| 304 | 516 | auto arg = ready_spy.takeFirst(); | ||
| 305 | 517 | auto items = qvariant_cast<QList<Item>>(arg.at(0)); | ||
| 306 | 518 | EXPECT_EQ(0, items.size()); | ||
| 307 | 519 | |||
| 308 | 520 | EXPECT_EQ(ItemListJob::Status::Error, j->status()); | ||
| 309 | 521 | EXPECT_EQ(StorageError::Type::LocalCommsError, j->error().type()); | ||
| 310 | 522 | EXPECT_EQ("LocalCommsError: Account::roots(): provider returned non-root item type: 0 (id = root_id)", | ||
| 311 | 523 | j->error().errorString()); | ||
| 312 | 512 | } | 524 | } |
| 313 | 513 | 525 | ||
| 314 | 514 | TEST_F(GetTest, basic) | 526 | TEST_F(GetTest, basic) |
| 315 | @@ -679,8 +691,9 @@ | |||
| 316 | 679 | spy.wait(SIGNAL_WAIT_TIME); | 691 | spy.wait(SIGNAL_WAIT_TIME); |
| 317 | 680 | 692 | ||
| 318 | 681 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 693 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 321 | 682 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: " | 694 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 322 | 683 | "file or folder must have at least one parent ID", j->error().errorString()); | 695 | "file or folder must have at least one parent ID" |
| 323 | 696 | , j->error().errorString()); | ||
| 324 | 684 | } | 697 | } |
| 325 | 685 | 698 | ||
| 326 | 686 | TEST_F(MetadataTest, empty_parent) | 699 | TEST_F(MetadataTest, empty_parent) |
| 327 | @@ -693,8 +706,9 @@ | |||
| 328 | 693 | spy.wait(SIGNAL_WAIT_TIME); | 706 | spy.wait(SIGNAL_WAIT_TIME); |
| 329 | 694 | 707 | ||
| 330 | 695 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 708 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 333 | 696 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: " | 709 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 334 | 697 | "parent_id of file or folder cannot be empty", j->error().errorString()); | 710 | "parent_id of file or folder cannot be empty", |
| 335 | 711 | j->error().errorString()); | ||
| 336 | 698 | } | 712 | } |
| 337 | 699 | 713 | ||
| 338 | 700 | TEST_F(MetadataTest, root_with_parent) | 714 | TEST_F(MetadataTest, root_with_parent) |
| 339 | @@ -707,8 +721,9 @@ | |||
| 340 | 707 | spy.wait(SIGNAL_WAIT_TIME); | 721 | spy.wait(SIGNAL_WAIT_TIME); |
| 341 | 708 | 722 | ||
| 342 | 709 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 723 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 345 | 710 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: " | 724 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = root_id): " |
| 346 | 711 | "parent_ids of root must be empty", j->error().errorString()); | 725 | "parent_ids of root must be empty", |
| 347 | 726 | j->error().errorString()); | ||
| 348 | 712 | } | 727 | } |
| 349 | 713 | 728 | ||
| 350 | 714 | TEST_F(MetadataTest, empty_name) | 729 | TEST_F(MetadataTest, empty_name) |
| 351 | @@ -721,8 +736,9 @@ | |||
| 352 | 721 | spy.wait(SIGNAL_WAIT_TIME); | 736 | spy.wait(SIGNAL_WAIT_TIME); |
| 353 | 722 | 737 | ||
| 354 | 723 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 738 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 357 | 724 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: " | 739 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 358 | 725 | "name cannot be empty", j->error().errorString()); | 740 | "name cannot be empty", |
| 359 | 741 | j->error().errorString()); | ||
| 360 | 726 | } | 742 | } |
| 361 | 727 | 743 | ||
| 362 | 728 | TEST_F(MetadataTest, empty_etag) | 744 | TEST_F(MetadataTest, empty_etag) |
| 363 | @@ -735,8 +751,9 @@ | |||
| 364 | 735 | spy.wait(SIGNAL_WAIT_TIME); | 751 | spy.wait(SIGNAL_WAIT_TIME); |
| 365 | 736 | 752 | ||
| 366 | 737 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 753 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 369 | 738 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: " | 754 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 370 | 739 | "etag of a file cannot be empty", j->error().errorString()); | 755 | "etag of a file cannot be empty", |
| 371 | 756 | j->error().errorString()); | ||
| 372 | 740 | } | 757 | } |
| 373 | 741 | 758 | ||
| 374 | 742 | TEST_F(MetadataTest, unknown_key) | 759 | TEST_F(MetadataTest, unknown_key) |
| 375 | @@ -762,8 +779,9 @@ | |||
| 376 | 762 | spy.wait(SIGNAL_WAIT_TIME); | 779 | spy.wait(SIGNAL_WAIT_TIME); |
| 377 | 763 | 780 | ||
| 378 | 764 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 781 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 381 | 765 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: " | 782 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 382 | 766 | "missing key \"size_in_bytes\" in metadata for \"child_id\"", j->error().errorString()); | 783 | "missing key \"size_in_bytes\" in metadata", |
| 383 | 784 | j->error().errorString()); | ||
| 384 | 767 | } | 785 | } |
| 385 | 768 | 786 | ||
| 386 | 769 | TEST_F(MetadataTest, wrong_type_for_time) | 787 | TEST_F(MetadataTest, wrong_type_for_time) |
| 387 | @@ -776,8 +794,9 @@ | |||
| 388 | 776 | spy.wait(SIGNAL_WAIT_TIME); | 794 | spy.wait(SIGNAL_WAIT_TIME); |
| 389 | 777 | 795 | ||
| 390 | 778 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 796 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 393 | 779 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: last_modified_time: " | 797 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 394 | 780 | "expected value of type QString, but received value of type qlonglong", j->error().errorString()); | 798 | "last_modified_time: expected value of type QString, but received value of type qlonglong", |
| 395 | 799 | j->error().errorString()); | ||
| 396 | 781 | } | 800 | } |
| 397 | 782 | 801 | ||
| 398 | 783 | TEST_F(MetadataTest, bad_parse_for_time) | 802 | TEST_F(MetadataTest, bad_parse_for_time) |
| 399 | @@ -790,8 +809,9 @@ | |||
| 400 | 790 | spy.wait(SIGNAL_WAIT_TIME); | 809 | spy.wait(SIGNAL_WAIT_TIME); |
| 401 | 791 | 810 | ||
| 402 | 792 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 811 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 405 | 793 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: last_modified_time: " | 812 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 406 | 794 | "value \"xyz\" does not parse as ISO-8601 date", j->error().errorString()); | 813 | "last_modified_time: value \"xyz\" does not parse as ISO-8601 date", |
| 407 | 814 | j->error().errorString()); | ||
| 408 | 795 | } | 815 | } |
| 409 | 796 | 816 | ||
| 410 | 797 | TEST_F(MetadataTest, missing_timezone) | 817 | TEST_F(MetadataTest, missing_timezone) |
| 411 | @@ -804,8 +824,9 @@ | |||
| 412 | 804 | spy.wait(SIGNAL_WAIT_TIME); | 824 | spy.wait(SIGNAL_WAIT_TIME); |
| 413 | 805 | 825 | ||
| 414 | 806 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 826 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 417 | 807 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: last_modified_time: " | 827 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 418 | 808 | "value \"2007-04-05T14:30\" lacks a time zone specification", j->error().errorString()); | 828 | "last_modified_time: value \"2007-04-05T14:30\" lacks a time zone specification", |
| 419 | 829 | j->error().errorString()); | ||
| 420 | 809 | } | 830 | } |
| 421 | 810 | 831 | ||
| 422 | 811 | TEST_F(MetadataTest, wrong_type_for_size) | 832 | TEST_F(MetadataTest, wrong_type_for_size) |
| 423 | @@ -818,8 +839,9 @@ | |||
| 424 | 818 | spy.wait(SIGNAL_WAIT_TIME); | 839 | spy.wait(SIGNAL_WAIT_TIME); |
| 425 | 819 | 840 | ||
| 426 | 820 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 841 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 429 | 821 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: size_in_bytes: " | 842 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 430 | 822 | "expected value of type qlonglong, but received value of type QString", j->error().errorString()); | 843 | "size_in_bytes: expected value of type qlonglong, but received value of type QString", |
| 431 | 844 | j->error().errorString()); | ||
| 432 | 823 | } | 845 | } |
| 433 | 824 | 846 | ||
| 434 | 825 | TEST_F(MetadataTest, negative_size) | 847 | TEST_F(MetadataTest, negative_size) |
| 435 | @@ -832,8 +854,9 @@ | |||
| 436 | 832 | spy.wait(SIGNAL_WAIT_TIME); | 854 | spy.wait(SIGNAL_WAIT_TIME); |
| 437 | 833 | 855 | ||
| 438 | 834 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 856 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 441 | 835 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider: size_in_bytes: " | 857 | EXPECT_EQ("LocalCommsError: Account::get(): received invalid metadata from provider (id = child_id): " |
| 442 | 836 | "expected value >= 0, but received -1", j->error().errorString()); | 858 | "size_in_bytes: expected value >= 0, but received -1", |
| 443 | 859 | j->error().errorString()); | ||
| 444 | 837 | } | 860 | } |
| 445 | 838 | 861 | ||
| 446 | 839 | TEST_F(DeleteTest, basic) | 862 | TEST_F(DeleteTest, basic) |
| 447 | @@ -1485,7 +1508,7 @@ | |||
| 448 | 1485 | auto arg = spy.takeFirst(); | 1508 | auto arg = spy.takeFirst(); |
| 449 | 1486 | EXPECT_EQ(ItemListJob::Status::Error, qvariant_cast<ItemListJob::Status>(arg.at(0))); | 1509 | EXPECT_EQ(ItemListJob::Status::Error, qvariant_cast<ItemListJob::Status>(arg.at(0))); |
| 450 | 1487 | 1510 | ||
| 452 | 1488 | EXPECT_EQ("Item::parents(): provider returned a file as a parent", j->error().message()); | 1511 | EXPECT_EQ("Item::parents(): provider returned a file as a parent (id = root_id)", j->error().message()); |
| 453 | 1489 | } | 1512 | } |
| 454 | 1490 | } | 1513 | } |
| 455 | 1491 | 1514 | ||
| 456 | @@ -1751,7 +1774,9 @@ | |||
| 457 | 1751 | EXPECT_FALSE(j->isValid()); | 1774 | EXPECT_FALSE(j->isValid()); |
| 458 | 1752 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 1775 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 459 | 1753 | EXPECT_EQ(StorageError::Type::LocalCommsError, j->error().type()); | 1776 | EXPECT_EQ(StorageError::Type::LocalCommsError, j->error().type()); |
| 461 | 1754 | EXPECT_EQ("LocalCommsError: Item::copy(): source and target item type differ", j->error().errorString()); | 1777 | EXPECT_EQ("LocalCommsError: Item::copy()provider error: source and target item type differ " |
| 462 | 1778 | "(source id = child_id, target id = new_item_id)", | ||
| 463 | 1779 | j->error().errorString()); | ||
| 464 | 1755 | } | 1780 | } |
| 465 | 1756 | 1781 | ||
| 466 | 1757 | TEST_F(MoveTest, basic) | 1782 | TEST_F(MoveTest, basic) |
| 467 | @@ -1863,7 +1888,8 @@ | |||
| 468 | 1863 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 1888 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 469 | 1864 | EXPECT_EQ(StorageError::Type::LocalCommsError, j->error().type()); | 1889 | EXPECT_EQ(StorageError::Type::LocalCommsError, j->error().type()); |
| 470 | 1865 | EXPECT_EQ("LocalCommsError", j->error().name()); | 1890 | EXPECT_EQ("LocalCommsError", j->error().name()); |
| 472 | 1866 | EXPECT_EQ("LocalCommsError: Item::move(): impossible root item returned by provider", j->error().errorString()); | 1891 | EXPECT_EQ("LocalCommsError: Item::move(): impossible root item returned by provider (id = root_id)", |
| 473 | 1892 | j->error().errorString()); | ||
| 474 | 1867 | } | 1893 | } |
| 475 | 1868 | 1894 | ||
| 476 | 1869 | TEST_F(MoveTest, type_mismatch) | 1895 | TEST_F(MoveTest, type_mismatch) |
| 477 | @@ -1899,7 +1925,8 @@ | |||
| 478 | 1899 | EXPECT_FALSE(j->isValid()); | 1925 | EXPECT_FALSE(j->isValid()); |
| 479 | 1900 | EXPECT_EQ(ItemJob::Status::Error, j->status()); | 1926 | EXPECT_EQ(ItemJob::Status::Error, j->status()); |
| 480 | 1901 | EXPECT_EQ(StorageError::Type::LocalCommsError, j->error().type()); | 1927 | EXPECT_EQ(StorageError::Type::LocalCommsError, j->error().type()); |
| 482 | 1902 | EXPECT_EQ("LocalCommsError: Item::move(): provider error: source and target item type differ", | 1928 | EXPECT_EQ("LocalCommsError: Item::move(): provider error: source and target item type differ " |
| 483 | 1929 | "(source id = child_id, target id = child_id)", | ||
| 484 | 1903 | j->error().errorString()); | 1930 | j->error().errorString()); |
| 485 | 1904 | } | 1931 | } |
| 486 | 1905 | 1932 | ||
| 487 | @@ -2094,7 +2121,7 @@ | |||
| 488 | 2094 | auto status = qvariant_cast<ItemJob::Status>(arg.at(0)); | 2121 | auto status = qvariant_cast<ItemJob::Status>(arg.at(0)); |
| 489 | 2095 | EXPECT_EQ(ItemJob::Status::Error, status); | 2122 | EXPECT_EQ(ItemJob::Status::Error, status); |
| 490 | 2096 | 2123 | ||
| 492 | 2097 | EXPECT_EQ("LocalCommsError: Item::createFolder(): impossible file item returned by provider", | 2124 | EXPECT_EQ("LocalCommsError: Item::createFolder(): impossible file item returned by provider (id = new_folder_id)", |
| 493 | 2098 | j->error().errorString()); | 2125 | j->error().errorString()); |
| 494 | 2099 | } | 2126 | } |
| 495 | 2100 | 2127 | ||
| 496 | @@ -2157,7 +2184,10 @@ | |||
| 497 | 2157 | QSignalSpy ready_spy(j.get(), &ItemListJob::itemsReady); | 2184 | QSignalSpy ready_spy(j.get(), &ItemListJob::itemsReady); |
| 498 | 2158 | QSignalSpy status_spy(j.get(), &ItemListJob::statusChanged); | 2185 | QSignalSpy status_spy(j.get(), &ItemListJob::statusChanged); |
| 499 | 2159 | status_spy.wait(SIGNAL_WAIT_TIME); | 2186 | status_spy.wait(SIGNAL_WAIT_TIME); |
| 501 | 2160 | ASSERT_EQ(0, ready_spy.count()); | 2187 | if (ready_spy.count() == 0) |
| 502 | 2188 | { | ||
| 503 | 2189 | ASSERT_TRUE(ready_spy.wait(SIGNAL_WAIT_TIME)); | ||
| 504 | 2190 | } | ||
| 505 | 2161 | 2191 | ||
| 506 | 2162 | EXPECT_EQ(ItemListJob::Status::Finished, j->status()); | 2192 | EXPECT_EQ(ItemListJob::Status::Finished, j->status()); |
| 507 | 2163 | 2193 | ||
| 508 | @@ -2303,7 +2333,8 @@ | |||
| 509 | 2303 | auto arg = spy.takeFirst(); | 2333 | auto arg = spy.takeFirst(); |
| 510 | 2304 | EXPECT_EQ(ItemListJob::Status::Error, qvariant_cast<ItemListJob::Status>(arg.at(0))); | 2334 | EXPECT_EQ(ItemListJob::Status::Error, qvariant_cast<ItemListJob::Status>(arg.at(0))); |
| 511 | 2305 | 2335 | ||
| 513 | 2306 | EXPECT_EQ("LocalCommsError: Item::list(): impossible root item returned by provider", j->error().errorString()); | 2336 | EXPECT_EQ("LocalCommsError: Item::list(): impossible root item returned by provider (id = child_id)", |
| 514 | 2337 | j->error().errorString()); | ||
| 515 | 2307 | } | 2338 | } |
| 516 | 2308 | 2339 | ||
| 517 | 2309 | TEST_F(ListTest, runtime_destroyed_while_item_list_job_running) | 2340 | TEST_F(ListTest, runtime_destroyed_while_item_list_job_running) |
| 518 | @@ -3370,7 +3401,8 @@ | |||
| 519 | 3370 | 3401 | ||
| 520 | 3371 | EXPECT_EQ(Uploader::Status::Error, uploader->status()); | 3402 | EXPECT_EQ(Uploader::Status::Error, uploader->status()); |
| 521 | 3372 | EXPECT_EQ(StorageError::Type::LocalCommsError, uploader->error().type()); | 3403 | EXPECT_EQ(StorageError::Type::LocalCommsError, uploader->error().type()); |
| 523 | 3373 | EXPECT_EQ("Item::createUploader(): impossible folder item returned by provider", uploader->error().message()); | 3404 | EXPECT_EQ("Item::createUploader(): impossible folder item returned by provider (id = some_id)", |
| 524 | 3405 | uploader->error().message()); | ||
| 525 | 3374 | } | 3406 | } |
| 526 | 3375 | 3407 | ||
| 527 | 3376 | TEST_F(UploadTest, cancel_success) | 3408 | TEST_F(UploadTest, cancel_success) |
| 528 | @@ -3702,7 +3734,8 @@ | |||
| 529 | 3702 | EXPECT_EQ(Uploader::Status::Error, qvariant_cast<Uploader::Status>(arg.at(0))); | 3734 | EXPECT_EQ(Uploader::Status::Error, qvariant_cast<Uploader::Status>(arg.at(0))); |
| 530 | 3703 | 3735 | ||
| 531 | 3704 | EXPECT_EQ(StorageError::Type::LocalCommsError, uploader->error().type()); | 3736 | EXPECT_EQ(StorageError::Type::LocalCommsError, uploader->error().type()); |
| 533 | 3705 | EXPECT_EQ("Item::createFile(): impossible folder item returned by provider", uploader->error().message()); | 3737 | EXPECT_EQ("Item::createFile(): impossible folder item returned by provider (id = some_id)", |
| 534 | 3738 | uploader->error().message()); | ||
| 535 | 3706 | } | 3739 | } |
| 536 | 3707 | 3740 | ||
| 537 | 3708 | TEST_F(CreateFileTest, exists) | 3741 | TEST_F(CreateFileTest, exists) |
I've left a few inline comments.
Also, do we want to make the "unconditionally emit itemsReady" change to MultiItemListJob too?