maryjane
/
syncevolution
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

command line: revise usability checking of datastores 

When configuring a new sync config, the command line checks whether a
datastore is usable before enabling it. If no datastores were listed
explicitly, only the usable ones get enabled. If unusable datastores
were explicitly listed, the entire configure operation fails.

This check was based on listing databases, which turned out to be too
unspecific for the WebDAV backend: when "database" was set to some URL
which is good enough to list databases, but not a database URL itself,
the sources where configured with that bad URL.

Now a new SyncSource::isUsable() operation is used, which by default
just falls back to calling the existing Operations::m_isEmpty. In
practice, all sources either check their config in open() or the
m_isEmpty operation, so the source is usable if no error is
enountered.

For WebDAV, the usability check is skipped because it would require
contacting a remote server, which is both confusing (why does a local
configure operation need the server?) and could fail even for valid
configs (server temporarily down). The check was incomplete anyway
because listing databases gave a fixed help text response when no
credentials were given. For usability checking that should have
resulted in "not usable" and didn't.

The output during the check was confusing: it always said "listing
databases" without giving a reason why that was done. The intention
was to give some feedback while a potentially expensive operation
ran. Now the isUsable() method itself prints "checking usability" if
(and only if!) such a check is really done.

Sometimes datastores were checked even when they were about to be
configure as "disabled" already. Now checking such datastores is
skipped.
This commit is contained in:
Patrick Ohly 2014-08-20 15:16:12 +02:00
parent 045bf7aa5b
commit 8ac69096e8
7 changed files with 188 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
src/backends/webdav/WebDAVSource.cpp
View File

