Merge pull request #5792 from rldhont/fix-getlegendgraphic-get-method

Fix WMS GetLegendGraphic to perform request with get method with single layer

Author: rldhont

assets/src/modules/Errors.js

@@ -6,15 +6,41 @@
  * @license MPL-2.0
  */
 
+/**
+ * Representing a request error with message and parameters before fetching
+ * @class
+ * @augments Error
+ * @property {string} name       - Error name: RequestError
+ * @property {string} message    - Error message
+ * @property {object} parameters - The request parameters
+ */
+export class RequestError extends Error {
+
+    /**
+     * Creating a request error with message and parameters before fetching
+     * @param {string} message    - Error message
+     * @param {object} params     - The request parameters
+     */
+    constructor(message, params) {
+        super(message);
+        this.name = "RequestError";
+        this.parameters = params;
+    }
+}
+
 /**
  * Representing a network error with message, resource and options fetched
  * @class
  * @augments Error
+ * @property {string} name - Error name: NetworkError
+ * @property {string} message - Error message
+ * @property {string} resource - The resource has been fetched
+ * @property {string} options - The resource options
  */
-class NetworkError extends Error {
+export class NetworkError extends Error {
 
     /**
-     * Creating an HTTP error with message and status code
+     * Creating an HTTP error with message, resource and options fetched
      * @param {string} message    - Error message
      * @param {string} resource   - The resource has been fetched
      * @param {string} options    - The resource options
@@ -30,9 +56,14 @@ class NetworkError extends Error {
 /**
  * Representing an HTTP error with message, status code, resource and options fetched
  * @class
- * @augments Error
+ * @augments NetworkError
+ * @property {string} name - Error name: HttpError
+ * @property {string} message - Error message
+ * @property {string} resource - The resource has been fetched
+ * @property {string} options - The resource options
+ * @property {number} statusCode - HTTP Error status code
  */
-class HttpError extends NetworkError {
+export class HttpError extends NetworkError {
 
     /**
      * Creating an HTTP error with message and status code
@@ -42,34 +73,36 @@ class HttpError extends NetworkError {
      * @param {string} options    - The resource options
      */
     constructor(message, statusCode, resource, options) {
-        super(message);
+        super(message, resource, options);
         this.name = "HttpError";
         this.statusCode = statusCode;
-        this.resource = resource;
-        this.options = options;
     }
 }
 
 /**
- * Representing an HTTP error with message, response, resource and options fetched
+ * Representing an response error with message, response, resource and options fetched
  * @class
- * @augments Error
+ * @augments HttpError
+ * @property {string} name - Error name: ResponseError
+ * @property {string} message - Error message
+ * @property {string} resource - The resource has been fetched
+ * @property {string} options - The resource options
+ * @property {number} statusCode - HTTP Error status code
+ * @property {Response} response - Response object from fetch
  */
-class ResponseError extends NetworkError {
+export class ResponseError extends HttpError {
 
     /**
      * Creating an HTTP error with message and status code
      * @param {string}   message    - Error message
-     * @param {Response} response - HTTP Error status code
+     * @param {Response} response   - Response object from fetch
      * @param {string}   resource   - The resource has been fetched
      * @param {string}   options    - The resource options
      */
     constructor(message, response, resource, options) {
-        super(message);
+        super(message, response.status, resource, options);
         this.name = "ResponseError";
         this.response = response;
-        this.resource = resource;
-        this.options = options;
     }
 }
 
@@ -78,7 +111,7 @@ class ResponseError extends NetworkError {
  * @class
  * @augments Error
  */
-class ConversionError extends Error {
+export class ConversionError extends Error {
 
     /**
      * Creating a conversion error
@@ -96,7 +129,7 @@ class ConversionError extends Error {
  * @class
  * @augments Error
  */
-class ValidationError extends Error {
+export class ValidationError extends Error {
 
     /**
      * Creating a validation error
@@ -114,7 +147,7 @@ class ValidationError extends Error {
  * @class
  * @augments ValidationError
  */
-class PropertyRequiredError extends ValidationError {
+export class PropertyRequiredError extends ValidationError {
 
     /**
      * Creating a property required error
@@ -126,5 +159,3 @@ class PropertyRequiredError extends ValidationError {
     }
 
 }
-
-export { NetworkError, HttpError, ResponseError, ConversionError, ValidationError, PropertyRequiredError };

assets/src/modules/WMS.js

@@ -6,6 +6,7 @@
  */
 
 import { Utils } from './Utils.js';
+import { RequestError } from './Errors.js';
 
 /**
  * @class
@@ -54,15 +55,35 @@ export default class WMS {
      * Get legend graphic from WMS
      * @param {object} options - optional parameters which can override this._defaultGetLegendGraphicsParameters
      * @returns {Promise} Promise object represents data
+     * @throws {HttpError} In case of not successful response (status not in the range 200 – 299)
+     * @throws {NetworkError} In case of catch exceptions
      * @memberof WMS
      */
     async getLegendGraphic(options) {
+        const layers = options['LAYERS'] ?? options['LAYER'];
+        // Check if layer is specified
+        if (!layers) {
+            return Promise.reject(
+                new RequestError(
+                    'LAYERS or LAYER parameter is required for getLegendGraphic request',
+                    options,
+                )
+            );
+        }
+        const params = new URLSearchParams({
+            ...this._defaultGetLegendGraphicParameters,
+            ...options
+        });
+        // Check if multiple layers are specified
+        if ((Array.isArray(layers) && layers.length == 1) ||
+            (!Array.isArray(layers) && layers.split(',').length == 1)) {
+            // Use GET request for single layer
+            return Utils.fetchJSON(`${globalThis['lizUrls'].wms}?${params}`);
+        }
+        // Use POST request for multiple layers
         return Utils.fetchJSON(globalThis['lizUrls'].wms, {
             method: "POST",
-            body: new URLSearchParams({
-                ...this._defaultGetLegendGraphicParameters,
-                ...options
-            })
+            body: params,
         });
     }
 }

lizmap/modules/lizmap/controllers/service.classic.php

@@ -671,12 +671,25 @@ protected function GetMap($wmsRequest)
      */
     protected function GetLegendGraphics($wmsRequest)
     {
-        $result = $wmsRequest->process();
-
         /** @var jResponseBinary $rep */
         $rep = $this->getResponse('binary');
+        // Etag header and cache control
+        $etag = $this->buildEtag('GetLegendGraphic');
+        $respCanBeCached = $this->canBeCached();
+        if ($respCanBeCached && $rep->isValidCache(null, $etag)) {
+            $this->setACAOHeader($rep);
+
+            return $rep;
+        }
+
+        $result = $wmsRequest->process();
         $this->setupBinaryResponse($rep, $result, 'qgis_server_legend');
 
+        if ($respCanBeCached) {
+            $this->setEtagCacheHeaders($rep, $etag);
+            $this->setACAOHeader($rep);
+        }
+
         return $rep;
     }
 

tests/end2end/playwright/globals.js

@@ -14,6 +14,11 @@ import * as path from 'path';
  * @typedef {import('@playwright/test').APIResponse} APIResponse
  */
 
+/**
+ * Playwright APIRequestContext
+ * @typedef {import('@playwright/test').APIRequestContext} APIRequestContext
+ */
+
 /**
  * Integer
  * @typedef {number} int
@@ -122,7 +127,7 @@ export async function gotoMap(url, page, mapMustLoad = true, layersInTreeView =
 
     await expect(async () => {
         const response = await page.goto(url);
-        expect(response.status()).toBeLessThan(400);
+        expect(response?.status()).toBeLessThan(400);
     }).toPass({
         intervals: [1_000, 2_000, 10_000],
         timeout: 60_000
@@ -134,9 +139,14 @@ export async function gotoMap(url, page, mapMustLoad = true, layersInTreeView =
         if (waitForGetLegendGraphic) {
             // Wait for WMS GetLegendGraphic promise
             const getLegendGraphicPromise = page.waitForRequest(
-                request => request.method() === 'POST' &&
+                request => (
+                    request.method() === 'POST' &&
                     request.postData() != null &&
                     request.postData()?.includes('GetLegendGraphic') === true
+                ) || (
+                    request.method() === 'GET' &&
+                    request.url().includes('GetLegendGraphic') === true
+                )
             );
             // Normal check about the map
             // Wait for WMS GetLegendGraphic
@@ -167,7 +177,7 @@ export async function reloadMap(page, check = true) {
 
     await expect(async () => {
         const response = await page.reload();
-        expect(response.status()).toBeLessThan(400);
+        expect(response?.status()).toBeLessThan(400);
     }).toPass({
         intervals: [1_000, 2_000, 10_000],
         timeout: 60_000
@@ -178,10 +188,14 @@ export async function reloadMap(page, check = true) {
     if (check) {
         // Wait for WMS GetLegendGraphic promise
         const getLegendGraphicPromise = page.waitForRequest(
-            request =>
+            request => (
                 request.method() === 'POST' &&
                 request.postData() != null &&
                 request.postData()?.includes('GetLegendGraphic') === true
+            ) || (
+                request.method() === 'GET' &&
+                request.url().includes('GetLegendGraphic') === true
+            )
         );
         // Normal check about the map
         // Wait for WMS GetLegendGraphic
@@ -245,7 +259,7 @@ export async function expectToHaveLengthCompare(title, parameters, expectedLengt
 
 /**
  * Get the JSON for the given project using the API
- * @param {import("playwright-core/types/types.js").APIRequestContext} request Request to use
+ * @param {APIRequestContext} request Request to use
  * @param {string} project The project name
  * @param {string} repository The repository name, default to "testsrepository".
  * @returns {Promise<any>} The JSON response
@@ -259,9 +273,9 @@ export async function jsonFromProjectApi(request, project, repository = 'testsre
 
 /**
  * Get the version of QGIS written in the project
- * @param {import("playwright-core/types/types.js").APIRequestContext} request Request to use
+ * @param {APIRequestContext} request Request to use
  * @param {string} project The project name
- * @returns {int} The QGIS version, written as "34004" for QGIS 3.40.4, to be easily sortable.
+ * @returns {Promise<int>} The QGIS version, written as "34004" for QGIS 3.40.4, to be easily sortable.
  */
 export async function qgisVersionFromProjectApi(request, project) {
     const response = await jsonFromProjectApi(request, project);
@@ -284,12 +298,11 @@ export async function checkJson(response, status = 200) {
     return await response.json();
 }
 
-/* eslint-disable jsdoc/check-types */
 /**
  * Check parameters against an object containing expected parameters
  * @param {string}                        title Check title, for testing and debug
  * @param {string}                        parameters List of parameters to check
- * @param {Object<string, string|RegExp>} expectedParameters List of expected parameters
+ * @param {{[key: string]: string|RegExp}} expectedParameters List of expected parameters
  * @returns {Promise<URLSearchParams>}    List of promise with parameters
  */
 export async function expectParametersToContain(title, parameters, expectedParameters) {
@@ -332,9 +345,9 @@ const adminPassword = "Basic " + btoa("admin:admin");
 
 /**
  * Create a GET request on a given URL with Basic authentication admin:admin
- * @param {import("playwright-core/types/types.js").APIRequestContext} request Request to use
+ * @param {APIRequestContext} request Request to use
  * @param {string} url URL to do a GET request on
- * @returns {Promise<import("playwright-core/types/types.js").APIResponse>} Response
+ * @returns {Promise<APIResponse>} Response
  */
 export async function requestGETWithAdminBasicAuth(request, url) {
     return await request.get(url,
@@ -347,10 +360,10 @@ export async function requestGETWithAdminBasicAuth(request, url) {
 
 /**
  * Create a POST request on a given URL with Basic authentication admin:admin
- * @param {import("playwright-core/types/types.js").APIRequestContext} request Request to use
+ * @param {APIRequestContext} request Request to use
  * @param {string} url URL to do a POST request on
  * @param {object} data parameters for the request
- * @returns {Promise<import("playwright-core/types/types.js").APIResponse>} Response
+ * @returns {Promise<APIResponse>} Response
  */
 export async function requestPOSTWithAdminBasicAuth(request, url, data) {
     return await request.post(url,
@@ -364,10 +377,10 @@ export async function requestPOSTWithAdminBasicAuth(request, url, data) {
 
 /**
  * Create a DELETE request on a given URL with Basic authentication admin:admin
- * @param {import("playwright-core/types/types.js").APIRequestContext} request Request to use
+ * @param {APIRequestContext} request Request to use
  * @param {string} url URL to do a DELETE request on
  * @param {object} data parameters for the request
- * @returns {Promise<import("playwright-core/types/types.js").APIResponse>} Response
+ * @returns {Promise<APIResponse>} Response
  */
 export async function requestDELETEWithAdminBasicAuth(request, url, data) {
     return await request.delete(url,
@@ -378,4 +391,3 @@ export async function requestDELETEWithAdminBasicAuth(request, url, data) {
             data: data
         });
 }
-/* eslint-enable jsdoc/check-types */