Issue #7506
This is a regression introduced by the delta sync feature (as the chunk offset
changed from being the chunk number to be the byte offset, it needs to be a
qint64 now)
Previously the source was deleted (or attempted to be deleted), even if
the new location was not acceptable for upload. This could make data
unavilable on the server.
For #7410
By introducing a PropagateRootDirectory job that explicitly
separates the directory deletion jobs from all the other jobs.
Note that this means that if there are errors in subJobs the
dirDeletionJobs won't get executed.
Previously the pin states of deleted files stayed in the 'flags'
database and could be inadvertently reused when a new file with the same
name appeared. Now they are deleted.
To make this work right, the meaning of the 'path' column in the 'flags'
table was changed: Previously it never had the .owncloud file suffix.
Now it's the same as in metadata.path.
This takes the safe parts from #7274 for inclusion in 2.6. The more
elaborate database schema changes (why use 'path' the join the two
tables in the first place?) shall go into master.
Previously RequestEtagJob did return the etag verbatim (including extra
quotes) while the db had the parsed form. That caused the etag
comparison during discovery move detection to always fail. The test
didn't catch it because the etags there didn't have quotes.
Now:
- RequestEtagJob will parse the etag, leading to a consistent format
- Tests have etags with quotes, detecting the problem
If one adds a new file to an online-only folder the previous behavior
was to upload the file in one sync and dehydrate it in the next. Now
these new files get set to Unspecified pin state, making them retain
their data.
Since Qt does not yet transparently resend HTTP2 requests in some cases
we do it manually.
The test showed a problem where the initial non-200 reply would close
the target temporary file and the follow-up request couldn't store any
data. Removing that close() call is safe because there also is a
_saveBodyToFile flag that guards writes to the target file.
Previously these result codes during remote discovery of the sync root
would not cause an error and the discovery would get stuck.
Also extends RemoteDiscovery tests to check for errors on the root item.
The idea is that the user's question is "is this folder's data available
offline?" and not "does this folder have AlwaysLocal pin state?".
The the answers to the two questions can differ: an always-local
folder can have subitems that are not always-local and are dehydrated.
The new availability enum intends to describe the answer to the user's
actual question and can be derived from pin states. If pin states aren't
stored in the database the way of calculating availability will depend
on the vfs plugin.
The pin state is a per-item attribute that has an effect on _type:
AlwaysLocal dehydrated files will be marked for hydration and OnlineOnly
hydrated files will be marked for dehydration.
Where exactly this effect materializes depends on how the pin states are
stored. If they're stored in the db (suffix) the dbEntry._type is
changed during the discovery.
If the pin state is stored in the filesystem, the localEntry._type must
be adjusted by the plugin's stat callback.
This patch makes pin states behave more consistently between plugins.
Previously with suffix-vfs pin states only had an effect on new remote
files. Now the effect of pinning or unpinning files or directories is as
documented and similar to other plugins.
Previously removing the vfs suffix of a file always triggered a
conflict. Now it may just cause a file download.
This was done because users expected symmetry in the rename actions and
renaming foo -> foo.owncloud already triggers the "make the file
virtual" action. Now foo.owncloud -> foo triggers the "download the
contents" action.
Users can rename a file *and* add/remove the vfs suffix at the same time
leading to very complex sync actions. This patch doesn't add support for
them, but adds tests and makes sure these cases do not cause unintened
behavior.
The rename will be propagated, but the users's hydrate/dehydrate request
will be ignored.
This could fix a problem where the client incorrectly decides to delete
local data.
Previously any sqlite3_step() return value that wasn't SQLITE_ROW would
be interpreted as "there's no more data here". Thus an sqlite error at a
bad time could cause the remote discovery to fail to read an unchanged
subtree from the database. These files would then be deleted locally.
With this change sqlite errors from sqlite3_step are detected and
logged. For the particular case of SyncJournalDb::getFilesBelowPath()
the error will now be propagated and the sync run will fail instead of
performing spurious deletes.
Note that many other database functions still don't distinguish
not-found from error cases. Most of them won't have as severe effects on
affected sync runs though.
As far as I'm aware local discovery can be skipped on folders that are
selective-sync blacklisted, so a local discovery is required when an
entry is removed from the blacklist.
Also rename
avoidReadFromDbOnNextSync() -> schedulePathForRemoteDiscovery()
since the old name might also imply it's not read from db in the local
discovery - which is not the case. Use Folder::
schedulePathForLocalDiscovery() for that.
Creating a new virtual file and replacing a file with a virtual one now
have their own text in the protocol, not just "Downloaded".
To do this, the SyncFileItem type is kept as
ItemTypeVirtualFileDehydration for these actions. Added new code to
ensure the type isn't written to the database.
While looking at this, I've also added documentation on SyncFileItem's
_file, _renameTarget, _originalFile and destination() because some of
the semantics weren't clear.
That change will be useful for the notifications. Previously the
dehydrated files were reported as "newly downloaded", now they're
reported as "updated".
- SyncJournalDB functions now behind internalPinStates() to avoid
accidental usage, when nearly everyone should go through Vfs.
- Rename Vfs::getPinState() to Vfs::pinState()
Any folder with a (potentially deeply) contained error will have
StatusWarning. StatusExcluded marks exclusions. The difference is useful
to know for VFS.
On Linux and Windows the file watcher can't distinguish between changes
that were caused by the process itself, like during a sync operation,
and external changes. To work around that the client keeps a list of
files it has touched and blocks notifications on these files for a bit.
The duration of this block was originally and arbitrarily set at 15
seconds. During manual tests I regularly thought there was a bug when
syncs didn't trigger, when the only problem was that my changes happened
too close to a previous sync operation.
This change reduces the duration to three seconds. I imagine that this
is still enough.
Also use std::chrono while at it.
The idea is to allow folders (and later maybe files?) to be
- pinned to be available locally
- pinned to be online only
- inherit their pin from the parent
Where this pinning only controls the default for new files.
Subfolders may have a different pin state, and contained files
may be hydrated or dehydrated based on user actions.
This value is stored in a new 'flags' table. The idea is to store
data there that doesn't necessarily exist for each metadata entry.
The selective sync state could be migrated to this table.
This is just a port to QtTest, I did not change the layout of the test.
I did search and replace to replace the assert with QCOMPARE/QVERIFY
I still call setup and setup_init like before (only explicitly, now)
Also ported the preformence tests to QBENCHMAK because windows don't have
gettimeofday.
Relates #6358
This is to avoid issues on OSX, where the ._ prefix has special meaning.
Originally (before 2.3.2) ._ was necessary to guarantee exclusion. But
since then the .sync_ prefix is excluded as well.
This does not affect existing database files.
This helps support 2.5 settings where there are virtual files in the
tree but new files aren't created virtual.
It's also a prelude for #6815
There's currently no way of
- upgrading vfs plugins (a silent suffix->winvfs upgrade is attempted
once only, when moving to master)
- disabling vfs capabilities outright
Inh most case we already have a record from before, so avoid doing a useless
lookup in the database.
In owncloudpropagator.cpp, directories do not have a checksum so no need
to call a function that preserves it
And test the Remove/Remove case.
This means we need to always query the database for all the entries.
This showed another small bug in the test in which sync item for virtual
files at the root could have a slash in front of them.
Unfortunately to do this, the local update phase must write to the
database, creating a new side-effect and order dependency (local update
must run before remote update).
Fixes two bugs that appeared since the introduction of the struct:
- when reading permissions from the journal, null ("") was read as
empty-not-null
- when reading permissinos from the server, empty ("") was read as null
Addresses #4608
Installing to lib/${APPLICATION_EXECUTABLE} has caused a bunch of
irritations in the past and subtle annoying to fix bugs. To avoid name
clashes with branded clients ${APPLICATION_EXECUTABLE} becomes now
part of the filename instead of the subfolder.
The concrete motivation to change this now is that on Windows there
is no RPATH and it's not possible to run owncloud directly from the
Craft Root folder, which is nice when you're developing on Windows.
It would have been possible to change this just for Windows but as
written earlier this has caused lots of issues and thus I think it's
a good idea to just stay consistent accross platforms when touching it.
We want to keep the body so we can get the message from it
(Issue #6459)
TestDownload::testErrorMessage did not fail because the FakeErrorReply
did not emit readyRead and did not implement bytesAvailable.
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
Otherwise a interrupted or unsuccessful download would mean that the
download-intend was forgotten. The next sync would reestablish the
virtual file instead.
Issue #6420
Store the X-Request-ID in the SyncFileItem and also in the blacklist.
Note that for consistency reason, the X-Request-ID is also in the
SyncFileItem if the request succeeds.
Currently there is no UI to access it, but it can be queried with sql
commands
To do this, we add the placeholder extension to the user exclude file
automatically. However, newer clients shouldn't use that exclude
pattern: so we also add version directives that allow making exclude
patterns dependent on the client version.
Now the db entries for placeholders will have the full placeholder
paths. That way older clients will, on remote discovery, delete the
placeholders and download the real files.
- Controled by an option.
- New remote files start out as ItemTypePlaceholder, are created with a
.owncloud extension.
- When their db entry is set to ItemTypePlaceholderDownload the next
sync run will download them.
- Files that aren't in the placeholder state sync as usual.
- See test cases in testsyncplaceholders.
Missing:
- User ui for triggering placeholder file download
- Maybe: Going back from file to placeholder?
Previously it depended on addFolder() / removeFolder() calls to adjust
watchers when new folders were added or removed. There also needed to be
complex move handling.
Now, any folder creation/move-in notifications automatically trigger
watcher additions and folder deletion/move-out triggers removal.
If a folder was renamed A -> B, the folder watcher for the inode
would be unaware and still report changes for A/foo. Now directory
renames in the watched folders are tracked and paths are updated
accordingly.
The socket api move and delete commands are not strictly about conflicts
since they also deal with files which couldn't be uploaded for some
other reason. Still the new ConflictSolver could be used in those cases.
This opens the door at reusing that logic in other places.
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
For conflicts generally as well as new files in read-only directories
the context menu will now present delete and move options.
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
This flagged mostly unused parameters. Didn't enable the
misc-non-private-member-variables-in-classes check as we got a lot of
those. Hopefully we'll get to fix them at some point but that feels too
early and too much work for now.
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
Some of the comments didn't match the size or were missing. This also
means reducing one of the 150 MB payloads left behind reducing the
execution time by a few more seconds. This is now around 30s execution
time which is more acceptable.
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
This is about the conflicts that happens when the file has been uploaded
correctly to the server, but the etag was not recieved because the connection
was closed before we got the reply.
We used to compare only the mtime when comparing the uploaded file and the
existing file. However, to be perfectly correct, we also should check the
size.
This was found because TestChunkingNG::connectionDroppedBeforeEtagRecieved is
flaky. Example of faillure found in https://drone.owncloud.com/owncloud/client/481/5
while testing PR #6626
(very trimmed log:)
06-29 07:58:02:015 [ info sync.csync.csync ]: ## Starting local discovery ##
06-29 07:58:02:016 [ info sync.csync.updater ]: Database entry found, compare: 1530259082 <-> 1530259051, etag: <-> 1644a8c8750, inode: 1935629 <-> 1935629, size: 301 <-> 300, perms: 0 <-> ff, type: 0 <-> 0, checksum: <-> SHA1:cc9adedebe27a6259efb8d6ed09f4f2eff559ad1, ignore: 0
06-29 07:58:02:016 [ info sync.csync.updater ]: file: A/a0, instruction: INSTRUCTION_EVAL <<=
06-29 07:58:02:972 [ warning sync.networkjob ]: QNetworkReply::NetworkError(OperationCanceledError) "Connection timed out" QVariant(Invalid)
.. next sync...
06-29 07:58:02:980 [ info sync.engine ]: #### Discovery start ####################################################
06-29 07:58:02:981 [ info sync.csync.csync ]: ## Starting local discovery ##
06-29 07:58:02:983 [ info sync.csync.updater ]: Database entry found, compare: 1530259082 <-> 1530259051, etag: <-> 1644a8c8750, inode: 1935629 <-> 1935629, size: 302 <-> 300, perms: 0 <-> ff, type: 0 <-> 0, checksum: <-> SHA1:cc9adedebe27a6259efb8d6ed09f4f2eff559ad1, ignore: 0
06-29 07:58:02:983 [ info sync.csync.updater ]: file: A/a0, instruction: INSTRUCTION_EVAL <<=
06-29 07:58:02:985 [ info sync.csync.csync ]: ## Starting remote discovery ##
06-29 07:58:02:985 [ info sync.networkjob ]: OCC::LsColJob created for "http://localhost/owncloud" + "" "OCC::DiscoverySingleDirectoryJob"
06-29 07:58:02:987 [ info sync.csync.updater ]: Database entry found, compare: 1530259082 <-> 1530259051, etag: 1644a8c8b26 <-> 1644a8c8750, inode: 0 <-> 1935629, size: 301 <-> 300, perms: ff <-> ff, type: 0 <-> 0, checksum: SHA1:5adcdac9608ae0811247f07f4cf1ab0a2ef99154 <-> SHA1:cc9adedebe27a6259efb8d6ed09f4f2eff559ad1, ignore: 0
06-29 07:58:02:987 [ info sync.csync.updater ]: file: A/a0, instruction: INSTRUCTION_EVAL <<=
06-29 07:58:02:989 [ info sync.csync.csync ]: Update detection for remote replica took 0.004 seconds walking 13 files
06-29 07:58:02:990 [ info sync.engine ]: #### Discovery end #################################################### 9 ms
06-29 07:58:02:990 [ info sync.database ]: Updating file record for path: "A/a0" inode: 1935629 modtime: 1530259082 type: 0 etag: "1644a8c8b26" fileId: "16383ea4" remotePerm: "WDNVCKR" fileSize: 301 checksum: "SHA1:cc9adedebe27a6259efb8d6ed09f4f2eff559ad1"
06-29 07:58:02:990 [ info sync.csync.reconciler ]: INSTRUCTION_UPDATE_METADATA client file: A/a0
06-29 07:58:02:990 [ info sync.csync.csync ]: Reconciliation for local replica took 0 seconds visiting 13 files.
06-29 07:58:02:990 [ info sync.csync.reconciler ]: INSTRUCTION_UPDATE_METADATA server dir: A
06-29 07:58:02:990 [ info sync.csync.csync ]: Reconciliation for remote replica took 0 seconds visiting 13 files.
06-29 07:58:02:990 [ info sync.engine ]: #### Reconcile end #################################################### 9 ms
06-29 07:58:02:990 [ info sync.database ]: Updating local metadata for: "A/a0" 1530259082 302 1935629
FAIL! : TestChunkingNG::connectionDroppedBeforeEtagRecieved(small file) '!fakeFolder.syncOnce()' returned FALSE. ()
Use smaller files so the test run faster.
Particulary usefull for TestChunkingNG::connectionDroppedBeforeEtagRecieved
Which had become so much slower after 2638332dc6
increased the timeout for bigger files
If we receive data without base64 encoding for encryption, it makes
sense to get it without base64 encoding out of decryption.
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
Make the codebase consistent, we already have a lot of implicit pointer comparisons.
Exception: Stay explicit on return's, example:
return _db != nullptr;
Signed-off-by: Michael Schuster <michael@schuster.ms>
We keep NULL in the pure C files in src/csync/std and test/csync.
We also replace Doxygen documentation referring to "NULL" to
"\c nullptr" (formatted as code).
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
This test was failing locally for me. Indeed, through QStandardPaths it
was finding the user settings of my production client and not having the
initial state it expected. Using QStandardPaths test mode then it starts
from a clean slate every time.
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
We simply use a static QObject using Q_GLOBAL_STATIC()
instead of allocating a leaking QObject on the heap.
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
In case of denormalized paths in the dav href (presence of . or .. in
the path) simple string startsWith comparison wasn't enough to know if
said href ended up in the right namespace. That's why we're now using
QUrl (pretending local file since we don't have a full URL in the href)
to normalize the path before comparison.
This could happen with broken proxies for instance where we would
wrongly validate the dav information resulting in potentially surprising
syncing and name collisions.
Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>
- When the the users logs because of 401 or 403 errors, it checks if the
server requested the remote wipe. If yes, locally deletes account and folders
connected to the account and notify the server. If no, proceeds to ask the
user to login again.
- The app password is restored in the keychain.
- WIP: The change also includes a test class for RemoteWipe.
Signed-off-by: Camila San <hello@camila.codes>
Add a test to test the data fingerprint feature make me realize it was broken.
The code was relying in the distinction between empty and null QByteArray,
but this was a bad idea as this difference is lost when going through QString.
Note that we also needed to adjust the server url to contains the user name
in the folder wizard. (As checkPathValidityForNewFolder expect the user name)
Issue #6654
Fix a strange warning seen on the log from the CI on Windows in
https://github.com/owncloud/client/pull/6621
The test shows, at the beginning
QObject::connect: No such signal DesktopServiceHook::destroyed(QObject*)
And crashes at the and.
My guess is that when QDesktopServices::setUrlHandler is called, the
QMetaObject is not yet initialized
But this is probably not the reason of the crash
It could happen that readyRead was emitted for incoming data while the
download was not yet finished. Then the network job could finish with
no more data arriving - so readyRead wasn't emitted again.
To fix this, the finished signal also gets connected to the readyRead
slot.
