Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server: revisit server exceptions #3985

Merged
merged 2 commits into from Jan 16, 2017
Merged

Conversation

@dmarteau
Copy link
Contributor

dmarteau commented Jan 12, 2017

This PR brings some changes on how to define exceptions in qgis server:

  • Add server exception base class for a more homogoneous exception handling.
  • Enable per service exception definition: each service may define the response format (handle OGC exception versioning that depends on the invoked service)
  • Handle QgsException gracefully (error 500)

#ifdef HAVE_SERVER_PYTHON_PLUGINS
// XXX Why this is enabled only fol plugins ?
sServerInterface->setConfigFilePath( configFilePath );
// XXX Why this is enabled only for plugins ?

This comment has been minimized.

@elpaso

elpaso Jan 12, 2017 Contributor

because server interface does not exist if plugins are disabled

This comment has been minimized.

@dmarteau

dmarteau Jan 12, 2017 Author Contributor

So this directive has no purpose now: server interface always exists now (passed to service at init time).

This comment has been minimized.

@elpaso

elpaso Jan 13, 2017 Contributor

That's correct, now that the plugin architecture is spread all over the server we can remove some of the conditional compilation switches.
I'll have a look to the other points.

This comment has been minimized.

@elpaso

elpaso Jan 13, 2017 Contributor

@dmarteau that make me think: you should make sure that the server can still be built with WITH_SERVER_PLUGINS disabled.

This comment has been minimized.

@elpaso

elpaso Jan 13, 2017 Contributor

Removed the conditional in
0f7dfc9

@dmarteau dmarteau force-pushed the dmarteau:revisit_server_exceptions branch from f7841fb to e04b5d4 Jan 12, 2017
#include <QFileInfo>
#include <QTextStream>

QString QgsMapServerUtils::createTempFilePath()

This comment has been minimized.

@nyalldawson

nyalldawson Jan 13, 2017 Collaborator

I'm very glad to see these methods go - Coverity has been flagging them as unsafe for as long as we've been running scan over QGIS!

private:
QString mCode;
QString mMessage;
QgsMapServiceException( const QString& code, const QString& message )

This comment has been minimized.

@nyalldawson

nyalldawson Jan 13, 2017 Collaborator

Why not just remove this class totally and document the api break in doc/api_break.dox?

This comment has been minimized.

@dmarteau

dmarteau Jan 13, 2017 Author Contributor

This is the idea at the end, but this is still heavily used in wfs, wcs: I plan to remove QgsMapServiceException when all services will be moved as new services modules.


int responseCode() const;

virtual QByteArray formatResponse( QString& responseFormat ) const;

This comment has been minimized.

@nyalldawson

nyalldawson Jan 13, 2017 Collaborator

responseFormat should be marked with an /Out/ annotation:

virtual QByteArray formatResponse( QString& responseFormat /Out/ ) const;

private:
QMap<QString, QString> mHeaders;
QBuffer mBuffer;
bool mFinished;
bool mHeadersSent;
bool mFinished = false;

This comment has been minimized.

@nyalldawson

nyalldawson Jan 13, 2017 Collaborator

QGIS standard is just a single space, not aligned

@elpaso
Copy link
Contributor

elpaso commented Jan 13, 2017

@dmarteau looks good to me. Just one thing you might want to check: if I remember correctly, in my original implementation the reason to keep a reference to the exception was that I wanted to be able to set exceptions from within a plugin. That idea was never implemented, but you might want to consider that now: a Python plugin filter should be able to access to the exception generated by a service and act accordingly (even by clearing the exception and generating its own alternate response).
It would also be very nice if the Python exceptions generated inside a plugin could bubble up into C++, which is not the case now.

@rldhont rldhont added the Server label Jan 13, 2017
@rldhont rldhont requested review from sbrunner, rldhont and mhugent Jan 13, 2017
@rldhont rldhont added this to the QGIS 3 milestone Jan 13, 2017
@dmarteau
Copy link
Contributor Author

dmarteau commented Jan 13, 2017

@elpaso I have removed the reference to the to the exception because there was no accesor to it: once set it was written the the output and a boolean indicating occucence of an exception was raised. Actually a plugin can still set an exception (exception objects are instanciable to python) and the exception is written to the body). From this, nothing has changed.

If you want that plugins can examine exception I would s suggest to set a new hook 'onExceptionRaised( ... )' that will pass the exception to the plugins when the exception is catched. Mainly because throwing an exception leaves the normal control flow and the place where you can deal with it it'is when you catch it.

