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

#7422: StyleEditor plugin must not rely on the workspace prefix inside the style names returned by GetCapabilities #7423

Merged
merged 5 commits into from
Oct 12, 2021

Conversation

allyoucanmap
Copy link
Contributor

@allyoucanmap allyoucanmap commented Oct 7, 2021

Description

This PR replaces the request GetCapabilities of the style editor with a rest/layers request to get the correct styles information. If the rest/layer request fails it fallback to GetCapabilities request to enable the styles selector

The Style Editor will work only with the styles included in the same workspace.

Please check if the PR fulfills these requirements

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

  • Bugfix

Issue

What is the current behavior?

#7422

What is the new behavior?

Styles can be edited and deleted if the users have permission

Breaking change

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

  • No

Other useful information

@allyoucanmap allyoucanmap added this to the 2021.02.00 milestone Oct 7, 2021
@allyoucanmap allyoucanmap self-assigned this Oct 7, 2021
@tdipisa tdipisa requested a review from dsuren1 October 7, 2021 09:51
@allyoucanmap allyoucanmap marked this pull request as draft October 8, 2021 08:01
@allyoucanmap allyoucanmap marked this pull request as ready for review October 11, 2021 09:23
@dsuren1
Copy link
Contributor

dsuren1 commented Oct 11, 2021

