[auth][bugfix] Auth DB migrate #5546

elpaso · 2017-11-06T12:43:59Z

Description

Add a migration step that copies the qgis authentication DB to the new profile folder.

Fixes https://issues.qgis.org/issues/17403 (Authentication DB is not migrated from 2 to 3)

m-kuhn · 2017-11-06T12:56:28Z

src/app/qgsversionmigration.cpp

+  QgsError authDbErrors = migrateAuthDb();
+  if ( !authDbErrors.isEmpty() )
+  {
+    for ( const auto &err : authDbErrors.messageList( ) )


you'll need to make a const temporary copy of the list or stick to Q_FOREACH. Same above.

@m-kuhn even if messageList is const (or at least that was it was mean to be) ?

I thought so, but might really be that the const works here based on some research I did right now.

Even if this works, I would prefer to have this copy in place. The reason is, that if this is done wrong we have an expensive detach and detecting this in code reviews is impossible if at the place where it's used no copy is taken.

The alternative would be that someone gets ground truth for the iterate-on-const-return first and then modifies the whole code-source accordingly so we can assume this while reviewing.

m-kuhn · 2017-11-06T12:58:06Z

src/app/qgsversionmigration.cpp

+  else
+  {
+    QgsDebugMsg( QStringLiteral( "OLD AUTH DB FILE %1" ).arg( oldAuthDbFilePath ) );
+    QgsDebugMsg( QStringLiteral( "NEW AUTH DB FILE %1" ).arg( newAuthDbFilePath ) );


That should probably stay quiet, there's enough info below I guess

m-kuhn · 2017-11-06T12:58:43Z

src/core/qgserror.h

+     * \brief messageList return the list of current error messages
+     * \return current list of error messages
+     */
+    const QList<QgsErrorMessage> messageList() { return mMessageList; }


That const here should be moved to the end of the signature.

Are you sure that we don't want to "add" a const after the signature?
I wanted the getter to return a const list too, to feed the range loops.

Yes, the idea was to add one after the signature. And to remove the one before, but that's already discussed above.

m-kuhn · 2017-11-06T14:15:22Z

src/core/qgserror.h

@@ -132,7 +132,7 @@ class CORE_EXPORT QgsError
     * \brief messageList return the list of current error messages
     * \return current list of error messages
     */
-    const QList<QgsErrorMessage> messageList() { return mMessageList; }
+    const QList<QgsErrorMessage> messageList() const { return mMessageList; }


Thanks @elpaso
Did you review if the const for the return type serves a purpose? I am asking because so far we always asked devs to drop it and it would be nice to have this done consistently throughout the codebase.

I was convinced that returning a const here allowed you to range loop iterate on the list without the need to make a const copy of it. But I might be wrong. Let me check if I can get an answer from a debugger.
Btw, I'm now making a const copy as you requested so this const is now unnecessary, but I'm still convinced that if the const copy is not needed we should not make it.

I assume even if we don't explicitly make a copy, a temporary for return will be created implicitly anyway, but looking forward to your findings 👍

const QList<QgsErrorMessage> errorList( stylesErrors.messageList( ) ); for ( const auto &err : errorList ) { errors.append( err ); } for ( const auto &err : stylesErrors.messageList( ) ) { errors.append( err ); }

@m-kuhn what I've found is that the instance address or err in the two loops is exactly the same when the messageList returns a const, but it is different if I remove the const from the signature (the one in front of it).

I think we can conclude that returning a const list allows to range loop iterate without the need to make a const copy of the list.

Btw, I guess you are right about the temporary (should we return a reference?) but to my limited understanding what matters here are the instances in the list, that are not copied, not the container itself.

Might be just my understanding:

What that means is, that if you compile with your compiler it actually does make a difference. And this might indeed be backed up by the C++ standard (from C++0x on).

What this also means is, that with the const copy of the list we are always on the safe side, since

... what matters here are the instances in the list, that are not copied, not the container itself.

Unless of course we modify all our return values from QList<X> to const QList<X>, then we will also be on the safe side with the other approach. But only for QGIS internal functions, I think the whole Qt codebase returns non-const lists.

no problems to remove the first const if that's the "standard" way for Qt to return lists.

I checked with Qt upstream, that's the response:

Making returned containers const inhibits move semantics, because const rvalues do not bind to the mutable rvalue references that move constructors and move assignment operators use.

NathanW2

Thanks. This looks fine to me logic wise. Thanks for adding the merge error message logic for me as well.

NathanW2 · 2017-11-07T21:22:28Z

Thanks a lot of tackling this one.

…

On Tue, Nov 7, 2017 at 5:26 PM, Alessandro Pasotti ***@***.*** > wrote: Merged #5546 <#5546>. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#5546 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXS3L2TeJ5-JbB4eiJieKWQeMhGGhWtks5s0AYIgaJpZM4QTL0u> .

elpaso · 2017-11-08T07:23:03Z

Thank you for taking care of our beloved users :)

elpaso added 3 commits November 6, 2017 12:42

[auth][bugfix] Add a getter for error list on qgsmigrate

cd23779

[auth][bugfix] Migrate qgis-auth.db from QGIS 2 to 3

7e69936

Fixes qgis#17403

Minor change: QString -> QStringLiteral in some debug messages

d9b4ae5

elpaso requested a review from NathanW2 November 6, 2017 12:43

m-kuhn reviewed Nov 6, 2017

View reviewed changes

const range loop iterators

af57600

m-kuhn reviewed Nov 6, 2017

View reviewed changes

getter return not-const list of errors

b57ec46

NathanW2 approved these changes Nov 6, 2017

View reviewed changes

elpaso merged commit 7c9cd49 into qgis:master Nov 7, 2017

elpaso deleted the auth_migrate branch November 7, 2017 07:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[auth][bugfix] Auth DB migrate #5546

[auth][bugfix] Auth DB migrate #5546

elpaso commented Nov 6, 2017

m-kuhn Nov 6, 2017

elpaso Nov 6, 2017

m-kuhn Nov 6, 2017 •

edited

Loading

m-kuhn Nov 6, 2017

m-kuhn Nov 6, 2017

elpaso Nov 6, 2017

m-kuhn Nov 6, 2017

m-kuhn Nov 6, 2017

elpaso Nov 6, 2017

m-kuhn Nov 6, 2017

elpaso Nov 6, 2017

m-kuhn Nov 6, 2017

elpaso Nov 6, 2017

m-kuhn Nov 9, 2017

NathanW2 left a comment

NathanW2 commented Nov 7, 2017 via email

elpaso commented Nov 8, 2017

[auth][bugfix] Auth DB migrate #5546

[auth][bugfix] Auth DB migrate #5546

Conversation

elpaso commented Nov 6, 2017

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-kuhn Nov 6, 2017 • edited Loading

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

NathanW2 left a comment

Choose a reason for hiding this comment

NathanW2 commented Nov 7, 2017 via email

elpaso commented Nov 8, 2017

m-kuhn Nov 6, 2017 •

edited

Loading