[Bugfix][Server] Add WMS SLD parameter support #7859

rldhont · 2018-09-11T08:32:42Z

Description

Fixed #19795 QGIS Server 3 / WMS: the SLD parameter support has been removed https://issues.qgis.org/issues/19795

To reactivate SLD parameter support, we add a new conversion capability in server parameter toUrl. And the capabilty to load the content associted to an URL.

Then if the SLD parameter is defined, the content is loaded and the SLD_BODY is set.

Checklist

Commit messages are descriptive and explain the rationale for changes
Commits which fix bugs include fixes #11111 in the commit message next to the description
I have read the QGIS Coding Standards and this PR complies with them
This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
New unit tests have been added for core changes
I have run the scripts/prepare-commit.sh script before each commit

pblottiere · 2018-09-11T09:15:33Z

Hi @rldhont,

There's currently a unit test for the SLD parameter (test_wms_getmap_sld) and the rendered image seems OK. How is it possible?

Moreover, I saw you added a SLD_BODY parameter. Can you give an example of a request for my education please?

Thanks :).

rldhont · 2018-09-11T09:21:32Z

Hi @pblottiere,

SLD_BODY is for the XML url encoded, and SLD is for an URL. The tests has to be updated to use SLD_BODY param and not SLD

pblottiere · 2018-09-11T09:40:04Z

SLD_BODY is for the XML url encoded, and SLD is for an URL. The tests has to be updated to use SLD_BODY param and not SLD

Thanks for the explanation 👍

rldhont · 2018-09-11T12:54:57Z

@pblottiere it's green!

pblottiere · 2018-09-11T13:17:39Z

src/server/qgsserverparameters.cpp

@@ -200,6 +205,107 @@ QgsRectangle QgsServerParameterDefinition::toRectangle( bool &ok ) const
  return extent;
 }

+QByteArray QgsServerParameterDefinition::loadUrl( bool &ok ) const
+{


Hi @nyalldawson,

I'm not very familiar with the QgsNetworkAccessManager class. Can you take a look on this method in particular to give me your opinion please?

Thanks :)

I'm not familiar with the design of server, but calling QCoreApplication::processEvents on the main thread is dangerous in app. I don't think that's an issue here as my understanding is that server does everything on the main thread?

There is a class QgsNetworkContentFetcher which handles the redirections etc for you, so it would be good to see that used to avoid duplicated code handling this.

I can't use QgsNetworkContentFetcher because I can't use connect because QgsServerParameterDefinition does not derived QObject.

Nyall is right, it would avoid a lot of duplicated code.

Why is it not possible to just use QgsNetworkContentFetcher in the same way as QgsLayoutItemHtml (for example in the fetchHtml method)?

Has explain, QgsServerParameterDefinition is not derived from QObject so it's not possible to use connect in this context. So I can't do like QgsLayoutItemHtml

connect( mFetcher, &QgsNetworkContentFetcher::finished, &loop, [&loaded, &loop ] { loaded = true; loop.quit(); } );

If you have an idea, how I can use QgsNetworkContentFetcher I'm interested.

Can't you call QObject:: connect?

Thanks @nyalldawson it's done.

@pblottiere done ;-)

Nice, it's much better this way. Thanks @rldhont :)

pblottiere · 2018-09-11T13:19:43Z

src/server/services/wms/qgswmsparameters.cpp

+      QByteArray ba = mWmsParameters[ QgsWmsParameter::SLD ].loadUrl();
+      if ( !ba.isEmpty() )
+      {
+        const QString sldBody = QString::fromStdString( ba.toStdString() );


@rldhont Do you think that the string conversion could be hidden in the loadUrl() method?

pblottiere · 2018-09-11T13:20:31Z

src/server/services/wms/qgswmsparameters.h

@@ -364,7 +381,7 @@ namespace QgsWms
       * Returns SLD if defined or an empty string.
       * \returns sld
       */
-      QString sld() const;
+      QString sldBody() const;


Can you update the documentation accordingly?

pblottiere · 2018-09-11T13:23:40Z

Hi @rldhont,

Apart from some minor comments, I just would like an opinion on the use of QgsNetworkAccessManager because I never used it myself (especially on the while loops).

Otherwise, the code looks good!

Thanks :)

rldhont · 2018-09-11T13:32:01Z

Thanks @pblottiere for the review

rldhont · 2018-09-12T15:24:40Z

@pblottiere done

pblottiere · 2018-09-12T18:10:50Z

src/server/qgsserverparameters.cpp

+
+  // Do the request
+  QNetworkReply *reply = nullptr;
+  // The following code blocks until the file is downloaded...


Is there some kind of timeout mechanism in this case (to avoid an infinite loop)?

Which duration ?

@pblottiere I have added a timeout mecahnism.

pblottiere · 2018-09-25T12:12:04Z

src/server/qgsserverparameters.cpp

+  QEventLoop loop;
+  QObject::connect( &fetcher, &QgsNetworkContentFetcher::finished, &loop, &QEventLoop::quit );
+
+  QgsMessageLog::logMessage( QStringLiteral( "Request started [url: %1]" ).arg( url.toString() ) );


Just wondering what happens here if the connection requires authentication ... unfortunately the current implementation of authmanager cannot automatically inject the authentication configuration based on a domain (and we probably don't want to do that without user confirmation or authentication configuration).

Not a blocker, but something that needs to be considered.

@pblottiere which class in the server is provided tr(...) ?

found ad done

pblottiere · 2018-09-25T12:12:14Z

src/server/qgsserverparameters.cpp

+  if ( !reply )
+  {
+    ok = false;
+    QgsMessageLog::logMessage( QStringLiteral( "Request failed [error: no reply - url: %1]" ).arg( url.toString() ) );


pblottiere · 2018-09-25T12:12:37Z

src/server/qgsserverparameters.cpp

+    ok = false;
+    if ( reply->error() != QNetworkReply::NoError )
+    {
+      QgsMessageLog::logMessage( QStringLiteral( "Request failed [error: %1 - url: %2]" ).arg( reply->errorString(), reply->url().toString() ) );


pblottiere · 2018-09-25T12:12:45Z

src/server/qgsserverparameters.cpp

+      QgsMessageLog::logMessage( QStringLiteral( "Request failed [error: %1 - url: %2]" ).arg( reply->errorString(), reply->url().toString() ) );
+    }
+    QVariant phrase = reply->attribute( QNetworkRequest::HttpReasonPhraseAttribute );
+    QgsMessageLog::logMessage( QStringLiteral( "Request error [status: %1 - reason phrase: %2] for %3" ).arg( status.toInt() ).arg( phrase.toString(), reply->url().toString() ) );


pblottiere · 2018-09-25T12:12:53Z

src/server/qgsserverparameters.cpp

+  if ( reply->error() != QNetworkReply::NoError )
+  {
+    ok = false;
+    QgsMessageLog::logMessage( QStringLiteral( "Request failed [error: %1 - url: %2]" ).arg( reply->errorString(), reply->url().toString() ) );


pblottiere · 2018-09-25T12:13:06Z

src/server/services/wms/qgswmsparameters.cpp

+    if ( !ok )
+    {
+      const QString msg = QString( "%1 request error for %2" ).arg( name( mName ), url.toString() );
+      QgsMessageLog::logMessage( msg );


The message is also raised and be displayed in an OGC Exceptions. You wrote in #8006 discussion that messages in OGC Exceptions does not have to be localized.

I can removed the logMessage because QgsServerParameterDefinition already logs messages.
What do you think ?

The message is also raised and be displayed in an OGC Exceptions. You wrote in #8006 discussion that messages in OGC Exceptions does not have to be localized.

Indeed.

I can removed the logMessage because QgsServerParameterDefinition already logs messages.
What do you think ?

Indeed :)

done @pblottiere thanks!

Fixed qgis#19795 QGIS Server 3 / WMS: the SLD parameter support has been removed To reactivate SLD parameter support, we add a new conversion capability in server parameter `toUrl`. And the capabilty to load the content associted to an URL. Then if the SLD parameter is defined, the content is loaded and the SLD_BODY is set.

rldhont added Requires Tests! Waiting on the submitter to add unit tests before eligible for merging Bugfix labels Sep 11, 2018

rldhont added this to the 3.4 milestone Sep 11, 2018

rldhont requested review from elpaso, sbrunner and pblottiere September 11, 2018 08:32

rldhont force-pushed the fix-server-sld-param-support branch from 2329f5d to f090730 Compare September 11, 2018 09:23

pblottiere reviewed Sep 11, 2018

View reviewed changes

rldhont changed the title ~~[Bugfix][Server[WIP]] Add WMS SLD parameter support~~ [Bugfix][Server] Add WMS SLD parameter support Sep 12, 2018

rldhont force-pushed the fix-server-sld-param-support branch from dc60cd7 to e3352eb Compare September 12, 2018 15:41

pblottiere reviewed Sep 12, 2018

View reviewed changes

rldhont removed the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Sep 13, 2018

nyalldawson modified the milestones: 3.4, 3.4.0 Sep 14, 2018

rldhont force-pushed the fix-server-sld-param-support branch 2 times, most recently from 5e9fed0 to e9d4442 Compare September 16, 2018 17:22

pblottiere reviewed Sep 25, 2018

View reviewed changes

rldhont added 4 commits September 27, 2018 21:08

[Tests][Server] Add test for WMS SLD parameter and move SLD_BODY test

34009d3

[Server] Use QgsNetworkContentFetcher to loadUrl

00cfe7d

[Server] Localize loadUrl messages in log

907c1a9

rldhont force-pushed the fix-server-sld-param-support branch from 4ec5ee9 to 907c1a9 Compare September 27, 2018 19:11

rldhont merged commit ad7d03c into qgis:master Sep 29, 2018

rldhont deleted the fix-server-sld-param-support branch February 7, 2019 21:47

qgib mentioned this pull request May 25, 2019

QGIS Server 3 / WMS: the SLD parameter support has been removed #27620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bugfix][Server] Add WMS SLD parameter support #7859

[Bugfix][Server] Add WMS SLD parameter support #7859

rldhont commented Sep 11, 2018 •

edited

Loading

pblottiere commented Sep 11, 2018

rldhont commented Sep 11, 2018

pblottiere commented Sep 11, 2018

rldhont commented Sep 11, 2018

pblottiere Sep 11, 2018

nyalldawson Sep 11, 2018

rldhont Sep 12, 2018

pblottiere Sep 18, 2018

rldhont Sep 25, 2018

nyalldawson Sep 25, 2018

rldhont Sep 25, 2018

rldhont Sep 25, 2018

pblottiere Sep 25, 2018

pblottiere Sep 11, 2018

pblottiere Sep 11, 2018

pblottiere commented Sep 11, 2018

rldhont commented Sep 11, 2018

rldhont commented Sep 12, 2018

pblottiere Sep 12, 2018

rldhont Sep 14, 2018

rldhont Sep 18, 2018

pblottiere Sep 25, 2018

elpaso Sep 25, 2018

rldhont Sep 26, 2018

rldhont Sep 26, 2018

pblottiere Sep 25, 2018

pblottiere Sep 25, 2018

pblottiere Sep 25, 2018

pblottiere Sep 25, 2018

pblottiere Sep 25, 2018

rldhont Sep 26, 2018

pblottiere Sep 27, 2018

rldhont Sep 27, 2018

[Bugfix][Server] Add WMS SLD parameter support #7859

[Bugfix][Server] Add WMS SLD parameter support #7859

Conversation

rldhont commented Sep 11, 2018 • edited Loading

Description

Checklist

pblottiere commented Sep 11, 2018

rldhont commented Sep 11, 2018

pblottiere commented Sep 11, 2018

rldhont commented Sep 11, 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

pblottiere commented Sep 11, 2018

rldhont commented Sep 11, 2018

rldhont commented Sep 12, 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

rldhont commented Sep 11, 2018 •

edited

Loading