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

Implements service modules registry - Experimental #3866

Merged
merged 35 commits into from Jan 10, 2017

Conversation

@dmarteau
Contributor

dmarteau commented Dec 13, 2016

Implementation for qgis/QGIS-Enhancement-Proposals#74

This is an experimental work in progress.

The interfaces need refinements and a native (C++) exemple implementation of module is missing.
I have issue with dynamic modules on mac: after make 'install' the runtime path/install name of qgis_server lib is set to the build location and not the install location, so module that link dynamically are also linked with libs in the build location not install location.
Help is welcome for setting the build process of dynamic modules.

What is done so far:

  • Handling of services as dynamic modules (native and python)
  • Découpling of the fcgi supports from the server codebase.
  • WMS supports is moved to new service provider system.

@rldhont rldhont added this to the QGIS 3 milestone Dec 13, 2016

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Dec 13, 2016

Contributor

@pblottiere this is the PR for QGIS Server modularity

Contributor

rldhont commented Dec 13, 2016

@pblottiere this is the PR for QGIS Server modularity

@nyalldawson

@dmarteau I'm not involved in server so can't comment on the design, so this feedback is just general QGIS conventions/qt usage advice.

Great work here!

Show outdated Hide outdated python/server/qgsserverrequest.sip Outdated
Show outdated Hide outdated python/server/qgsserverrequest.sip Outdated
Show outdated Hide outdated python/server/qgsserverrequest.sip Outdated
Show outdated Hide outdated python/server/qgsserverrequest.sip Outdated
Show outdated Hide outdated python/server/qgsserverrequest.sip Outdated
Show outdated Hide outdated src/server/qgsserviceloader.h Outdated
Show outdated Hide outdated src/server/qgsserviceregistry.cpp Outdated
Show outdated Hide outdated src/server/qgsserviceregistry.cpp Outdated
Show outdated Hide outdated src/server/qgsserviceregistry.cpp Outdated
Show outdated Hide outdated src/server/qgsserviceregistry.h Outdated
@wonder-sk

Nice work - well done - looking for more :-)

Do I understand correctly that the request/response classes will have some implementations provided in the server (e.g. impl based on QNetworkRequest/QNetworkReply and impl working with just memory buffers) ?

Show outdated Hide outdated python/server/qgsserverrequest.sip Outdated
Show outdated Hide outdated python/server/qgsserverresponse.sip Outdated
Show outdated Hide outdated python/server/qgsserverresponse.sip Outdated
Show outdated Hide outdated python/server/qgsserverresponse.sip Outdated
Show outdated Hide outdated python/server/qgsserviceregistry.sip Outdated
Show outdated Hide outdated python/server/qgsserviceregistry.sip Outdated
Show outdated Hide outdated python/server/qgsserviceregistry.sip Outdated
Show outdated Hide outdated python/server/qgsserviceregistry.sip Outdated
Show outdated Hide outdated src/server/qgsserviceregistry.cpp Outdated
Show outdated Hide outdated python/server/qgsserverrequest.sip Outdated
@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Dec 14, 2016

Contributor

@wonder-sk This is exactly that. Thanks.

Contributor

dmarteau commented Dec 14, 2016

@wonder-sk This is exactly that. Thanks.

Show outdated Hide outdated python/server/qgsservice.sip Outdated
Show outdated Hide outdated python/server/qgsserverresponse.sip Outdated
Show outdated Hide outdated src/server/qgsserverrequest.h Outdated
Show outdated Hide outdated src/server/qgsserverrequest.h Outdated
Show outdated Hide outdated src/server/qgsserverresponse.h Outdated
Show outdated Hide outdated src/server/qgsserverresponse.h Outdated

@rldhont rldhont merged commit 9be984c into qgis:master Jan 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 10, 2017

Contributor

Merge done

Contributor

rldhont commented Jan 10, 2017

Merge done

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Jan 10, 2017

Contributor

Brilliant work David!

Contributor

nyalldawson commented Jan 10, 2017

Brilliant work David!

@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Jan 10, 2017

Contributor

@nyalldawson thanks.

Contributor

dmarteau commented Jan 10, 2017

@nyalldawson thanks.

@sbrunner

This comment has been minimized.

Show comment
Hide comment
@sbrunner

sbrunner Jan 11, 2017

Contributor

Many thanks for your great job :-)

Contributor

sbrunner commented Jan 11, 2017

Many thanks for your great job :-)

@elpaso

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Jan 11, 2017

Contributor

Great job @dmarteau, thanks a lot!

Contributor

elpaso commented Jan 11, 2017

Great job @dmarteau, thanks a lot!

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jan 11, 2017

Member

Yay, good job 👍 ! 3.0 will be THE release!

Member

m-kuhn commented Jan 11, 2017

Yay, good job 👍 ! 3.0 will be THE release!

#ifdef HAVE_SERVER_PYTHON_PLUGINS
// XXX Why this is enabled only fol plugins ?

This comment has been minimized.

@elpaso

elpaso Jan 12, 2017

Contributor

because when Python plugins are disabled there is no sServerInterface

@elpaso

elpaso Jan 12, 2017

Contributor

because when Python plugins are disabled there is no sServerInterface

@dmarteau dmarteau referenced this pull request Jan 16, 2017

Merged

WFS & WCS as module #4004

@elpaso

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Apr 20, 2017

Contributor

@dmarteau I just noticed that this is gone: we absolutely need it back in some way, the possibility to alter the response body from the plugins is a core feature.

Contributor

elpaso commented on src/server/qgsrequesthandler.h in 64fc56c Apr 20, 2017

@dmarteau I just noticed that this is gone: we absolutely need it back in some way, the possibility to alter the response body from the plugins is a core feature.

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Apr 20, 2017

Contributor

I understand that there is now a QgsServerResponse object, you should probably let the plugin access that instance directly in r/w mode.

Contributor

elpaso replied Apr 20, 2017

I understand that there is now a QgsServerResponse object, you should probably let the plugin access that instance directly in r/w mode.

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Apr 20, 2017

Contributor
Contributor

dmarteau replied Apr 20, 2017

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Apr 20, 2017

Contributor

What you have changed is:
- virtual QByteArray body() { return mBody; }
you removed the possibility to get the response body (previously it was just a member of the request), which breaks most plugins.

Before you removed it, it was possible to alter (get/set) the response body by replacing it completely (which is still possible with appendBody) or editing it, that is one of the main reasons why the plugin system was created in the first place, see for example the reference example plugin filter:
https://github.com/elpaso/qgis-helloserver/blob/master/filters/WatermarkFilter.py#L44

I see that there is now the QgsServerResponse object that we agreed upon, so, to restore the original behavior you must create a getter for the response, or, much better a convenience method to get the body as a bytearray from the buffer wrapped by the response class.

To keep API compatibility, we need:
QByteArray body() that returns the mResponse.io().buffer().data() (or something like that).

Please don't forget the bindings and I'll eventually add some tests.

Contributor

elpaso replied Apr 20, 2017

What you have changed is:
- virtual QByteArray body() { return mBody; }
you removed the possibility to get the response body (previously it was just a member of the request), which breaks most plugins.

Before you removed it, it was possible to alter (get/set) the response body by replacing it completely (which is still possible with appendBody) or editing it, that is one of the main reasons why the plugin system was created in the first place, see for example the reference example plugin filter:
https://github.com/elpaso/qgis-helloserver/blob/master/filters/WatermarkFilter.py#L44

I see that there is now the QgsServerResponse object that we agreed upon, so, to restore the original behavior you must create a getter for the response, or, much better a convenience method to get the body as a bytearray from the buffer wrapped by the response class.

To keep API compatibility, we need:
QByteArray body() that returns the mResponse.io().buffer().data() (or something like that).

Please don't forget the bindings and I'll eventually add some tests.

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Apr 20, 2017

Contributor

Ho, I see.
I don't remember exactly why this method has beeen removed (maybe no good reason). But no problem to get it back.

Contributor

dmarteau replied Apr 20, 2017

Ho, I see.
I don't remember exactly why this method has beeen removed (maybe no good reason). But no problem to get it back.

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Apr 21, 2017

Contributor

To be clear, it is natural that plugins can access data. I want to say that In case of streaming, you have to retrieve the data at each sendResponse call, before beeing sent to the client.
I'm preparing a new PR.

Contributor

dmarteau replied Apr 21, 2017

To be clear, it is natural that plugins can access data. I want to say that In case of streaming, you have to retrieve the data at each sendResponse call, before beeing sent to the client.
I'm preparing a new PR.

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Apr 21, 2017

Contributor

@dmarteau sure, that is exactly the reason why I implemented sendResponse in the first place, from the API docs:

 virtual void QgsServerFilter::sendResponse 	( ) 	
	virtual

Method called when the QgsRequestHandler sends its data to FCGI stdout. This normally occours at the end of core services processing just after the responseComplete() plugin hook. For streaming services (like WFS on getFeature requests, sendResponse() might have been called several times before the response is complete: in this particular case, sendResponse() is called once for each feature before hitting responseComplete()

I now that is far from ideal, but this is really the only way it can work with streaming services, where you might want to be able to modify the chunks before they are sent over the wire.

BTW are you available on IRC/Gitter or any other chat system?

Contributor

elpaso replied Apr 21, 2017

@dmarteau sure, that is exactly the reason why I implemented sendResponse in the first place, from the API docs:

 virtual void QgsServerFilter::sendResponse 	( ) 	
	virtual

Method called when the QgsRequestHandler sends its data to FCGI stdout. This normally occours at the end of core services processing just after the responseComplete() plugin hook. For streaming services (like WFS on getFeature requests, sendResponse() might have been called several times before the response is complete: in this particular case, sendResponse() is called once for each feature before hitting responseComplete()

I now that is far from ideal, but this is really the only way it can work with streaming services, where you might want to be able to modify the chunks before they are sent over the wire.

BTW are you available on IRC/Gitter or any other chat system?

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Apr 21, 2017

Contributor

IRC, on #qgis ?

Contributor

dmarteau replied Apr 21, 2017

IRC, on #qgis ?

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Apr 21, 2017

Contributor

yes, on freenode (I'm elpaso)

Contributor

elpaso replied Apr 21, 2017

yes, on freenode (I'm elpaso)

This comment has been minimized.

Show comment
Hide comment
@dmarteau
Contributor

dmarteau replied Apr 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment