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

#8597 Simplifying query params #8617

Merged

Conversation

alex-fko
Copy link
Contributor

@alex-fko alex-fko commented Sep 23, 2022

Description

Part of the query parameters have a complex syntax and may be hard to use by end-user.
This PR updates existing and add new query parameters with simple syntax.

  • featureInfo parameter got a simple syntax: #/viewer/openlayers/new?featureInfo=38.72,-95.625 where lat and lng are comma-separated values.

  • mapInfo parameter allows to use SEARCH_LAYER_WITH_FILTER action upon specific layet to get feature info based on filter criteria:
    #/viewer/openlayers/new?addLayers=tiger:poly_landmarks;gs_stable_wms&mapinfo=tiger:poly_landmarks&mapInfoFilter=CFCC='H41'
    mapInfoFilter in this case is a supplementary parameter to pass cql filter in it.
    Previously it was not possible to trigger SEARCH_LAYER_WITH_FILTER on a layer that is added via query parameters. Now if both addLayers and mapInfo parameters present in a query string, app will compare name of layer in mapInfo and will delay search execution whenever layers is also listed in addLayers. Example above will delay search execution because tiger:poly_landmarks is in both parameters.

  • addLayers is equivalent of ADD_LAYERS_FROM_CATALOGS action: #/viewer/openlayers/new?addLayers=tiger:poly_landmarks;gs_stable_wms,tiger-ny;gs_stable_wms&layerFilters=CFCC='H41'
    layerFilters is a supplementary parameter to pass cql filter applied upon layer addition.

  • background allows to dynamically add background to the map and activate it. Supports default backgrounds provided by static service defined in localConfig.json (default_map_backgrounds) as well as other layers:
    #/viewer/openlayers/new?background=Sentinel
    #/viewer/openlayers/new?background=tiger-ny;gs_stable_wms
    Please note that default service for background is default_map_backgrounds. Based on the selection criteria for this service it is enough to pass desired layer name even partially, e.g. background=Sen, it will use first layer matching search criteria.

All parameters from now on support both camelCase and lowercase, e.g. addLayers and addlayers.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

What is the current behavior?

#8597

What is the new behavior?
Existing parameters syntax is updated according to proposal.
New parameters are added (mapInfo, background, addLayers).
Added support of having mapInfo processor applied to the dynamically loaded layer (via addLayers)

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

Support of delayed execution of mapInfo request upon layer added via query parameters
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the functionality and it works quite well.

I noticed something that should be improved in doc and specification, moreover some syntax issue that we didn't noticed during the planning.

  • You used in the example the separator | instead of ;. I see the application manage both. I think we should manage only one to avoid confusion or possible overlaps with reserved chars. both ; and | do not seems to be used in CQL, but | is less common to find in many keyboards, while ; is easier. For this reason I should use only ;. Anyway we have some problems with some special chars, so see comments below.
  • I noticed during the review that both "layer names" and "cql_filters" may contain , and cql_filter can contain " chars. Here we are using these chars to separate values.

@alex-fko followed the specification correctyl but we have to decide what to do to avoid issues.

In particular
- , in layer names can be send to WMS to require more than one layers in the same call overlapped
- , in cql_filter are used in filters IN
- " in cql_filter are used to specifiy attribute names and it is needed when these names have speces. E.g. the attribute filter FEATURE SIZE = 1 is not valid. While "FEATURE_SIZE" = 1 is the correct syntax.

Alternative solutions should include complex escaping strategies that may make the syntax and the code more complex.

Proposed changes

I propose to change:

  • | to separate layers and cql_filters. | is not present in ecql specification (or at least I couldn't find it), while ";" is persent, and for consistency with the syntax, we should use the same separator for layers and filters.
  • ; to separate layer name and service
  • " remove initial and final " in the filters in the original proposal, that are unnecessary and overlap with the cql_sintax.

@tdipisa @alex-fko please let me know your opinion about it.

  • I also added some changes to the doc in general, to clarify more the usage and to correct generated doc indentation. please apply my changes (and check them on the generated doc).

docs/developer-guide/map-query-parameters.md Outdated Show resolved Hide resolved
docs/developer-guide/map-query-parameters.md Outdated Show resolved Hide resolved
docs/developer-guide/map-query-parameters.md Outdated Show resolved Hide resolved
docs/developer-guide/map-query-parameters.md Outdated Show resolved Hide resolved
docs/developer-guide/map-query-parameters.md Outdated Show resolved Hide resolved
docs/developer-guide/map-query-parameters.md Outdated Show resolved Hide resolved
docs/developer-guide/map-query-parameters.md Show resolved Hide resolved
@tdipisa
Copy link
Member

tdipisa commented Sep 27, 2022

I tested the functionality and it works quite well.

I noticed something that should be improved in doc and specification, moreover some syntax issue that we didn't noticed during the planning.

  • You used in the example the separator | instead of ;. I see the application manage both. I think we should manage only one to avoid confusion or possible overlaps with reserved chars. both ; and | do not seems to be used in CQL, but | is less common to find in many keyboards, while ; is easier. For this reason I should use only ;. Anyway we have some problems with some special chars, so see comments below.
  • I noticed during the review that both "layer names" and "cql_filters" may contain , and cql_filter can contain " chars. Here we are using these chars to separate values.

@alex-fko followed the specification correctyl but we have to decide what to do to avoid issues.

In particular - , in layer names can be send to WMS to require more than one layers in the same call overlapped - '," in cql_filter are used in filters IN - " in cql_filter are used to specifiy attribute names and it is needed when these names have speces. E.g. the attribute filter FEATURE SIZE = 1 is not valid. While "FEATURE_SIZE" = 1 is the correct syntax.

Proposed changes

I propose to change:

  • | to separate layers and cql_filters. | is not present in ecql specification, while ";" is persent, and for consistency with the syntax, we should use the same separator for layers and filters.
  • ; to separate layer name and service
  • " remove initial and final " in the filters in the original proposal, that are unnecessary and overlap with the cql_sintax.

@tdipisa @alex-fko please let me know your opinion about it.

  • I also added some changes to the doc in general, to clarify more the usage and to correct generated doc indentation. please apply my changes (and check them on the generated doc).

@offtherailz let's have a call tomorrow to check this together please.

@tdipisa
Copy link
Member

tdipisa commented Sep 27, 2022

Proposed changes

I propose to change:

  • | to separate layers and cql_filters. | is not present in ecql specification (or at least I couldn't find it), while ";" is persent, and for consistency with the syntax, we should use the same separator for layers and filters.

| is not so common for these purposes and I don't think ; can generate problems in the use cases we need to cover. Consider that also GeoServer allow CQL filters separation using ;

https://gs-stable.geosolutionsgroup.com/geoserver/gs/wms?service=WMS&version=1.1.0&request=GetMap&layers=gs%3Any_landmarks,gs:ny_roads&CQL_FILTER=LANAME=%27Central%20Park%27;NAME=%27West%20Dr%27&bbox=-74.047185%2C40.679648%2C-73.90782%2C40.882078&width=528&height=768&srs=EPSG%3A4326&styles=&format=image/png

As far as the separator for layers is concerned I don't see any reason to use , that is the most common separator. It is not a problem in general to use different separators if needed for different query params while it is a problem instead to allow two different separators for the same param :-) that it is effectively confusing.

  • ; to separate layer name and service

ok with this, only ; is requested as per original proposal.

  • " remove initial and final " in the filters in the original proposal, that are unnecessary and overlap with the cql_sintax.

Ok, if it is not necessary we can remove "

alex-fko and others added 2 commits September 28, 2022 11:45
Co-authored-by: Lorenzo Natali <lorenzo.natali@geosolutionsgroup.com>
@alex-fko
Copy link
Contributor Author

@tdipisa @offtherailz
I found | as separator to be more prominent in query string making it a little bit more readable because semicolon ; is getting lost surrounded by letters. Anyway, as discussed above, I removed it in favor of keeping only one type of separator: ;.

So now:
; - is used as a delimiter for layer name and service;
, - is used as a delimiter for layer;service definitions & for cql filters;

Double quotes are removed from examples in documentation.
Documentation is updated.

@offtherailz
Copy link
Member

offtherailz commented Sep 29, 2022

@alex-fko cql_filter syntax contains , so we should separate filters with ";" (as GeoServer does).

So summarising:

  • layer;service,layer2;service2
  • cql_filter;cql_filter2

Unfortunately this is the only syntax that doesn't requre escaping. It is a little asymmetric but I can not imagine a different solution.

Maybe the usage of ; in 2 contexts may be confusing, so your proposal of using | makes more sense. Anyway I should proceed with with ; if @tdipisa agrees.

So we have to separate also cql_filters with ;. This is still missing.

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ";" as separator for cql_filters and update the examples in the PR to test as asked.

, is reserved for cql

; is used, but only to separate conditions ( filter expressions ) that is exactly the usage we are having now. Also geoserver allows this this way.

Reference:
https://github.com/geotools/geotools/blob/main/modules/library/cql/ECQL.md

if (parsed.length) {
const defaultSource = selectedServiceSelector(state);
const pairs = parsed.map(el => el.split(";"));
const layerFilters = (parameters.layerFilters ?? '').split(',') ?? [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use ";". "," is reserved in cql.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated it

docs/developer-guide/map-query-parameters.md Outdated Show resolved Hide resolved
@offtherailz offtherailz self-requested a review September 29, 2022 09:32
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the description (and/or the issue) to make it testable from the test team.

@alex-fko
Copy link
Contributor Author

Description is updated, here is the complex example that might be useful to test changes on dev:
#/viewer/openlayers/new?addLayers=unesco:Unesco_point;gs_stable_wms&layerFilters=cod_unesco='IT_1025'&background=Sen&mapInfo=unesco:Unesco_point&mapInfoFilter=cod_unesco='IT_1025'

@offtherailz offtherailz merged commit 382c3f1 into geosolutions-it:master Sep 29, 2022
@offtherailz offtherailz added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Sep 29, 2022
@tdipisa
Copy link
Member

tdipisa commented Sep 29, 2022

@ElenaGallo please test this in DEV and let us know if we can backport to 2022.02.xx

@tdipisa tdipisa added this to the 2022.02.01 milestone Sep 29, 2022
@ElenaGallo
Copy link
Contributor

Test passed on DEV, @alex-fko please backport to 2022.02.xx. Thanks

alex-fko added a commit to alex-fko/MapStore2 that referenced this pull request Sep 30, 2022
alex-fko added a commit to alex-fko/MapStore2 that referenced this pull request Oct 4, 2022
@alex-fko alex-fko removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants