Projects in PostgreSQL #6752

wonder-sk · 2018-04-05T14:44:14Z

This introduces the possibility to load/save QGIS projects inside PostgreSQL databases.

More details about the internals in the QEP: qgis/QGIS-Enhancement-Proposals#118

In Project menu, there are two new menu items:

Load From
Save To

Both menu items have a sub-menu with list of extra project storage implementations - currently just PostgreSQL. Clicking the action will open a dialog to pick PostgreSQL connection name, schema name and project.

Projects stored in PostgreSQL can be also loaded from browser panel (their entries are located within the schema they are stored in) - either by double-clicking them or by dragging them to main canvas.

Developed for Arpa Piemonte (Dipartimento Tematico Geologia e Dissesto) within ERIKUS project.

nyalldawson · 2018-04-05T20:17:34Z

python/core/qgsprojectstorage.sip.in

+
+    virtual bool removeProject( const QString &uri ) = 0;
+%Docstring
+Removes and existing project at the given URI. Returns true if the removal


nyalldawson · 2018-04-05T20:19:21Z

python/core/qgsprojectstorage.sip.in

+was successful.
+%End
+
+    virtual bool readProjectMetadata( const QString &uri, QgsProjectStorage::Metadata &metadata /Out/ );


I think this name could easily get mistaken for dealing with QgsProjectMetadata objects. Maybe change it to readProjectStorageMetadata instead?

nyalldawson · 2018-04-05T20:24:03Z

python/core/qgsprojectstorage.sip.in

+%End
+      public:
+        QString name;
+        QDateTime lastModified;


I think we should use a variant for lastModified checks and storage - it's possible some providers won't have access to a last modified time and may instead need to use a version number or project hash instead.

I would really prefer to stick with QDateTime - if there would be a storage implementation that can't support last modified time, I would prefer to introduce some other member(s) to accommodate more kinds of metadata (or just a QVariantMap for any extra stuff). Some implementations would keep the time value invalid in the worst case... Would that work for you? I am trying to avoid variants as much as possible as they suddenly make all the nice c++ type checks disappear...

I'd be happy to see a QVariantMap added, but that kind-of goes against your desire to avoid variants. I guess in either case we need a bool QgsProjectStorage::hasProjectChanged( const QgsProjectStorage::Metadata& metadata ) member in QgsProjectStorage, right?

I think we would need a hasProjectChanged() virtual method only if we were to introduce support for project storage implementation that cannot store last modified time for some reason. For now QGIS app just compares timestamps - in the future when needed the extension to use hasProjectChanged() should be fairly straightforward...

nyalldawson · 2018-04-05T20:25:19Z

src/app/qgisapp.cpp

+void QgisApp::populateProjectStorageMenu( QMenu *menu, bool saving )
+{
+  menu->clear();
+  for ( QgsProjectStorage *storage : QgsApplication::projectStorageRegistry()->projectStorages() )


This will detach (I suggest using qt creator 4.6 and enable clazy warnings)

thanks for the tip... looks like I should upgrade!

nyalldawson · 2018-04-05T20:26:32Z

src/app/qgisapp.cpp

+          if ( QgsProject::instance()->write() )
+          {
+            setTitleBarText_( *this ); // update title bar
+            mStatusBar->showMessage( tr( "Saved project to: %1" ).arg( uri ), 5000 );


Won't this get a bit annoying? I save every 10 seconds sometimes :p

We have the same kind of code in QgisApp::fileSave() and QgisApp::fileSaveAs() so I just brought it here as well for consistency... I am happy to remove it if you prefer - but this is just status bar text update - not a popup in the message bar

Ah - I read that wrong and thought it was message bar. In that case no issue here!

nyalldawson · 2018-04-05T20:42:18Z

src/core/qgsprojectstorageregistry.h

+ * Registry of storage backends that QgsProject may use.
+ * This is a singleton that should be accessed through QgsApplication::projectStorageRegistry().
+ *
+ * \since QGIS 3.2


ingroup core

nyalldawson · 2018-04-05T20:46:42Z

src/providers/postgres/qgspostgresprojectstorage.h

+class QgsPostgresProjectStorage : public QgsProjectStorage
+{
+  public:
+    QgsPostgresProjectStorage();


nyalldawson · 2018-04-05T20:47:25Z

src/providers/postgres/qgspostgresprojectstoragedialog.h

+
+  private:
+
+    bool mSaving;  //!< Whether using this dialog for loading or saving a project


nyalldawson · 2018-04-05T20:48:23Z

src/ui/qgisapp.ui

@@ -42,15 +42,27 @@
      <string>New From Template</string>
     </property>
    </widget>
+    <widget class="QMenu" name="mProjectToStorageMenu">
+     <property name="title">
+      <string>Save To</string>


I'm fairly certain HIG guidelines is not to capitalise to/from

I got inspiration from the existing "New From Template" string - going to change that one as well

Thanks - I tried to catch a lot of these a week or so ago, but I've obviously missed some.

nyalldawson · 2018-04-05T20:49:20Z

tests/src/core/testqgsprojectstorage.cpp

+  QCOMPARE( prj1.baseName(), QString( "project1" ) );
+  QVERIFY( prj1.lastModified().secsTo( QDateTime::currentDateTime() ) < 1 );
+
+  // read the project back


Can you add a test here which uses zipped contents too?

Good point - added in b8a9329

nyalldawson · 2018-04-05T20:52:30Z

Looks great!

One thing I'd like to see is a way to disable storage for specific postgres connections - I can see cases where admins would want postgres connections to be used for data only and not expose this new functionality to certain connections. Maybe a checkbox in the connection properties? (And maybe default to off for safety?)

luipir · 2018-04-06T09:28:24Z

src/providers/postgres/qgspostgresprojectstorage.cpp

+  QString username = u.userName();
+  QString password = u.password();
+  QgsDataSourceUri::SslMode sslMode = QgsDataSourceUri::SslPrefer;
+  QString authConfigId;  // TODO: ?


see blacklisted tests to manage authConfigId to integrate QGIS Auth infrastructure instead to use only basci auth.
btw in this moment it can be tested because we don't have any pki enable docker to test against (I'm slowly working on that when I've some spare time, sorry)

hi @luipir this is an older commit, the todo has been addressed already

ho sorry, I just saw it in c765295

wonder-sk · 2018-04-06T09:38:44Z

@nyalldawson Thanks a lot for the review!

One thing I'd like to see is a way to disable storage for specific postgres connections - I can see cases where admins would want postgres connections to be used for data only and not expose this new functionality to certain connections.

I would prefer that admins set up the permissions in the database rather than inside QGIS:

in QGIS users could still easily change the connection config, ignoring admin's policy
in QGIS the permissions would be very coarse, while in the database admins can manage permissions with much greater control, e.g. allow loading/saving of projects only in some schemas, allow only some users/groups to use the projects, make projects read-only by default, ...
for postgres we already create/use table layer_styles (and more could come, e.g. metadata table) which may also need a similar enable/disable option for consistency (but as said, I think this should be configured in postgres permissions)

already addressed

wonder-sk · 2018-04-06T14:47:45Z

Everything from the review should be addressed now...

nyalldawson · 2018-04-07T02:13:39Z

I would prefer that admins set up the permissions in the database rather than inside QGIS:

I totally agree that this is a critical part of the correct approach, but,...

in QGIS users could still easily change the connection config, ignoring admin's policy
in QGIS the permissions would be very coarse, while in the database admins can manage permissions with much greater control, e.g. allow loading/saving of projects only in some schemas, allow only some users/groups to use the projects, make projects read-only by default, ...
for postgres we already create/use table layer_styles (and more could come, e.g. metadata table) which may also need a similar enable/disable option for consistency (but as said, I think this should be configured in postgres permissions)

What I want to avoid is a feature being exposed in the ui which has been blocked on the database (via postgres permissions) and is not desired in an organisation. (I'd present the same argument if the database style store feature was a new PR too! That should also have a way to disable per connection).

I've got a couple of organisations I maintain where I would not want to see them using this feature (for various internal IT/maintenance reasons, not because of stability of your code or anything!) and for that reason I'd like to be able to disable this option for the organisation's data store via the scripts which setup the connection. I just know that there's users who will click these options and try to use it just because it's there... and I'd prefer that the option was disabled in the UI as opposed to users hitting an unfriendly database permission issue.

I think it's also a bit risky to suddenly add this to existing installs/connections - organisation's db/QGIS maintainers may not be aware that on installing 3.2 this option will suddenly become possible and their users can start storing projects in the databases, possibly before any proper backup/policies/etc in place. So my preference would be adding a checkbox for this, which defaults to disabled for existing connections but defaults to enabled when creating a new connection.

NathanW2 · 2018-04-07T03:58:56Z

I agree with Nyall on all those points rasied.

…

On Sat, 7 Apr. 2018, 12:13 pm Nyall Dawson, ***@***.***> wrote: I would prefer that admins set up the permissions in the database rather than inside QGIS: I totally agree that this is a critical part of the correct approach, but,... in QGIS users could still easily change the connection config, ignoring admin's policy in QGIS the permissions would be very coarse, while in the database admins can manage permissions with much greater control, e.g. allow loading/saving of projects only in some schemas, allow only some users/groups to use the projects, make projects read-only by default, ... for postgres we already create/use table layer_styles (and more could come, e.g. metadata table) which may also need a similar enable/disable option for consistency (but as said, I think this should be configured in postgres permissions) What I want to avoid is a feature being exposed in the ui which has been blocked on the database (via postgres permissions) and is not desired in an organisation. (I'd present the same argument if the database style store feature was a new PR too! That should also have a way to disable per connection). I've got a couple of organisations I maintain where I would not want to see them using this feature (for various internal IT/maintenance reasons, not because of stability of your code or anything!) and for that reason I'd like to be able to disable this option for the organisation's data store via the scripts which setup the connection. I just know that there's users who will click these options and try to use it just because it's there... and I'd prefer that the option was disabled in the UI as opposed to users hitting an unfriendly database permission issue. I think it's also a bit risky to suddenly add this to existing installs/connections - organisation's db/QGIS maintainers may not be aware that on installing 3.2 this option will suddenly become possible and their users can start storing projects in the databases, possibly before any proper backup/policies/etc in place. So my preference would be adding a checkbox for this, which defaults to disabled for existing connections but defaults to enabled when creating a new connection. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6752 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXS3M0W47Gza0m_uoiS5YFD3vmlG5w1ks5tmCDWgaJpZM4TIlZ1> .

This is going to be used as an abstraction of how/where project files are stored.

Switched from using URLs in mime data to URI list for drag-n-drop of projects so that they are handled in the same way as the other browser items.

…fied

Was working for files, now working for projects in postgres as well

wonder-sk · 2018-04-07T11:14:32Z

What I want to avoid is a feature being exposed in the ui which has been blocked on the database (via postgres permissions) and is not desired in an organisation.

I see your point and I do not really have a strong opinion about that, so let's have a per-connection opt-in configuration option for that... I assume you meant that users would not be able to write nor read projects in such connections (even if the qgis_projects table existed), right?

… impls

wonder-sk · 2018-04-07T15:28:04Z

Rebased, Nyall's comments addressed in 42969a4 (variables) and e80f604 (enable/disable postgres projects storage)

NathanW2 · 2018-04-09T09:46:57Z

Nice!

…

On Mon, Apr 9, 2018 at 5:53 PM, Martin Dobias ***@***.***> wrote: Merged #6752 <#6752>. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6752 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXS3JIbzePZMpv2LHbKtjP55DboZLCbks5tmxNugaJpZM4TIlZ1> .

nyalldawson · 2018-04-10T04:32:30Z

@wonder-sk

Found some issues while testing this today:

If you try to open a project from an unavailable connection (e.g. from recent projects list), you get a crash
QgsPostgresProjectStorage::readProject (and writeProject) pushes handy errors to the read write context, but these aren't used anywhere, and instead a generic "Unable to open %1" error is used instead. It would be better to pop the last error from the context here and use the more descriptive error instead.
QgsPostgresProjectStorage::readProject and writeProject have a bunch of untranslated strings when errors are pushed to the context.

wonder-sk · 2018-04-10T09:33:01Z

Thanks for testing @nyalldawson - fixed in 4a885e6

nyalldawson reviewed Apr 5, 2018

View reviewed changes

luipir previously requested changes Apr 6, 2018

View reviewed changes

wonder-sk force-pushed the projects-in-postgres branch from f18c6b3 to 2ed64ef Compare April 6, 2018 13:56

wonder-sk added 17 commits April 7, 2018 12:58

Introduced QgsProjectStorage and QgsProjectStorageRegistry

a30646f

This is going to be used as an abstraction of how/where project files are stored.

Extend project storage interface: remove/rename projects, GUI support

5963028

Implement project storage for PostgreSQL + tests of the storage

0f5ea53

Store last modified time in project storage + retrieve it

ab83455

GUI implementation to load/save projects in PostgreSQL

0c701fb

Added removal of projects

fe686d3

Support for project items in browser for PostgreSQL

2e847e2

Switched from using URLs in mime data to URI list for drag-n-drop of projects so that they are handled in the same way as the other browser items.

Handle ssl mode, service, authcfg in postgresql project URIs

47d5b7f

Add abstraction of project's absolute file path, base name, last modi…

15f7ec7

…fied

Recent projects work now for postgres projects

3c3de3f

Deprecate QgsProject::fileInfo()

2bbdc34

Handle the situation that the project was changed in meanwhile

4281140

Was working for files, now working for projects in postgres as well

Update postgres project storage unit test

d1464a0

Fix spelling and doxygen doc

3ccb6c3

Fixes after Nyall's review + doxygen fixes

2aabad9

Add a test to check that extra content in project ZIP is saved/loaded

17a8c45

Test fixes

663a9ac

wonder-sk added 2 commits April 7, 2018 16:56

Add project_basename to project expr context, project_path for custom…

42969a4

… impls

Make storage of projects in postgres opt-in for each connection

e80f604

wonder-sk force-pushed the projects-in-postgres branch from 2ed64ef to e80f604 Compare April 7, 2018 15:25

Help text for project_basename

d6e7043

wonder-sk merged commit f378a23 into qgis:master Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Projects in PostgreSQL #6752

Projects in PostgreSQL #6752

wonder-sk commented Apr 5, 2018

nyalldawson Apr 5, 2018

nyalldawson Apr 5, 2018

nyalldawson Apr 5, 2018

wonder-sk Apr 6, 2018

nyalldawson Apr 7, 2018

wonder-sk Apr 7, 2018

nyalldawson Apr 5, 2018

wonder-sk Apr 6, 2018

nyalldawson Apr 5, 2018

wonder-sk Apr 6, 2018

nyalldawson Apr 7, 2018

nyalldawson Apr 5, 2018

nyalldawson Apr 5, 2018

nyalldawson Apr 5, 2018

nyalldawson Apr 5, 2018

wonder-sk Apr 6, 2018

nyalldawson Apr 7, 2018

nyalldawson Apr 5, 2018

wonder-sk Apr 6, 2018

nyalldawson commented Apr 5, 2018

luipir Apr 6, 2018

wonder-sk Apr 6, 2018

luipir Apr 6, 2018

wonder-sk commented Apr 6, 2018

wonder-sk commented Apr 6, 2018

nyalldawson commented Apr 7, 2018

NathanW2 commented Apr 7, 2018 via email

wonder-sk commented Apr 7, 2018

wonder-sk commented Apr 7, 2018

NathanW2 commented Apr 9, 2018 via email

nyalldawson commented Apr 10, 2018

wonder-sk commented Apr 10, 2018


		private:

		bool mSaving; //!< Whether using this dialog for loading or saving a project

Projects in PostgreSQL #6752

Projects in PostgreSQL #6752

Conversation

wonder-sk commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyalldawson commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wonder-sk commented Apr 6, 2018

wonder-sk commented Apr 6, 2018

nyalldawson commented Apr 7, 2018

NathanW2 commented Apr 7, 2018 via email

wonder-sk commented Apr 7, 2018

wonder-sk commented Apr 7, 2018

NathanW2 commented Apr 9, 2018 via email

nyalldawson commented Apr 10, 2018

wonder-sk commented Apr 10, 2018