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] WIP clean project parsing by using QgsProject #3942

Merged
merged 9 commits into from Jan 30, 2017

Conversation

Projects
None yet
8 participants
@pblottiere
Member

pblottiere commented Jan 3, 2017

The final aim of this PR is to remove project parsing done in server by using the QgsProject class instead.

A QgsServerProjectUtils is introduced to retrieve specific entries from a project (using the same method than QgsLayerTreeUtils or QgsSymbolLayerUtils).

Tests are added at each stage to check the current behavior.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 3, 2017

Contributor

@pblottiere do you think your work has to be merge before @dmarteau one ? #3866

Contributor

rldhont commented Jan 3, 2017

@pblottiere do you think your work has to be merge before @dmarteau one ? #3866

@rldhont rldhont added this to the QGIS 3 milestone Jan 3, 2017

@rldhont rldhont added the For Review label Jan 3, 2017

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 3, 2017

Member

By doing tests around the getCapabilities request, I wondered:

  • for now, in case of WCS and WFS with no service url defined in the project, the WMS service url is used instead. Is it correct to do that?
  • in the WMS server, the SchemaLocation entry indicates the WMS service url defined in the project, but not in WFS nor in WCS. Again, is it the correct behaviour?
Member

pblottiere commented Jan 3, 2017

By doing tests around the getCapabilities request, I wondered:

  • for now, in case of WCS and WFS with no service url defined in the project, the WMS service url is used instead. Is it correct to do that?
  • in the WMS server, the SchemaLocation entry indicates the WMS service url defined in the project, but not in WFS nor in WCS. Again, is it the correct behaviour?
@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 3, 2017

Member

@rldhont

do you think your work has to be merge before @dmarteau one ? #3866

As we said with @dmarteau in #3866, it doesn't seem necessary.

Member

pblottiere commented Jan 3, 2017

@rldhont

do you think your work has to be merge before @dmarteau one ? #3866

As we said with @dmarteau in #3866, it doesn't seem necessary.

@rldhont rldhont requested review from elpaso, mhugent, sbrunner, rldhont and mhugo Jan 4, 2017

@elpaso

elpaso approved these changes Jan 5, 2017

Good job! Thanks!

Show outdated Hide outdated python/server/qgsserverprojectutils.sip
Show outdated Hide outdated src/server/qgsserver.h
}
return url;
}
void QgsServerProjectParser::combineExtentAndCrsOfGroupChildren( QDomElement& groupElem, QDomDocument& doc, bool considerMapExtent ) const

This comment has been minimized.

@elpaso

elpaso Jan 5, 2017

Contributor

A general consideration here: I think it's ok to get the service URLs from the project, but they are not mandatory (and should remain optional), just make sure that the server can build and provide sensible default URLs for at least the core services.
@dmarteau maybe we should try to address this in the 3.0 refactoring: every service should be able to build its own URL (with a sensible default method in the base class).

@elpaso

elpaso Jan 5, 2017

Contributor

A general consideration here: I think it's ok to get the service URLs from the project, but they are not mandatory (and should remain optional), just make sure that the server can build and provide sensible default URLs for at least the core services.
@dmarteau maybe we should try to address this in the 3.0 refactoring: every service should be able to build its own URL (with a sensible default method in the base class).

This comment has been minimized.

@dmarteau

dmarteau Jan 19, 2017

Contributor

Actually the build of service URL is done on a per-service basis from the requests url (no more getenv access). There is no exposed service url atm in QgsService interface but that can be easily added.
See https://github.com/qgis/QGIS/blob/master/src/server/services/wms/qgswmsutils.h#L56. This function is basically implemented for each services.

@dmarteau

dmarteau Jan 19, 2017

Contributor

Actually the build of service URL is done on a per-service basis from the requests url (no more getenv access). There is no exposed service url atm in QgsService interface but that can be easily added.
See https://github.com/qgis/QGIS/blob/master/src/server/services/wms/qgswmsutils.h#L56. This function is basically implemented for each services.