Now about bubbling exceptions from python to C++, yes I had some thoughts about it, but I need to look further on this (dealing with exception across language can be tricky).

@dmarteau dmarteau force-pushed the dmarteau:revisit_server_exceptions branch from e04b5d4 to d260089 Jan 13, 2017
@dmarteau
Copy link
Contributor Author

dmarteau commented Jan 13, 2017

travis config problems ?

@rldhont
Copy link
Contributor

rldhont commented Jan 14, 2017

@dmarteau build restarted

@dmarteau
Copy link
Contributor Author

dmarteau commented Jan 14, 2017

@rldhont @nyalldawson Is master broken ? I can't rebase because the project is not compiling:

 /Users/david/Projets/qgis/QGIS/build-debug/src/core/qgsexpression_texts.cpp:3:52: error: no member named
    'gFunctionHelpTexts' in 'QgsExpression'
 QHash<QString, QgsExpression::Help> QgsExpression::gFunctionHelpTexts;
@rldhont
Copy link
Contributor

rldhont commented Jan 14, 2017

@dmarteau travis does not complain...

@nyalldawson
Copy link
Collaborator

nyalldawson commented Jan 14, 2017

Is master broken ? I can't rebase because the project is not compiling:

@dmarteau cleaning your build folder should fix this

@dmarteau dmarteau force-pushed the dmarteau:revisit_server_exceptions branch from d260089 to 1e6dc73 Jan 14, 2017
    - Add server exception base class
    - Enable per service exception definition
    - Handle QgsException gracefully (error 500)
    - Handle OGC exception versioning
@dmarteau dmarteau force-pushed the dmarteau:revisit_server_exceptions branch from 1e6dc73 to 8b0526d Jan 14, 2017
@dmarteau
Copy link
Contributor Author

dmarteau commented Jan 15, 2017

@nyalldawson yes that fixed it, thx.

@rldhont rldhont merged commit 474252a into qgis:master Jan 16, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@haubourg
Copy link
Contributor

haubourg commented Jan 16, 2017

Hi, this commit seems to break compiling here (ubuntu 16.04). previous commit passes. Following are also broken.
I have this:

[ 13%] Building CXX object src/core/CMakeFiles/qgis_core.dir/moc_qgsdataprovider.cpp.o
/home/regis/GITHUB_Perso/QGIS3/QGIS/build/src/core/qgsexpression_texts.cpp:3:52: error: ‘QHash<QString, QgsExpression::Help> QgsExpression::gFunctionHelpTexts’ is not a static data member of ‘class QgsExpression’
QHash<QString, QgsExpression::Help> QgsExpression::gFunctionHelpTexts;
^
/home/regis/GITHUB_Perso/QGIS3/QGIS/build/src/core/qgsexpression_texts.cpp: In static member function ‘static void QgsExpression::initFunctionHelp()’:
/home/regis/GITHUB_Perso/QGIS3/QGIS/build/src/core/qgsexpression_texts.cpp:7:8: error: ‘gFunctionHelpTexts’ was not declared in this scope
if( !gFunctionHelpTexts.isEmpty() )
^
/home/regis/GITHUB_Perso/QGIS3/QGIS/build/src/core/qgsexpression_texts.cpp:10:3: error: ‘gFunctionHelpTexts’ was not declared in this scope
gFunctionHelpTexts.insert( "$area",
^
[ 13%] Building CXX object src/core/CMakeFiles/qgis_core.dir/moc_qgsfeature.cpp.o
src/core/CMakeFiles/qgis_core.dir/build.make:9511 : la recette pour la cible « src/core/CMakeFiles/qgis_core.dir/qgsexpression_texts.cpp.o » a échouée
make[2]: *** [src/core/CMakeFiles/qgis_core.dir/qgsexpression_texts.cpp.o] Erreur 1
make[2]: *** Attente des tâches non terminées....
CMakeFiles/Makefile2:1177 : la recette pour la cible « src/core/CMakeFiles/qgis_core.dir/all » a échouée
make[1]: *** [src/core/CMakeFiles/qgis_core.dir/all] Erreur 2
Makefile:160 : la recette pour la cible « all » a échouée
make: *** [all] Erreur 2

@haubourg
Copy link
Contributor

haubourg commented Jan 16, 2017

please forget, other errors occur later in previous commits too

@rldhont
Copy link
Contributor

rldhont commented Jan 16, 2017

@haubourg as @nyalldawson has said, you have to clean your build folder.

@haubourg
Copy link
Contributor

haubourg commented Jan 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.