Hi @allyoucanmap, I have few questions
Map used: Demo map - 10358
Questions
Since we are showing only styles of current workspace of the layer (for authenticated user)

  • In some instance, it is not clear which style is selected (Ex: with ny_roads, actual style selected is tiger_roads but I can see Base SLD style is selected (since only one is shown)
  • And unable to make a new selection from the listed style (if there is only one)

Example

  • So can we not update styles of other workspace (since we cannot edit without authentication)

Style selected: tiger_roads (layer - ny_roads)
2021-10-11 18_39_25-

Apart from the ones above everything looks fine to me.

@allyoucanmap
Copy link
Contributor Author

Since we are showing only styles of current workspace of the layer (for authenticated user)

@dsuren1 we discussed with @tdipisa that due to new changes on new version of GeoServer the style editor will work only with the style available in its own workspace.

  • In some instance, it is not clear which style is selected (Ex: with ny_roads, actual style selected is tiger_roads but I can see Base SLD style is selected (since only one is shown)

Unfortunately mapstore is currently assuming that the first style it recieves in the list of available styles is the default style and this particular style has styles from different workspaces. I need to investigate further to understand how to fix this and I would like to create a new issue including the splitting of concepts such as style selection and style editor (style editor should never work with capabilities at this point) @tdipisa

  • And unable to make a new selection from the listed style (if there is only one)

we should be able to select the style with this new commit 91a1d4a . Before we stored the default style with an empty string but with this fix that store the actual name we should be able to see the correct style in the map

  • So can we not update styles of other workspace (since we cannot edit without authentication)

This is the behaviour we want from now on only styles of the same workspace of the layer can be edited. The GeoServer is showing the list of all styles even if the GetCapabilities is targeting a specific workspace, this behaviour is not correct and it should be a fix in the future that it will remove all the styles outside the workspace

@tdipisa
Copy link
Member

tdipisa commented Oct 12, 2021

This is the behaviour we want from now on only styles of the same workspace of the layer can be edited. The GeoServer is showing the list of all styles even if the GetCapabilities is targeting a specific workspace, this behaviour is not correct and it should be a fix in the future that it will remove all the styles outside the workspace

@allyoucanmap I would like to do an additional step directly within this PR to make things more clear for the user:

  1. We list in any case all styles
  2. If the user selects a style belonging to the layer workspace, the style is applied on map and the style editor is enabled
  3. If the user selects a global style (non in a workspace), the style is applied on map but the style editor is not enabled. In this case we should include a message somehow (an icon with a tooltip? only a tooltip?) to inform the user why the style editor cannot be used with the selected style
  4. The user guide here must be updated a bit to document the above new behavior: a warning just below the existing notes should be enough

Unfortunately mapstore is currently assuming that the first style it recieves in the list of available styles is the default style and this particular style has styles from different workspaces. I need to investigate further to understand how to fix this and I would like to create a new issue including the splitting of concepts such as style selection and style editor (style editor should never work with capabilities at this point) @tdipisa

@allyoucanmap, then you can also open a new issue to investigate/discuss a possible improvement to detach the selection part from the editing part and put in in 2021.02.01 or 2021.03.00 according also to the ms-geonode-client needs @giohappy.

@giohappy
Copy link
Contributor

@tdipisa can you calrofy why editing a style in the global workspace will be disabled?

@tdipisa
Copy link
Member

tdipisa commented Oct 12, 2021

@tdipisa can you calrofy why editing a style in the global workspace will be disabled?

@giohappy this was my understanding according to the recent GS updates and my discussion with @allyoucanmap. If there is still the ability to edit the global styles, why didn't you mention this in our call @allyoucanmap?

@allyoucanmap
Copy link
Contributor Author

why didn't you mention this @allyoucanmap in our call?

@tdipisa I'm sorry I'll check later

@giohappy
Copy link
Contributor

giohappy commented Oct 12, 2021

let's recap @allyoucanmap. From the discussion with @aaime global styles or styles from other workspaces souldn't leask into the GetCapabilities under a workspace. So, if the style editor only relies on the workspaced GC the user shouldn never see styles from outside the workspace itself.

BUT currently Geoserver has a bug and external styles are also listed, although without any means to tell where those styles are coming from.
So, I guess this is still a problem in case you cannot rely on the REST API, since you will never now if that style is local to the workspace ore coming from elsewhere. Right @allyoucanmap?

In any case I don't think this affects the ability to edit a global style. Does it?

@tdipisa
Copy link
Member

tdipisa commented Oct 12, 2021

Let's meet again @giohappy and @allyoucanmap if needed

@allyoucanmap
Copy link
Contributor Author

allyoucanmap commented Oct 12, 2021

In any case I don't think this affects the ability to edit a global style. Does it?

@giohappy yes, you can edit the global styles. I got confused, now I'm working on the fix

@aaime
Copy link
Member

aaime commented Oct 12, 2021

Global style can legitimately leak into a workspace specific caps document, unless they end up being covered by a same-named local style (in that case, the latter wins). Styles from other workspaces should not be there instead.

@allyoucanmap
Copy link
Contributor Author

@tdipisa @giohappy

removed the restriction of editing in workspace here e2a402d

now the editing works also for global styles

@giohappy
Copy link
Contributor

@aaime so if I a style is used inside requests toward a workspace, without prefixing its name with the workspace, either a global or a local style is selected automatically. Do I understand it correctly?

@aaime
Copy link
Member

aaime commented Oct 12, 2021

If the service used is workspace specific (the workspace appears in the path) and there is a naming conflict between local and global styles, the local one is picked. It should not be possible to access the global one in that situation.

@tdipisa tdipisa self-requested a review October 12, 2021 13:04
@tdipisa tdipisa merged commit e980e77 into geosolutions-it:master Oct 12, 2021
@tdipisa tdipisa added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Oct 12, 2021
@tdipisa
Copy link
Member

tdipisa commented Oct 13, 2021

@ElenaGallo all good for me in DEV. Please do a double check and let us know for the backport to 2021.02.xx.

@ElenaGallo
Copy link
Contributor

Test passed, @allyoucanmap please backport to 2021.02.xx.

allyoucanmap added a commit to allyoucanmap/MapStore2 that referenced this pull request Oct 14, 2021
…ce prefix inside the style names returned by GetCapabilities (geosolutions-it#7423)

* replace get capabilities request with GeoServer rest api to get information about styles

* get missing styles information from get capabilities

* store the current default style name

* remove restriction on workspace for editing
tdipisa pushed a commit that referenced this pull request Oct 14, 2021
…e the style names returned by GetCapabilities (#7423) (#7447)

* replace get capabilities request with GeoServer rest api to get information about styles

* get missing styles information from get capabilities

* store the current default style name

* remove restriction on workspace for editing
@tdipisa tdipisa removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Oct 14, 2021
allyoucanmap added a commit that referenced this pull request Oct 14, 2021
…de the style names returned by GetCapabilities (#7423)

* replace get capabilities request with GeoServer rest api to get information about styles

* get missing styles information from get capabilities

* store the current default style name

* remove restriction on workspace for editing
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.

6 participants