@ -1560,6 +1560,23 @@ bool WebDAVSource::isEmpty()
return revisions.empty();
}
bool WebDAVSource::isUsable()
{
// Traditionally, this check used getDatabases() which
// succeeded with a fixed response if no credentials were given.
// Only if credentials were give was the source really checked,
// and that check was broken because it did not really verify
// that a given "database" really was a collection.
//
// It was also surprising that a simple configure operation
// resulted in a server access.
//
// So let's pretend that all WebDAV sources are usable for now
// until we figure out a better approach.
return true;
}
void WebDAVSource::close()
{
m_session.reset();

1
src/backends/webdav/WebDAVSource.h
View File

@ -79,6 +79,7 @@ class WebDAVSource : public TrackingSyncSource, private boost::noncopyable
/* implementation of SyncSource interface */
virtual void open();
virtual bool isEmpty();
virtual bool isUsable();
virtual void close();
virtual Databases getDatabases();
void getSynthesisInfo(SynthesisInfo &info,

141
src/syncevo/Cmdline.cpp
View File

@ -1227,52 +1227,52 @@ bool Cmdline::run() {
sources.erase(entry);
}
// check whether the sync source works; this can
// take some time, so allow the user to abort
SE_LOG_INFO(NULL, "%s: looking for databases...",
source.c_str());
// Even if the peer config does not exist yet
// (fromScratch == true), the source config itself
// may already exist with a username/password
// using the keyring. Need to retrieve that
// password before using the source.
//
// We need to check for databases again here,
// because otherwise we don't know whether the
// source is usable. The "database" property can
// be empty in a usable source, and the "sync"
// property in some potential other peer config
// is not accessible.
PasswordConfigProperty::checkPasswords(to->getUserInterfaceNonNull(),
*to,
PasswordConfigProperty::CHECK_PASSWORD_SOURCE|
PasswordConfigProperty::CHECK_PASSWORD_RESOLVE_PASSWORD|
PasswordConfigProperty::CHECK_PASSWORD_RESOLVE_USERNAME,
boost::assign::list_of(source));
SyncSourceParams params(source, to->getSyncSourceNodes(source), to);
auto_ptr<SyncSource> syncSource(SyncSource::createSource(params, false, to.get()));
if (syncSource.get() == NULL) {
disable = "no backend available";
} else {
try {
SyncSource::Databases databases = syncSource->getDatabases();
if (databases.empty()) {
disable = "no database to synchronize";
// Only check the source if it is not already disabled.
if (selected ||
sourceConfig->getSync() != "disabled") {
// Even if the peer config does not exist yet
// (fromScratch == true), the source config itself
// may already exist with a username/password
// using the keyring. Need to retrieve that
// password before using the source.
//
// We need to check for databases again here,
// because otherwise we don't know whether the
// source is usable. The "database" property can
// be empty in a usable source, and the "sync"
// property in some potential other peer config
// is not accessible.
PasswordConfigProperty::checkPasswords(to->getUserInterfaceNonNull(),
*to,
PasswordConfigProperty::CHECK_PASSWORD_SOURCE|
PasswordConfigProperty::CHECK_PASSWORD_RESOLVE_PASSWORD|
PasswordConfigProperty::CHECK_PASSWORD_RESOLVE_USERNAME,
boost::assign::list_of(source));
SyncSourceParams params(source, to->getSyncSourceNodes(source), to);
auto_ptr<SyncSource> syncSource(SyncSource::createSource(params, false, to.get()));
if (syncSource.get() == NULL) {
disable = "no backend available";
} else {
try {
syncSource->open();
if (!syncSource->isUsable()) {
disable = "unusable";
}
} catch (...) {
std::string explanation;
Exception::handle(explanation, HANDLE_EXCEPTION_NO_ERROR);
disable = "backend failed: " + explanation;
}
} catch (...) {
std::string explanation;
Exception::handle(explanation, HANDLE_EXCEPTION_NO_ERROR);
disable = "backend failed: " + explanation;
}
} else {
disable = "inactive";
}
// Checking can take some time, so allow the user to abort.
s.checkForNormal();
SE_LOG_INFO(NULL, "%s: %s\n",
source.c_str(),
disable.empty() ? "okay" : disable.c_str());
}
// Do sanity checking of source (can it be enabled?),
// but only set the sync mode if configuring a peer.
// Verify usability of source (can it be enabled?).
// Only set the sync mode if configuring a peer.
// A context-only config doesn't have the "sync"
// property.
string syncMode;
@ -1281,13 +1281,31 @@ bool Cmdline::run() {
// and it cannot be enabled, otherwise disable it silently
if (selected) {
Exception::throwError(SE_HERE, source + ": " + disable);
} else if (disable == "unusable") {
// More detailed INFO message must have been
// printed by isUsable().
} else {
// Print out own check result.
SE_LOG_INFO(NULL, "%s: %s", source.c_str(), disable.c_str());
}
if (sourceConfig->getSync() != "disabled") {
syncMode = "disabled";
}
} else {
if (selected) {
// user absolutely wants it: enable even if off by default
ConfigProps filter = m_props.createSourceFilter(m_server, source);
ConfigProps::const_iterator sync = filter.find("sync");
syncMode = sync == filter.end() ? "two-way" : sync->second;
}
if (configureContext) {
SE_LOG_INFO(NULL, "%s: configuring datastore",
source.c_str());
} else {
SE_LOG_INFO(NULL, "%s: configuring datastore with sync mode '%s'",
source.c_str(),
syncMode.empty() ? sourceConfig->getSync().c_str() : syncMode.c_str());
}
syncMode = "disabled";
} else if (selected) {
// user absolutely wants it: enable even if off by default
ConfigProps filter = m_props.createSourceFilter(m_server, source);
ConfigProps::const_iterator sync = filter.find("sync");
syncMode = sync == filter.end() ? "two-way" : sync->second;
}
if (!syncMode.empty() &&
!configureContext) {
@ -3502,14 +3520,10 @@ protected:
"foobar@default", NULL);
cmdline.doit(false);
CPPUNIT_ASSERT_EQUAL(std::string(""), cmdline.m_out.str());
CPPUNIT_ASSERT_EQUAL(std::string("[INFO] addressbook: looking for databases...\n"
"[INFO] addressbook: okay\n"
"[INFO] calendar: looking for databases...\n"
"[INFO] calendar: okay\n"
"[INFO] memo: looking for databases...\n"
"[INFO] memo: okay\n"
"[INFO] todo: looking for databases...\n"
"[INFO] todo: okay\n"
CPPUNIT_ASSERT_EQUAL(std::string("[INFO] addressbook: inactive\n"
"[INFO] calendar: inactive\n"
"[INFO] memo: inactive\n"
"[INFO] todo: inactive\n"
"[ERROR] Unsupported value for the \"keyring\" property, no such keyring found: no-such-keyring"),
cmdline.m_err.str());
}
@ -3834,11 +3848,12 @@ protected:
CONFIG_CONTEXT_CUR_VERSION);
CPPUNIT_ASSERT_EQUAL_DIFF(expected, res);
// add calendar
// add unusable calendar: it is unusable because the file backend
// cannot create the directory
{
TestCmdline cmdline("--configure",
"--datastore-property", "database@foobar = file://tmp/test2",
"--datastore-property", "backend = calendar",
"--datastore-property", "database@foobar = file:///no-such-dir/test2",
"--datastore-property", "backend = file",
"@foobar",
"calendar",
NULL);
@ -3847,8 +3862,8 @@ protected:
res = scanFiles(root);
removeRandomUUID(res);
expected +=
"sources/calendar/config.ini:backend = calendar\n"
"sources/calendar/config.ini:database = file://tmp/test2\n"
"sources/calendar/config.ini:backend = file\n"
"sources/calendar/config.ini:database = file:///no-such-dir/test2\n"
"sources/calendar/config.ini:# databaseFormat = \n"
"sources/calendar/config.ini:# databaseUser = \n"
"sources/calendar/config.ini:# databasePassword = \n";
@ -3873,9 +3888,15 @@ protected:
boost::replace_all(expected,
"addressbook/config.ini:# databaseFormat = ",
"addressbook/config.ini:databaseFormat = text/x-vcard");
boost::replace_all(expected,
"calendar/config.ini:backend = calendar",
"calendar/config.ini:backend = file");
boost::replace_all(expected,
"calendar/config.ini:# database = ",
"calendar/config.ini:database = file://tmp/test2");
"calendar/config.ini:database = file:///no-such-dir/test2");
boost::replace_all(expected,
"calendar/config.ini:sync = two-way",
"calendar/config.ini:sync = disabled");
sortConfig(expected);
CPPUNIT_ASSERT_EQUAL_DIFF(expected, res);

1
src/syncevo/MapSyncSource.h
View File

@ -267,6 +267,7 @@ class MapSyncSource :
virtual void beginSync(const std::string &lastToken, const std::string &resumeToken);
virtual std::string endSync(bool success);
virtual bool isEmpty() { return dynamic_cast<SyncSource &>(*m_sub).getOperations().m_isEmpty(); }
virtual bool isUsable() { return dynamic_cast<SyncSource &>(*m_sub).isUsable(); }
virtual InsertItemResult insertItem(const std::string &luid, const std::string &item);
virtual void readItem(const std::string &luid, std::string &item);
virtual void deleteItem(const string &luid);

21
src/syncevo/SyncSource.cpp
View File

@ -26,6 +26,7 @@
#include <syncevo/SyncSource.h>
#include <syncevo/SyncContext.h>
#include <syncevo/util.h>
#include <syncevo/SuspendFlags.h>
#include <syncevo/SynthesisEngine.h>
#include <synthesis/SDK_util.h>
@ -249,6 +250,26 @@ void SyncSource::popSynthesisAPI() {
m_synthesisAPI.pop_back();
}
bool SyncSource::isUsable()
{
if (getOperations().m_isEmpty) {
try {
SE_LOG_INFO(getDisplayName(), "checking usability...");
getOperations().m_isEmpty();
return true;
} catch (...) {
std::string explanation;
Exception::handle(explanation, HANDLE_EXCEPTION_NO_ERROR);
SE_LOG_INFO(getDisplayName(), "%s", explanation.c_str());
return false;
}
} else {
// Cannot check, assume it is usable.
return true;
}
}
SourceRegistry &SyncSource::getSourceRegistry()
{
static SourceRegistry sourceRegistry;

25
src/syncevo/SyncSource.h
View File

@ -1967,6 +1967,31 @@ class SyncSource : virtual public SyncSourceBase, public SyncSourceConfig, publi
*/
virtual void open() = 0;
/**
* Checks whether the source as currently configured can access
* data. open() must have been called first.
*
* The default implementation calls Operations::m_isEmpty; this
* assumes that m_isEmpty really does something with the
* database. Derived classes which neither check their config in
* open() nor during m_isEmpty() should provide their own
* implementation of isUsable(). It might also be possible to
* check usability more efficiently.
*
* It is also possible to suppress the usability check by
* providing an implementation which always returns true, for
* example when checking would be too expensive or lead to
* unexpected operations (like accessing a remote server).
*
* If unusable, the implementation should print an INFO message
* explaining why the source is unusable. Whether that is an
* error will be determined by the caller.
*
* @return false if the source is definitely unusable, true if it
* is usable or might be
*/
virtual bool isUsable();
/**
* Returns the actual database that is in use. open() must
* have been called first.

59
test/test-dbus.py
View File

@ -5796,11 +5796,23 @@ class CmdlineUtil(DBusUtil):
return (out, err, s.returncode)
def sourceCheckOutput(self, sources=['addressbook', 'calendar', 'memo', 'todo']):
defSourceOutput = ("checking usability...", "configuring datastore with sync mode 'two-way'")
def sourceCheckOutput(self, sources=None):
'''returns the output produced by --configure when checking sources'''
if not (isinstance(sources, type([])) or isinstance(sources, type(()))):
sources = (sources,)
return ''.join(['[INFO] %s: looking for databases...\n[INFO] %s: okay\n' % (i, i) for i in sources])
if sources is None:
sources = [(source, self.defSourceOutput) for source in ('addressbook', 'calendar', 'memo', 'todo')]
elif not (isinstance(sources, type([])) or isinstance(sources, type(()))):
sources = [ (sources, self.defSourceOutput) ]
res = []
for i in sources:
source = i[0]
result = i[1]
if not (isinstance(result, type([])) or isinstance(result, type(()))):
result = [result]
for e in result:
res.append('[INFO] %s: %s\n' % (source, e))
return ''.join(res)
def assertNoErrors(self, err):
'''check that error output is empty'''
@ -6373,7 +6385,11 @@ spds/sources/todo/config.txt:# evolutionpassword =
out, err, code = self.runCmdline(['--configure',
'--sync-property', 'proxyHost = proxy',
'scheduleworld', 'addressbook'])
self.assertSilent(out, err, ignore=self.sourceCheckOutput('addressbook'))
self.assertSilent(out, err, ignore=self.sourceCheckOutput([('addressbook', ["checking usability...",
"configuring datastore with sync mode 'two-way'"]),
('calendar', 'not selected'),
('memo', 'not selected'),
('todo', 'not selected')]))
res = sortConfig(scanFiles(root))
res = self.removeRandomUUID(res)
expected = self.ScheduleWorldConfig()
@ -6473,7 +6489,10 @@ spds/sources/todo/config.txt:# evolutionpassword =
args.append("synthesis")
out, err, code = self.runCmdline(args)
self.assertSilent(out, err, ignore=self.sourceCheckOutput())
self.assertSilent(out, err, ignore=self.sourceCheckOutput([('addressbook', ["checking usability...", "configuring datastore with sync mode 'two-way'"]),
('calendar', 'inactive'),
('memo', ["checking usability...", "configuring datastore with sync mode 'two-way'"]),
('todo', 'inactive')]))
res = scanFiles(root, "synthesis")
expected = sortConfig(self.SynthesisConfig())
self.assertEqualDiff(expected, res)
@ -6916,8 +6935,8 @@ sources/xyz/config.ini:# databasePassword = """)
out, err, code = self.runCmdline(["--configure",
"--template", "yahoo",
"target-config@my-yahoo"])
# TODO: why does it check 'addressbook'? It's disabled in the template.
self.assertSilent(out, err, ignore=self.sourceCheckOutput(['addressbook', 'calendar']))
self.assertSilent(out, err, ignore=self.sourceCheckOutput([('addressbook', 'inactive'),
('calendar', "configuring datastore with sync mode 'two-way'")]))
out, err, code = self.runCmdline(["--print-config", "target-config@my-yahoo"])
self.assertNoErrors(err)
@ -6932,7 +6951,8 @@ sources/xyz/config.ini:# databasePassword = """)
# configure Google Calendar/Contacts with template derived from config name
out, err, code = self.runCmdline(["--configure",
"target-config@google"])
self.assertSilent(out, err, ignore=self.sourceCheckOutput(['addressbook', 'calendar']))
self.assertSilent(out, err, ignore=self.sourceCheckOutput([('addressbook', "configuring datastore with sync mode 'two-way'"),
('calendar', "configuring datastore with sync mode 'two-way'")]))
out, err, code = self.runCmdline(["--print-config", "target-config@google"])
self.assertNoErrors(err)
@ -7331,8 +7351,7 @@ syncevolution/default/sources/eds_event/config.ini:backend = calendar
expectSuccess = False)
self.assertEqualDiff('', out)
err = stripOutput(err)
self.assertEqualDiff(self.sourceCheckOutput('eds_event').replace('okay', 'no backend available') +
'[ERROR] error code from SyncEvolution fatal error (local, status 10500): eds_event: no backend available\n', err)
self.assertEqualDiff('[ERROR] error code from SyncEvolution fatal error (local, status 10500): eds_event: no backend available\n', err)
shutil.rmtree(self.configdir, True)
# allow user to proceed if they wish and possible: here
@ -7393,8 +7412,7 @@ syncevolution/default/sources/eds_event/config.ini:backend = calendar
"--source-property", "type = file:text/x-vcard",
"@foobar",
"addressbook"])
self.assertSilent(out, err, ignore=self.sourceCheckOutput('addressbook'))
self.assertSilent(out, err, ignore=self.sourceCheckOutput([('addressbook', ('checking usability...', 'configuring datastore'))]))
root = self.configdir + "/foobar"
res = self.removeRandomUUID(scanFiles(root))
expected = '''.internal.ini:contextMinVersion = {0}
@ -7414,15 +7432,16 @@ sources/addressbook/config.ini:# databasePassword =
# add calendar
out, err, code = self.runCmdline(["--configure",
"--source-property", "database@foobar = file://tmp/test2",
"--source-property", "backend = calendar",
"--source-property", "backend = file",
"--source-property", "databaseFormat = text/calendar",
"@foobar",
"calendar"])
self.assertSilent(out, err)
res = self.removeRandomUUID(scanFiles(root))
expected += '''sources/calendar/config.ini:backend = calendar
expected += '''sources/calendar/config.ini:backend = file
sources/calendar/config.ini:database = file://tmp/test2
sources/calendar/config.ini:# databaseFormat =
sources/calendar/config.ini:databaseFormat = text/calendar
sources/calendar/config.ini:# databaseUser =
sources/calendar/config.ini:# databasePassword =
'''
@ -7441,8 +7460,12 @@ sources/calendar/config.ini:# databasePassword =
"addressbook/config.ini:database = file://tmp/test")
expected = expected.replace("addressbook/config.ini:# databaseFormat = ",
"addressbook/config.ini:databaseFormat = text/x-vcard")
expected = expected.replace("calendar/config.ini:backend = calendar",
"calendar/config.ini:backend = file")
expected = expected.replace("calendar/config.ini:# database = ",
"calendar/config.ini:database = file://tmp/test2")
expected = expected.replace("calendar/config.ini:# databaseFormat = ",
"calendar/config.ini:databaseFormat = text/calendar")
expected = sortConfig(expected)
self.assertEqualDiff(expected, res)
@ -7465,7 +7488,9 @@ sources/calendar/config.ini:# databasePassword =
out, err, code = self.runCmdline(["--configure",
"--template", "SyncEvolution",
"--source-property", "addressbook/type=file:text/vcard:3.0",
"--source-property", "addressbook/database=file:///tmp/test",
"--source-property", "calendar/type=file:text/calendar:2.0",
"--source-property", "calendar/database=file:///tmp/test",
"syncevo@syncevo"])
self.assertSilent(out, err, ignore=self.sourceCheckOutput())
@ -7507,7 +7532,7 @@ sources/calendar/config.ini:# databasePassword =
if haveEDS:
# limit output to one specific backend, chosen via config
out, err, code = self.runCmdline(["--configure", "backend=evolution-contacts", "@foo-config", "bar-source"])
self.assertSilent(out, err, ignore=self.sourceCheckOutput('bar-source'))
self.assertSilent(out, err, ignore=self.sourceCheckOutput([('bar-source', ('checking usability...', 'configuring datastore'))]))
out, err, code = self.runCmdline(["--print-databases", "@foo-config", "bar-source"])
self.assertNoErrors(err)
self.assertTrue(out.startswith("@foo-config/bar-source:\n"))