Merge lp:~3v1n0/u1db-qt/uri-path-parsing into lp:u1db-qt

Proposed by Marco Trevisan (Treviño) on 2015-02-28
Status: Merged
Approved by: Christian Dywan on 2015-03-05
Approved revision: 126
Merged at revision: 128
Proposed branch: lp:~3v1n0/u1db-qt/uri-path-parsing
Merge into: lp:u1db-qt
Diff against target: 174 lines (+70/-17)
4 files modified
examples/u1db-qt-example-1/u1db-qt-example-1.qml (+1/-1)
src/database.cpp (+35/-13)
src/database.h (+4/-1)
tests/test-database.cpp (+30/-2)
To merge this branch: bzr merge lp:~3v1n0/u1db-qt/uri-path-parsing
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2015-03-05
Christian Dywan 2015-02-28 Approve on 2015-03-05
Review via email: mp+251369@code.launchpad.net

Commit message

Database: support parsing of URI paths using something such as "file://"+ path

Description of the change

Make the database to parse and use local URIs starting with file://

Unfortunately this does not fix lp:1426178, which seems related to something
else.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Christian Dywan (kalikiana) wrote :

It seems unexpected to me that passing a URI would set the property to a different value - an API consumer should be able to assume that you can get back what you set. So initializeIfNeeded should be where the URI is supported, which is also where :memory: is special-cased. And maybe in the unit test we can check that there's no error in addition to comparing the path?

review: Needs Fixing
lp:~3v1n0/u1db-qt/uri-path-parsing updated on 2015-03-05
125. By Marco Trevisan (Treviño) on 2015-03-05

Database: don't change the internal m_path value when sanitizing, just keep the user value

And sanitize it when creating the Db

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Christian Dywan (kalikiana) wrote :

You left out the bit where the folder is being created. That is needed.

review: Needs Fixing
lp:~3v1n0/u1db-qt/uri-path-parsing updated on 2015-03-05
126. By Marco Trevisan (Treviño) on 2015-03-05

TestDatabase: open temporary files, to actually create paths

And update QUrl test

Christian Dywan (kalikiana) wrote :

I didn't realize how the change of the if/else in fact deduplicate code here, so in fact it's all good now and a nice clean-up on top of it! Thanks a lot!

review: Approve
lp:~3v1n0/u1db-qt/uri-path-parsing updated on 2015-03-05
127. By Marco Trevisan (Treviño) on 2015-03-05

Database: keep the ":memory:" special path saved in Database::MEMORY_PATH

128. By Marco Trevisan (Treviño) on 2015-03-05

Database: make sure that setting an empty path, moves the db to memory

Marco Trevisan (Treviño) (3v1n0) wrote :

Cool... On a side: is there any reason why the path is not set as ":memory:" by default (but as an empty string)?

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Christian Dywan (kalikiana) wrote :