@m-kuhn m-kuhn added the Server label Jan 8, 2017

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 10, 2017

Contributor

@pblottiere #3866 has been merged, so you have to update this PR.

Contributor

rldhont commented Jan 10, 2017

@pblottiere #3866 has been merged, so you have to update this PR.

@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Jan 10, 2017

Contributor

@pblottiere Just for your information: there is an extra QgsProject* parameter in the QgsService::executeRequest method, this parameter is not used atm but is the result of a discussion with mhugo for passing the project to the services when (wfs|wms|...)parsers will be removed.

Contributor

dmarteau commented Jan 10, 2017

@pblottiere Just for your information: there is an extra QgsProject* parameter in the QgsService::executeRequest method, this parameter is not used atm but is the result of a discussion with mhugo for passing the project to the services when (wfs|wms|...)parsers will be removed.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 18, 2017

Contributor

@pblottiere #4004 has been merged, so you have to update this PR.

Contributor

rldhont commented Jan 18, 2017

@pblottiere #4004 has been merged, so you have to update this PR.

@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Jan 19, 2017

Contributor

@pblottiere The heavy part of service refactoring as modules is over. This should culminate with this PR !

Contributor

dmarteau commented Jan 19, 2017

@pblottiere The heavy part of service refactoring as modules is over. This should culminate with this PR !

@rldhont

Rebase needed

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 20, 2017

Member

@rldhont @dmarteau

Actually I wanted to do more about cleaning the project parser but I was stuck with some QgsProject singleton in core.

So I have to dig in to properly rebase (because some singleton has been removed since my last commits) and improve this PR.

In other words, this PR is not ready to merge for now.

Member

pblottiere commented Jan 20, 2017

@rldhont @dmarteau

Actually I wanted to do more about cleaning the project parser but I was stuck with some QgsProject singleton in core.

So I have to dig in to properly rebase (because some singleton has been removed since my last commits) and improve this PR.

In other words, this PR is not ready to merge for now.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 25, 2017

Contributor

@pblottiere what is your plan for this PR ? We can help if needed

Contributor

rldhont commented Jan 25, 2017

@pblottiere what is your plan for this PR ? We can help if needed

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 25, 2017

Member

@rldhont I was on another project since the beginning of the year but from today I have more time to work on Qgis and especially this PR.

I let you know in the days to follow.

Thank you for your concern.

Member

pblottiere commented Jan 25, 2017

@rldhont I was on another project since the beginning of the year but from today I have more time to work on Qgis and especially this PR.

I let you know in the days to follow.

Thank you for your concern.

@wonder-sk

This comment has been minimized.

Show comment
Hide comment
@wonder-sk

wonder-sk Jan 26, 2017

Member

Great work - looking forward to having it merged!

Few thoughts for this PR:

  • QgsServerProjectUtils may be useful in qgis_core library - in QGIS desktop that may be useful for the server configuration GUIs
  • QgsServerProjectUtils could take QgsProject in constructor, so that individual member functions do not need to ask for QgsProject
  • for QgsMapLayer reading/writing I thought it may be useful to have something like QgsPathResolver object (instead of directly using QgsProject), the resolver would have just readPath/writePath methods. The idea is that map layer is not only read within the context of a project - it can be also read/written from/to QML files where we presumably want to resolve relative paths to the QML file's path rather than the current project... (not saying it has to be done within this PR, I can try to sort it out when this PR is merged)
Member

wonder-sk commented Jan 26, 2017

Great work - looking forward to having it merged!

Few thoughts for this PR:

  • QgsServerProjectUtils may be useful in qgis_core library - in QGIS desktop that may be useful for the server configuration GUIs
  • QgsServerProjectUtils could take QgsProject in constructor, so that individual member functions do not need to ask for QgsProject
  • for QgsMapLayer reading/writing I thought it may be useful to have something like QgsPathResolver object (instead of directly using QgsProject), the resolver would have just readPath/writePath methods. The idea is that map layer is not only read within the context of a project - it can be also read/written from/to QML files where we presumably want to resolve relative paths to the QML file's path rather than the current project... (not saying it has to be done within this PR, I can try to sort it out when this PR is merged)
@dmarteau

Wouldn't that be appropriate to move wmsServiceUrl local to the service ? Is there a specifiic reason to get methods globally from QgsServerProjectUtils
(same for wfs / wcs) ?

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 27, 2017

Member

@dmarteau

From my point of view, the aim of QgsServerProjectUtils is to contains everything concerning the project parsing.

Member

pblottiere commented Jan 27, 2017

@dmarteau

From my point of view, the aim of QgsServerProjectUtils is to contains everything concerning the project parsing.

@rldhont

I think the @wonder-sk proposals about QgsPathResolver will be a great enhancement in QGIS environment.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 27, 2017

Member

@rldhont Please, do not merge this PR yet. I would like to check some things first.

Member

pblottiere commented Jan 27, 2017

@rldhont Please, do not merge this PR yet. I would like to check some things first.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 27, 2017

Contributor

@pblottiere I'm waiting for @dmarteau and your validation before merging

Contributor

rldhont commented Jan 27, 2017

@pblottiere I'm waiting for @dmarteau and your validation before merging

@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Jan 27, 2017

Contributor

@wonder-sk I'm not even sure that QgsServerProjectUtils should be a class but only a collection of function in a namespace: I don't think we want removing parser and introduce a new kind of wrapper class around qgisproject. IMHO the functional approach makes more sens than OO in this case.

Contributor

dmarteau commented Jan 27, 2017

@wonder-sk I'm not even sure that QgsServerProjectUtils should be a class but only a collection of function in a namespace: I don't think we want removing parser and introduce a new kind of wrapper class around qgisproject. IMHO the functional approach makes more sens than OO in this case.

@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Jan 27, 2017

Contributor

@pblottiere I understand: but this is is specifically related to services ?

Contributor

dmarteau commented Jan 27, 2017

@pblottiere I understand: but this is is specifically related to services ?

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 27, 2017

Member

@dmarteau

I'm not even sure that QgsServerProjectUtils should be a class but only a collection of function in a namespace: I don't think we want removing parser and introduce a new kind of wrapper class around qgisproject. IMHO the functional approach makes more sens than OO in this case.

Indeed, why not.

I understand: but this is is specifically related to services ?

@wonder-sk was talking about moving it in core. But I don't have a lot of visibility on this point.

Member

pblottiere commented Jan 27, 2017

@dmarteau

I'm not even sure that QgsServerProjectUtils should be a class but only a collection of function in a namespace: I don't think we want removing parser and introduce a new kind of wrapper class around qgisproject. IMHO the functional approach makes more sens than OO in this case.

Indeed, why not.

I understand: but this is is specifically related to services ?

@wonder-sk was talking about moving it in core. But I don't have a lot of visibility on this point.

@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Jan 27, 2017

Contributor

@pblottiere please remove src/server/qgswmsserver.[cpp|h] and all others qgswfsserver/qgswcsserver: these files have been removed since #4019.

Contributor

dmarteau commented Jan 27, 2017

@pblottiere please remove src/server/qgswmsserver.[cpp|h] and all others qgswfsserver/qgswcsserver: these files have been removed since #4019.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Jan 27, 2017

Contributor

@pblottiere I can merge when you want

Contributor

rldhont commented Jan 27, 2017

@pblottiere I can merge when you want

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 27, 2017

Member

@rldhont I'm moving the QgsServerProjectUtils as a namespace like @dmarteau advise me.

Then, it should be ok.

Member

pblottiere commented Jan 27, 2017

@rldhont I'm moving the QgsServerProjectUtils as a namespace like @dmarteau advise me.

Then, it should be ok.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 30, 2017

Member

@dmarteau I replaced the QgsServerProjectUtils class by a namespace. However, the build is failing on travis, due to the SERVER_EXPORT (but everything is compiling from my side, maybe a different version of gcc).

And if I don't use the SERVER_EXPORT, functions within the namespace are not visible outside the qgis_server shared library, and so can't be used in wms/wcs/wfs modules nor in Python binding.

Do you have an idea? Or do we keep the class to avoid this?

Member

pblottiere commented Jan 30, 2017

@dmarteau I replaced the QgsServerProjectUtils class by a namespace. However, the build is failing on travis, due to the SERVER_EXPORT (but everything is compiling from my side, maybe a different version of gcc).

And if I don't use the SERVER_EXPORT, functions within the namespace are not visible outside the qgis_server shared library, and so can't be used in wms/wcs/wfs modules nor in Python binding.

Do you have an idea? Or do we keep the class to avoid this?

@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Jan 30, 2017

Contributor

@pblottiere you do not export a namespace: use SERVER_EXPORT on function declaration.

Contributor

dmarteau commented Jan 30, 2017

@pblottiere you do not export a namespace: use SERVER_EXPORT on function declaration.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 30, 2017

Member

@dmarteau Thank you for your advice! Still plenty of things to learn...

Member

pblottiere commented Jan 30, 2017

@dmarteau Thank you for your advice! Still plenty of things to learn...

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jan 30, 2017

Member

@rldhont This PR is ready to merge. Sorry for the delay.

Member

pblottiere commented Jan 30, 2017

@rldhont This PR is ready to merge. Sorry for the delay.

@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Jan 30, 2017

Contributor

@pblottiere you are welcome :-) And thank you for your work.

Contributor

dmarteau commented Jan 30, 2017

@pblottiere you are welcome :-) And thank you for your work.

@rldhont rldhont merged commit 50319b1 into qgis:master Jan 30, 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 30, 2017

Contributor

thanks @pblottiere

Contributor

rldhont commented Jan 30, 2017

thanks @pblottiere

@sbrunner

This comment has been minimized.

Show comment
Hide comment
@sbrunner

sbrunner Jan 30, 2017

Contributor

Many thanks @pblottiere :-)

Contributor

sbrunner commented Jan 30, 2017

Many thanks @pblottiere :-)

@@ -32,7 +32,7 @@ class SampleService: public QgsService
}
void executeRequest( const QgsServerRequest& request, QgsServerResponse& response,
QgsProject* project )
const QgsProject* project )

This comment has been minimized.

@vmora

vmora Mar 30, 2017

Contributor

What is the meaning of a null ptr there ? If none, why not a const & rather than a const (naked) ptr ?

@vmora

vmora Mar 30, 2017

Contributor

What is the meaning of a null ptr there ? If none, why not a const & rather than a const (naked) ptr ?

const QgsServerRequest& request, QgsServerResponse& response );
void writeGetCapabilities( QgsServerInterface* serverIface, const QgsProject* project,
const QString& version, const QgsServerRequest& request,
QgsServerResponse& response );

This comment has been minimized.

@vmora

vmora Mar 30, 2017

Contributor
  • .vs. & -> to me ptr (smart to indicate who is resp. for the resource) indicate the possibility for null, I'd use & everywhere else. Is the function able to do something meaningfull without the serverIface... maybe, without a project... maybe. If its the case, doc, if it's not &
@vmora

vmora Mar 30, 2017

Contributor
  • .vs. & -> to me ptr (smart to indicate who is resp. for the resource) indicate the possibility for null, I'd use & everywhere else. Is the function able to do something meaningfull without the serverIface... maybe, without a project... maybe. If its the case, doc, if it's not &
@dmarteau

This comment has been minimized.

Show comment
Hide comment
@dmarteau

dmarteau Mar 30, 2017

Contributor

@vmora: because of history: when this function was added in #3866. QgsProject was still a singleton and thus this parameters was only there to make room for passing QgsProject instance when it will be ready and using reference was not an option then.

Contributor

dmarteau commented Mar 30, 2017

@vmora: because of history: when this function was added in #3866. QgsProject was still a singleton and thus this parameters was only there to make room for passing QgsProject instance when it will be ready and using reference was not an option then.

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