Fixes maps wms zoom limitation #1915

junqiu-lei · 2022-07-18T23:31:38Z

Signed-off-by: Junqiu Lei junqiu@amazon.com

Description

When using WMS to customize base tile maps in region map or coordinate maps, user aren’t able to get into higher zoom levels than opensearch maps service default maximum zoom level.

By this PR fixed, the WMS max zoom value can be reached to 22 which is originally deigned here

Issues Resolved

opensearch-project/maps#19

Check List

codecov-commenter · 2022-07-19T00:25:00Z

Codecov Report

Merging #1915 (9678a4b) into main (6dfe116) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1915      +/-   ##
==========================================
+ Coverage   67.48%   67.49%   +0.01%     
==========================================
  Files        3076     3076              
  Lines       59144    59144              
  Branches     8989     8989              
==========================================
+ Hits        39915    39921       +6     
+ Misses      17044    17040       -4     
+ Partials     2185     2183       -2

Impacted Files	Coverage Δ
.../maps_legacy/public/map/base_maps_visualization.js	`1.96% <0.00%> (ø)`
...ic/application/models/sense_editor/sense_editor.ts	`64.88% <0.00%> (+0.88%)`	⬆️
packages/osd-optimizer/src/node/cache.ts	`52.77% <0.00%> (+2.77%)`	⬆️
...s/osd-optimizer/src/node/node_auto_tranpilation.ts	`87.75% <0.00%> (+4.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dfe116...9678a4b. Read the comment docs.

vamshin · 2022-07-19T18:06:31Z

src/plugins/maps_legacy/public/map/base_maps_visualization.js

@@ -164,7 +164,7 @@ export function BaseMapsVisualizationProvider() {

      try {
        if (this._wmsConfigured()) {
-          if (WMS_MINZOOM > this._opensearchDashboardsMap.getMaxZoomLevel()) {
+          if (WMS_MAXZOOM > this._opensearchDashboardsMap.getMaxZoomLevel()) {


Good catch! Thanks for the fix.

Why do we need to also reset the min zoom level in this case (L168)? It seems like maybe the min and max zoom levels should each be reset under different conditions.

Could we get more insight the original logic here and the original intent and ensure a regression isn't happening by changing this? Seems like code that has existed since even before the legacy version 6.x and it's changing the value.

From my understanding the original code was checking if the max zoom level was less than 0 then it would reset the min and max to 0 and 22.

Now the logic is if 22 > than max zoom level then it will always set the max zoom level to 22. So it seems regardless of what the map is configure to it will always set the max zoom level to 22 even if the user's WMS does not support a zoom level of 22?

Hi @kavilla, There was a same issue reported at Kibana 7.1.1 at 2019 elastic/kibana#49709. From that issue context, we could see the WMS zoom level limitation wasn't correct in some versions and didn't get fixed.

Based on this previous PR on Kibana elastic/kibana@4e2401c#diff-8eef4c226bf8f716c6420fcec4400202088971b418cb9bc31bdc4cecb57bc365R117, I still think it's designed to checking with max zoom level with current existing max zoom level which is from opensearch maps service max zoom level.

If we don't set up the WMS_MAXZOOM to 22, the maps max zoom would firstly be set and limited by default maps service max zoom. So in order to make sure all WMS users are able to load their WMS tiles at max potential zoom level, we need to set up max zoom limitation to 22.

Hi @joshuarrrr, the code logic already exist since kibana, in my understand, here for WMS cases, it can make sure users always have zoom level from 0 - 22.

Hi @junqiu-lei, yeah the bug makes sense in that it checks the maps service and defines the max zoom level which doesn't make sense if WMS is defined and it should use WMS configurations.

But I think this logic is intentional to be the case somehow that value returns as a negative and then it has some default case that at least set the same level from a valid (non-negative) value. But with change, regardless what the user configures for max zoom we will always set max zoom to 22. This seems like it addresses the bug as much is the increases the maximum to 22 instead of 10. It also prevents the edge case where that max zoom is a negative value. If somehow the value returned is negative for max zoom then max zoom would be negative.

So is the bug actually occurring at another location? Because I see earlier in the file the map gets configured with these default values and it must be overwritten, but in the rest of this function it sets the options that were configured so perhaps that value is not being applied or the previous base layer needs to be nulled out for WMS to take those settings?

If that becomes to hairy, I'd rather keeps this logic and add a new conditional that does what you added while keeping intact the previous condtion.

Hi @kavilla, I updated the WMS zoom check condition to include both existing check and bug fix check, so that we can fix the bug and avoid abandon the existing logic.

vamshin

LGTM! Thanks

martin-gaievski · 2022-07-19T19:15:15Z

Do we have unit/any other type of tests in this project? If so I think we need to update tests and add checks for edge cases like min/max values

junqiu-lei · 2022-07-19T20:24:14Z

Do we have unit/any other type of tests in this project? If so I think we need to update tests and add checks for edge cases like min/max values

I didn't find any existing test file for base_maps_visualization.js to add for this PR. I have tested it manually on region maps and coordinate maps when at wms or default maps.

ashwin-pc

LGTM

joshuarrrr · 2022-07-22T19:06:56Z

Do we have unit/any other type of tests in this project? If so I think we need to update tests and add checks for edge cases like min/max values

@martin-gaievski We do have functional tests that test min/max zoom levels - see https://github.com/opensearch-project/OpenSearch-Dashboards/pull/1823/files, for example.

junqiu-lei · 2022-07-22T19:31:31Z

Do we have unit/any other type of tests in this project? If so I think we need to update tests and add checks for edge cases like min/max values

@martin-gaievski We do have functional tests that test min/max zoom levels - see https://github.com/opensearch-project/OpenSearch-Dashboards/pull/1823/files, for example.

Hi Josh, thanks for catching, I think the tests here are for default maps tiles zoom levels, not includes WMS, let me check if we can add the WMS zoom test here.

kavilla · 2022-07-26T08:18:27Z

src/plugins/maps_legacy/public/map/base_maps_visualization.js

@@ -164,7 +164,7 @@ export function BaseMapsVisualizationProvider() {

      try {
        if (this._wmsConfigured()) {
-          if (WMS_MINZOOM > this._opensearchDashboardsMap.getMaxZoomLevel()) {
+          if (WMS_MAXZOOM > this._opensearchDashboardsMap.getMaxZoomLevel()) {


Could we get more insight the original logic here and the original intent and ensure a regression isn't happening by changing this? Seems like code that has existed since even before the legacy version 6.x and it's changing the value.

From my understanding the original code was checking if the max zoom level was less than 0 then it would reset the min and max to 0 and 22.

Now the logic is if 22 > than max zoom level then it will always set the max zoom level to 22. So it seems regardless of what the map is configure to it will always set the max zoom level to 22 even if the user's WMS does not support a zoom level of 22?

junqiu-lei · 2022-07-27T20:30:41Z

Hi @kavilla, how do you think based on my last comment? We can have a offline quick sync about the WMS part code from Kibana if you think it's needed. Then I can update the PR with functional tests coordinately.

src/plugins/maps_legacy/public/map/base_maps_visualization.js

Signed-off-by: Junqiu Lei <junqiu@amazon.com>

kavilla

Thanks!

When using WMS to customize base tile maps in region map or coordinate maps, user aren’t able to get into higher zoom levels than opensearch maps service default maximum zoom level. Issue Resolved: opensearch-project/maps#19 Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit 4dd7f14)

When using WMS to customize base tile maps in region map or coordinate maps, user aren’t able to get into higher zoom levels than opensearch maps service default maximum zoom level. Issue Resolved: opensearch-project/maps#19 Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit 4dd7f14) Co-authored-by: Junqiu Lei <junqiu@amazon.com>

junqiu-lei added bug Something isn't working maps Issues or PRs related to the Maps Service labels Jul 18, 2022

junqiu-lei requested a review from a team as a code owner July 18, 2022 23:31

junqiu-lei self-assigned this Jul 18, 2022

vamshin reviewed Jul 19, 2022

View reviewed changes

vamshin previously approved these changes Jul 19, 2022

View reviewed changes

martin-gaievski previously approved these changes Jul 19, 2022

View reviewed changes

ashwin-pc previously approved these changes Jul 19, 2022

View reviewed changes

kavilla requested changes Jul 26, 2022

View reviewed changes

junqiu-lei dismissed stale reviews from ashwin-pc, martin-gaievski, and vamshin via 9609ff3 August 2, 2022 01:28

kavilla reviewed Aug 9, 2022

View reviewed changes

src/plugins/maps_legacy/public/map/base_maps_visualization.js Outdated Show resolved Hide resolved

Fix maps wms zoom limitation

e615a77

Signed-off-by: Junqiu Lei <junqiu@amazon.com>

junqiu-lei force-pushed the map-wms-zoom-patch branch from 9609ff3 to e615a77 Compare August 9, 2022 18:30

junqiu-lei requested review from kavilla and joshuarrrr August 9, 2022 18:33

kavilla approved these changes Aug 11, 2022

View reviewed changes

ashwin-pc approved these changes Aug 11, 2022

View reviewed changes

kavilla added backport 2.x v2.3.0 labels Aug 12, 2022

kavilla merged commit 4dd7f14 into opensearch-project:main Aug 12, 2022

opensearch-trigger-bot bot mentioned this pull request Aug 12, 2022

[Backport 2.x] Fixes maps wms zoom limitation #2136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes maps wms zoom limitation #1915

Fixes maps wms zoom limitation #1915

junqiu-lei commented Jul 18, 2022 •

edited

Loading

codecov-commenter commented Jul 19, 2022

vamshin Jul 19, 2022

joshuarrrr Jul 22, 2022

kavilla Jul 26, 2022 •

edited

Loading

junqiu-lei Jul 26, 2022 •

edited

Loading

kavilla Jul 29, 2022 •

edited

Loading

junqiu-lei Aug 2, 2022

vamshin left a comment

martin-gaievski commented Jul 19, 2022

junqiu-lei commented Jul 19, 2022

ashwin-pc left a comment

joshuarrrr commented Jul 22, 2022

junqiu-lei commented Jul 22, 2022

kavilla Jul 26, 2022 •

edited

Loading

junqiu-lei commented Jul 27, 2022

kavilla left a comment

Fixes maps wms zoom limitation #1915

Fixes maps wms zoom limitation #1915

Conversation

junqiu-lei commented Jul 18, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Jul 19, 2022

Codecov Report

vamshin Jul 19, 2022

Choose a reason for hiding this comment

joshuarrrr Jul 22, 2022

Choose a reason for hiding this comment

kavilla Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

junqiu-lei Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

kavilla Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

junqiu-lei Aug 2, 2022

Choose a reason for hiding this comment

vamshin left a comment

Choose a reason for hiding this comment

martin-gaievski commented Jul 19, 2022

junqiu-lei commented Jul 19, 2022

ashwin-pc left a comment

Choose a reason for hiding this comment

joshuarrrr commented Jul 22, 2022

junqiu-lei commented Jul 22, 2022

kavilla Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

junqiu-lei commented Jul 27, 2022

kavilla left a comment

Choose a reason for hiding this comment

junqiu-lei commented Jul 18, 2022 •

edited

Loading

kavilla Jul 26, 2022 •

edited

Loading

junqiu-lei Jul 26, 2022 •

edited

Loading

kavilla Jul 29, 2022 •

edited

Loading

kavilla Jul 26, 2022 •

edited

Loading