The original reasoning is this: If no path is set the database is in memory. Unlike the Python implementation the code path is exactly the same as using a file, relying on sqlite's support for :memory:. Only the explicit support for relative paths and now URIs requires code that is aware of the :memory: case. API users don't have to know about it either.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'examples/u1db-qt-example-1/u1db-qt-example-1.qml'
2--- examples/u1db-qt-example-1/u1db-qt-example-1.qml 2013-05-02 18:20:33 +0000
3+++ examples/u1db-qt-example-1/u1db-qt-example-1.qml 2015-03-05 17:51:40 +0000
4@@ -36,7 +36,7 @@
5
6 U1db.Database {
7 id: aDatabase
8- path: "aDatabase1"
9+ path: "file:///tmp/aDatabase1.db";
10 }
11
12 /*!
13
14=== modified file 'src/database.cpp'
15--- src/database.cpp 2015-02-19 10:37:54 +0000
16+++ src/database.cpp 2015-03-05 17:51:40 +0000
17@@ -24,6 +24,7 @@
18 #include <QStandardPaths>
19 #include <QDir>
20 #include <QSqlError>
21+#include <QUrl>
22 #include <QUuid>
23 #include <QStringList>
24 #include <QJsonDocument>
25@@ -34,6 +35,8 @@
26
27 QT_BEGIN_NAMESPACE_U1DB
28
29+const QString Database::MEMORY_PATH = ":memory:";
30+
31 namespace
32 {
33 class ScopedTransaction
34@@ -85,6 +88,32 @@
35 }
36
37 /*!
38+ Sanitize path
39+ */
40+QString Database::sanitizePath(const QString& path)
41+{
42+ if (path == Database::MEMORY_PATH)
43+ return path;
44+
45+ if (!path.count())
46+ return Database::MEMORY_PATH;
47+
48+ QUrl url(path);
49+
50+ if (url.isValid() && url.isLocalFile())
51+ {
52+ return url.path();
53+ }
54+ else if (QDir::isRelativePath(path))
55+ {
56+ QString dataPath(QStandardPaths::writableLocation(QStandardPaths::DataLocation));
57+ return QDir(dataPath).absoluteFilePath(path);
58+ }
59+
60+ return path;
61+}
62+
63+/*!
64 Checks if the underlying SQLite database is ready to be used
65 Only to be used as a utility function by initializeIfNeeded()
66 */
67@@ -140,24 +169,17 @@
68 if (!m_db.isValid())
69 return setError("QSqlDatabase error");
70
71- if (path != ":memory:" && QDir::isRelativePath(path)) {
72- QString dataPath(QStandardPaths::writableLocation(QStandardPaths::DataLocation));
73- QString absolutePath(QDir(dataPath).absoluteFilePath(path));
74- QString parent(QFileInfo(absolutePath).dir().path());
75- if (!QDir().mkpath(parent))
76- setError(QString("Failed to make data folder %1").arg(parent));
77- m_db.setDatabaseName(absolutePath);
78- }
79- else
80+ if (path != Database::MEMORY_PATH)
81 {
82 QDir parent(QFileInfo(path).dir());
83 if (!parent.mkpath(parent.path()))
84 setError(QString("Failed to make parent folder %1").arg(parent.path()));
85- m_db.setDatabaseName(path);
86 }
87
88+ m_db.setDatabaseName(path);
89+
90 if (!m_db.open())
91- return setError(QString("Failed to open %1: %2").arg(path).arg(m_db.lastError().text()));
92+ return setError(QString("Failed to open '%1`: %2").arg(path).arg(m_db.lastError().text()));
93 if (!isInitialized())
94 {
95 if (!isInitialized())
96@@ -751,11 +773,11 @@
97
98 beginResetModel();
99 m_db.close();
100- initializeIfNeeded(path);
101+ initializeIfNeeded(sanitizePath(path));
102 endResetModel();
103
104 m_path = path;
105- Q_EMIT pathChanged(path);
106+ Q_EMIT pathChanged(m_path);
107 }
108
109 /*!
110
111=== modified file 'src/database.h'
112--- src/database.h 2014-01-24 11:13:35 +0000
113+++ src/database.h 2015-03-05 17:51:40 +0000
114@@ -77,13 +77,16 @@
115 void docLoaded(const QString& docId, QVariant content) const;
116 private:
117 //Q_DISABLE_COPY(Database)
118+ static const QString MEMORY_PATH;
119+
120 QString m_path;
121 QSqlDatabase m_db;
122 QString m_error;
123
124 QString getReplicaUid();
125+ QString sanitizePath(const QString& path);
126 bool isInitialized();
127- bool initializeIfNeeded(const QString& path=":memory:");
128+ bool initializeIfNeeded(const QString& path=Database::MEMORY_PATH);
129 bool setError(const QString& error);
130 QString getDocIdByRow(int row) const;
131
132
133=== modified file 'tests/test-database.cpp'
134--- tests/test-database.cpp 2014-11-07 14:30:31 +0000
135+++ tests/test-database.cpp 2015-03-05 17:51:40 +0000
136@@ -43,8 +43,36 @@
137 QCOMPARE(db.getPath(), QString(""));
138 QSignalSpy modelReset(&db, SIGNAL(pathChanged(const QString&)));
139 QTemporaryFile file;
140- db.setPath(file.fileName());
141- QCOMPARE(db.getPath(), file.fileName());
142+ QCOMPARE(file.open(), true);
143+ db.setPath(file.fileName());
144+ QCOMPARE(db.getPath(), file.fileName());
145+ QVERIFY(db.lastError().isEmpty());
146+ }
147+
148+ void testCanSetEmptyPath()
149+ {
150+ Database db;
151+ QCOMPARE(db.getPath(), QString());
152+ QSignalSpy modelReset(&db, SIGNAL(pathChanged(const QString&)));
153+ QTemporaryFile file;
154+ QCOMPARE(file.open(), true);
155+ db.setPath(file.fileName());
156+ QCOMPARE(db.getPath(), file.fileName());
157+ db.setPath("");
158+ QCOMPARE(db.getPath(), QString());
159+ QVERIFY(db.lastError().isEmpty());
160+ }
161+
162+ void testCanSetPathUsingQUrl()
163+ {
164+ Database db;
165+ QCOMPARE(db.getPath(), QString(""));
166+ QSignalSpy modelReset(&db, SIGNAL(pathChanged(const QString&)));
167+ QTemporaryFile file;
168+ QCOMPARE(file.open(), true);
169+ QString url = QUrl::fromLocalFile(file.fileName()).toString();
170+ db.setPath(url);
171+ QCOMPARE(db.getPath(), url);
172 }
173
174 void testNonExistingParentFolder()

Subscribers

People subscribed via source and target branches

to all changes: