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

[Fix][Server] WMS Dimension identifying range #43417

Closed
wants to merge 10 commits into from

Conversation

rldhont
Copy link
Contributor

@rldhont rldhont commented May 27, 2021

Description

For WMS Dimension, range values are separated by / in request for example 0/1, but values can also contain / like a french date. An other character is reserved by WMS Dimension, the , is used to separate multiple values in request.

The QGIS Server WMS code was identifying a range if the request dimension provided values contain /, the fix identifying a range if the dimension provided values contain only 1 /.

Funded by Ifremer

@rldhont rldhont added Needs Backporting Bug Either a bug report, or a bug fix. Let's hope for the latter! Server Related to QGIS server labels May 27, 2021
@rldhont rldhont requested review from elpaso and pblottiere May 27, 2021 07:13
@github-actions github-actions bot added this to the 3.20.0 milestone May 27, 2021
@rldhont
Copy link
Contributor Author

rldhont commented May 27, 2021

Hi @nyalldawson, @m-kuhn, @elpaso, @pblottiere, I'd like to see this fix in next release-3_16 without the mounth of quarantine. Can I set the label backport release-3_16 ?

@elpaso
Copy link
Contributor

elpaso commented May 27, 2021

No tests?

@pblottiere
Copy link
Member

Hello @rldhont,

+1 to add some tests. I suppose that some unit tests in tests/src/server/wms would be enough in this case.

@nyalldawson nyalldawson added the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label May 27, 2021
@pblottiere
Copy link
Member

+1 to add some tests. I suppose that some unit tests in tests/src/server/wms would be enough in this case.

@rldhont if you don't have time I can add some unit tests if you want. Let me know if I can help to move forward.

@rldhont
Copy link
Contributor Author

rldhont commented Jun 10, 2021

@pblottiere helps are welcome

@m-kuhn
Copy link
Member

m-kuhn commented Jun 10, 2021

I had a look at it before but was unsure about what problem it solves (only the how, not the what was in the pr message). If that's clear to others, good for me. Otherwise a short issue report would be great.

@nyalldawson nyalldawson modified the milestones: 3.20.0, 3.22 (Feature) Jun 20, 2021
@github-actions
Copy link

github-actions bot commented Jul 5, 2021

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 5, 2021
@rldhont rldhont force-pushed the fix-server-wms-dimension-range branch 3 times, most recently from a54b275 to 23b683f Compare July 8, 2021 07:11
@rldhont rldhont added backport queued_ltr_backports Queued Backports backport release-3_20 and removed Needs Backporting Requires Tests! Waiting on the submitter to add unit tests before eligible for merging stale Uh oh! Seems this work is abandoned, and the PR is about to close. labels Jul 8, 2021
@rldhont
Copy link
Contributor Author

rldhont commented Jul 8, 2021

I have added new tests around time, date and dimension with / in values. I have also updated the PR description.

@m-kuhn
Copy link
Member

m-kuhn commented Jul 8, 2021

Thanks for the update.
Is there no such thing as escaping for / and other reserved characters? Going by the count doesn't seem to be very stable?

@m-kuhn
Copy link
Member

m-kuhn commented Jul 8, 2021

See also

https://www.mapserver.org/ogc/wms_dimension.html

multiple range values: for example: …&elevation=480/490,490/500&…

@rldhont
Copy link
Contributor Author

rldhont commented Jul 8, 2021

@m-kuhn in the OGC WMS 1.1.0 specification

In either case, value uses the format described in Table C.1 to provide a single value, a comma-separated list, or an interval of the form start/end without a resolution. valueshall not contain whitespace. An interval in a request value is a request for all the data from the start value up to and including the end value.

In QGIS server, dimension values are firstly splitted by , then it checked that splitted string contains /. Count is not very stable, but the user can choose a field with values that contains / or ,, and I am not sure we can avoid this field from list in user interface.

@m-kuhn
Copy link
Member

m-kuhn commented Jul 8, 2021

Is there no such thing as escaping or encoding? I.e. specify 27\/12\/2021 or 27%2F12%2F2021? I think that's how this kind of requirements are normally handled.

@rldhont
Copy link
Contributor Author

rldhont commented Jul 8, 2021

@m-kuhn I have found a way to escape / and , in values. The solution is to use the Numeric character reference of \ and , in values and to percent escaping it in url request.

So, we have:

  • \ becomes / in GetCapabilities document and %26%2347; in URL
  • , becames , in GetCapabilities document and %26%2344; in URL

@rldhont rldhont force-pushed the fix-server-wms-dimension-range branch from 91e03e4 to 2cae223 Compare July 8, 2021 12:29
@m-kuhn
Copy link
Member

m-kuhn commented Jul 8, 2021

@m-kuhn I have found a way to escape / and , in values. The solution is to use the Numeric character reference of \ and , in values and to percent escaping it in url request.

So, we have:

  • \ becomes / in GetCapabilities document and %26%2347; in URL
  • , becames , in GetCapabilities document and %26%2344; in URL

If the GetCapabilities and the URL look different, it seems that QGIS Server is already html decoding the string. This leads me to think that this is something that should be fixed on client side rather than server side.

I.e. if you use a default QGIS 3.16 server and send a number (or otherwise) encoded url ... does it possibly just work?

@rldhont
Copy link
Contributor Author

rldhont commented Jul 8, 2021

If the GetCapabilities and the URL look different, it seems that QGIS Server is already html decoding the string. This leads me to think that this is something that should be fixed on client side rather than server side.

I.e. if you use a default QGIS 3.16 server and send a number (or otherwise) encoded url ... does it possibly just work?

If you URL encode a value, QGIS Server is decoded it so if you put %26%2347; in the url, you get & #47; server side. So if you put %2F or %252F, you get / server side. So to be sure to only have / for range, / in values is replaced by /.

@m-kuhn
Copy link
Member

m-kuhn commented Jul 8, 2021

Decoding happens inside QGIS server in qgsserverparameters.cpp as far as I can see.

https://github.com/qgis/QGIS/blob/master/src/server/qgsserverparameters.cpp#L570

If the encoded values are also kept available here, this should allow for a very clean solution (i.e. no double-encoding as in the current proposal in this PR. No changes to the GetCapabilities request, keep / and , and a normally (single) html encoded value in the request)

@m-kuhn
Copy link
Member

m-kuhn commented Jul 12, 2021

I would suggest to store the query as QUrlQuery in QgsServerParameters and this way make it available in it's raw, unprocessed form to the wmsrenderer and other implementations. I think that should be fairly straightforward and will offer the full power to implementations in the services. What do you think?

@github-actions
Copy link

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 27, 2021
@rldhont rldhont force-pushed the fix-server-wms-dimension-range branch from cd18ba5 to d0e311b Compare July 28, 2021 19:37
@rldhont rldhont force-pushed the fix-server-wms-dimension-range branch from 2402517 to 55f0cd6 Compare August 14, 2021 05:32
@github-actions
Copy link

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@github-actions github-actions bot closed this Aug 22, 2021
@rldhont rldhont deleted the fix-server-wms-dimension-range branch May 10, 2022 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport queued_ltr_backports Queued Backports Bug Either a bug report, or a bug fix. Let's hope for the latter! Server Related to QGIS server stale Uh oh! Seems this work is abandoned, and the PR is about to close.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants