[GEOS-11275] Wicket 9 upgrade #7154

bradh · 2023-10-04T00:59:37Z

Work in progress on Wicket 8 migration (https://github.com/geoserver/geoserver/wiki/Jakarta-EE#wicket)

Look for "TODO WICKET8" for remaining work. Mostly this is verification, which should occur at the end. However there are broken modules and broken tests that need to be debugged.

Checklist

I have read the contribution guidelines.
I have sent a Contribution Licence Agreement (not required for small changes, e.g., fixing typos in documentation).
First PR targets the main branch (backports managed later; ignore for branch specific issues).
All the build checks are green (see automated QA checks).

For core and extension modules:

New unit tests have been added covering the changes.
Documentation has been updated (if change is visible to end users).
The REST API docs have been updated (when changing configuration objects or the REST controllers).
There is an issue in the GeoServer Jira (except for changes that do not affect administrators or end users in any way).
Commit message(s) must be in the form [GEOS-XYZWV] Title of the Jira ticket.
Bug fixes and small new features are presented as a single commit.
Each commit has a single objective (if there are multiple commits, each has a separate JIRA ticket describing its goal).

aaime · 2023-11-06T17:08:51Z

@bradh I was thinking, if most of the remaining work is to interactively test stuff, maybe we could try to involve others as well (e.g., users) just asking to run side-by-side a GeoServer 2.24.x and a binary from this branch? Maybe setup a spreadsheet of pages to test to divide work. Just thinking out loud.

bradh · 2023-11-06T22:13:25Z

There is still one part that I need to fix: https://github.com/geoserver/geoserver/pull/7154/files#diff-150ffebe59f506d34ef00088e4dbe5e57cb4372a806a40c249ce7f0b00c3aca1

I am currently thinking something like:

diff --git a/src/web/security/core/src/test/java/org/geoserver/security/web/AbstractSecurityNamedServicePanelTest.java b/src/web/security/core/src/test/java/org/geoserver/security/web/AbstractSecurityNamedServicePanelTest.java
index e135f31965..268e9fa4cb 100644
--- a/src/web/security/core/src/test/java/org/geoserver/security/web/AbstractSecurityNamedServicePanelTest.java
+++ b/src/web/security/core/src/test/java/org/geoserver/security/web/AbstractSecurityNamedServicePanelTest.java
@@ -14,9 +14,12 @@
 import java.util.List;
 import org.apache.wicket.Component;
 import org.apache.wicket.ajax.AjaxRequestHandler;
+import org.apache.wicket.ajax.markup.html.AjaxLink;
 import org.apache.wicket.extensions.ajax.markup.html.modal.ModalWindow;
 import org.apache.wicket.markup.html.form.CheckBox;
 import org.apache.wicket.markup.html.form.FormComponent;
+import org.apache.wicket.markup.html.list.ListItem;
+import org.apache.wicket.markup.html.list.ListView;
 import org.apache.wicket.markup.repeater.Item;
 import org.apache.wicket.markup.repeater.data.DataView;
 import org.apache.wicket.util.tester.FormTester;
@@ -203,27 +206,24 @@ protected String getSecurityConfigClassName() {
         return formTester.getForm().get("config.className").getDefaultModelObjectAsString();
     }
 
-    @SuppressWarnings("deprecation")
     protected <T extends SecurityNamedServicePanelInfo> void setSecurityConfigClassName(
-            Class<T> clazz) {
-        // TODO WICKET8 - This should not be commented out.
-        // ListView list = (ListView)
-        // tester.getLastRenderedPage().get("servicesContainer:services");
-        // int toClick = -1;
-        // for (int i = 0; i < list.getList().size(); i++) {
-        //     if (clazz.isInstance(list.getList().get(i))) {
-        //        toClick = i;
-        //        break;
-        //    }
-        // }
-        // AjaxLink link = (AjaxLink) list.get(toClick).get("link");
-        // if (link.isEnabled()) {
-        //    tester.executeAjaxEvent(link, "click");
-        // }
-        //        formTester.select("config.className", index);
-        //
-        // tester.executeAjaxEvent(formTester.getForm().getPageRelativePath()+":config.className",
-        // "change");
+            Class<T> clazz) throws Exception {
+        ListView list = (ListView) tester.getLastRenderedPage().get("servicesContainer:services");
+        list.forEach(
+                i -> {
+                    if (i instanceof ListItem) {
+                        ListItem listItem = (ListItem) i;
+                        listItem.forEach(
+                                action -> {
+                                    if (action instanceof AjaxLink) {
+                                        AjaxLink link = (AjaxLink) action;
+                                        if (link.isEnabled()) {
+                                            tester.executeAjaxEvent(link, "click");
+                                        }
+                                    }
+                                });
+                    }
+                });
     }
 
     protected void clickSave() {

but not sure I understand the base code that well, or how to adequately test this.

In terms of user testing, maybe that would be best saved for post- Wicket 9? Otherwise we'll just be going back to ask for more test help pretty soon.

For Wicket 9, I think the biggest work is going to be https://github.com/geoserver/geoserver/pull/7154/files#diff-0f799f2da8c62ce7254c4f4c914fd1f0ad5b3968a3bbc73c0fda75e7f458fe68
Any chance you want to tackle that?

aaime · 2023-11-07T11:24:48Z

Good idea to wait for wicket 9 as well before involving users (not sure you wanted to do that too given the PR title, but good we're moving there as well).

The link you referred me to shows 229 modified files, not sure which is the specific issue. But yeah, happy to help in general, just mind I might take a long time if it's big.

bradh · 2023-11-07T23:17:07Z

src/web/core/src/main/java/org/geoserver/web/wicket/CodeMirrorEditor.java

@@ -45,6 +41,9 @@
 *
 * @author Andrea Aime
 */
+// TODO WICKET8 - migrate to https://yauaa.basjes.nl/ per


@aaime This is the codemirror part.

bradh · 2023-11-07T23:17:34Z

Good idea to wait for wicket 9 as well before involving users (not sure you wanted to do that too given the PR title, but good we're moving there as well).

I think I'm well advanced on wicket 9 now. At least it builds and passes CI. A lot of deprecation suppressions for the ModalWindow change (which will block Wicket 10), and haven't tested the runtime impact of the CSP policy yet.

The link you referred me to shows 229 modified files, not sure which is the specific issue. But yeah, happy to help in general, just mind I might take a long time if it's big.

I hoped it would go to a file within that list. The problem is the codemirror editor, which uses functions that are deprecated in wicket 8 and removed in wicket 9, to do sniffing of browser versions. Have added a comment that should take you to that.

aaime · 2023-11-11T15:47:17Z

Regarding the diff in AbstractSecurityNamedServicePanelTest, I had a look at the code. I don't remember working on it before, but semantically, the new bits are not doing the same thing. The old code was getting a ListView, was checking its contents, and if the item in it was matching a given reference class, it was extracting the ajax links, and clicking it.
The current code just iterates over all the the elements in the list, finds all ajax links in them, and clicks each one of them.
It might incidentally end up getting the same result (only one enabled ajax link anyways), but it does not seem to be the same in general.

aaime · 2023-11-11T16:06:06Z

Wow, this browser detection thing is a real can of work. I get that the old Wicket code might have been imprecise, but Yauaa is really overboard:

To actually use it in your application you need to create an instance of the UserAgentAnalyzer.

Please instantiate a new UserAgentAnalyzer as few times as possible because the initialization step for a full UserAgentAnalyzer (i.e. all fields) usually takes something in the range of 2-5 seconds and uses a few hundred MiB of memory to store all the analysis datastructures.

This analyzer can only do a single analysis at a time and to ensure correct working the main method is synchronized

We cannot seriously keep a pool of these objects around, each one with a monster "few hundred megabytes"... need to think of some alternative.

aaime · 2023-11-11T16:20:28Z

ua-parser seems promising size wise, but appears to have been abandoned months ago... it depends on a version of snakeyaml that has vulnerabilties, and PRs to upgrade Snakeyaml or just remove the dependency have been ignored.

aaime · 2023-11-11T16:29:33Z

Perhaps a simpler approach is to just assume that in 2023, no one should be using IE 8, Firefox less than 3, Safari less than 5, or Opera less than 9. Which seems to be a fair assumption based on the browser version stats in these page:

https://www.stetic.com/market-share/browser/
https://caniuse.com/usage-table

bradh · 2023-11-11T22:43:49Z

Perhaps a simpler approach is to just assume that in 2023, no one should be using IE 8, Firefox less than 3, Safari less than 5, or Opera less than 9. Which seems to be a fair assumption based on the browser version stats in these page:

Or if they are, they won't be upgrading geoserver. Its possible that those browsers will have other issues with wicket 10 features anyway.

So the replacement is just removal of the detection code.

aaime · 2023-11-12T10:35:19Z

The real "fun" seems to be replacing ModalWindow with ModalDialog ... as far as I can tell, it's more primitive but also more flexible. There is an example of how to rebuild a modal window like experience here:
https://github.com/apache/wicket/tree/master/wicket-examples/src/main/java/org/apache/wicket/examples/ajax/builtin/modal

That is going to affect quite a bit of places:

aaime@colossus ~/devel/git-gs (wicket8) $ git grep -l "new ModalWindow" | grep -v Test   | sort | uniq
src/community/cog/src/main/java/org/geoserver/web/data/store/cog/panel/CogInput.java
src/community/elasticsearch/src/main/java/org/geoserver/elasticsearch/ElasticConfigurationPanel.java
src/community/solr/src/main/java/org/geoserver/solr/SolrConfigurationPanel.java
src/extension/importer/web/src/main/java/org/geoserver/importer/web/ImportTaskTable.java
src/extension/metadata/src/main/java/org/geoserver/metadata/web/panel/ProgressPanel.java
src/extension/wps/web-wps/src/main/java/org/geoserver/wps/web/ComplexInputPanel.java
src/extension/wps/web-wps/src/main/java/org/geoserver/wps/web/WPSRequestBuilder.java
src/extension/wps/web-wps/src/main/java/org/geoserver/wps/web/WPSRequestBuilderPanel.java
src/web/core/src/main/java/org/geoserver/web/admin/ModuleStatusPanel.java
src/web/core/src/main/java/org/geoserver/web/data/layergroup/LayerGroupEntryPanel.java
src/web/core/src/main/java/org/geoserver/web/data/layergroup/RootLayerEntryPanel.java
src/web/core/src/main/java/org/geoserver/web/data/resource/FeatureResourceConfigurationPanel.java
src/web/core/src/main/java/org/geoserver/web/data/store/StorePanel.java
src/web/core/src/main/java/org/geoserver/web/wicket/browser/FileInput.java
src/web/core/src/main/java/org/geoserver/web/wicket/CRSPanel.java
src/web/core/src/main/java/org/geoserver/web/wicket/GeoServerDialog.java
src/web/demo/src/main/java/org/geoserver/web/demo/DemoRequestsPage.java
src/web/demo/src/main/java/org/geoserver/web/demo/ReprojectPage.java
src/web/wcs/src/main/java/org/geoserver/wcs/web/demo/WCSRequestBuilder.java
src/web/wcs/src/main/java/org/geoserver/wcs/web/demo/WCSRequestBuilderPanel.java
src/web/wms/src/main/java/org/geoserver/wms/web/data/AbstractStylePage.java
src/web/wms/src/main/java/org/geoserver/wms/web/WMSAdminPage.java

I'm thinking we could rebuild our own ModalWindow on top of ModalDialog, in a way that's mostly API compatible with the old deprecated class, at least for the few parts that we actually use.

bradh · 2023-11-15T08:57:05Z

Regarding the diff in AbstractSecurityNamedServicePanelTest, I had a look at the code. I don't remember working on it before, but semantically, the new bits are not doing the same thing. The old code was getting a ListView, was checking its contents, and if the item in it was matching a given reference class, it was extracting the ajax links, and clicking it. The current code just iterates over all the the elements in the list, finds all ajax links in them, and clicks each one of them. It might incidentally end up getting the same result (only one enabled ajax link anyways), but it does not seem to be the same in general.

I've reverted this.

bradh · 2023-11-16T01:04:22Z

I'm thinking we could rebuild our own ModalWindow on top of ModalDialog, in a way that's mostly API compatible with the old deprecated class, at least for the few parts that we actually use.

I've just pushed a commit that centralises the use of ModalWindow into a GSModalWindow wrapper.
The wrapper is here:
d82bc2b#diff-d848262a28b51311cd5d074fc78d30bc4c6c16e32f87be5c75d26dcf70d3a090

That shows what API we need for direct replacement.

Based on that, it might be worth doing a couple of implementations (possibly with a shared base class if we need to share styling). One case is the GeoServerDialog, which is the only place that uses a bunch of the GSModalWindow methods. Then the other implementation is the "everything else" case. A later refinement could be to create a GSModalPopup, since there is a bit of duplicated (probably copy-n-pasted) code spread across multiple packages for that.

aaime · 2023-11-16T08:56:56Z

I've just pushed a commit that centralises the use of ModalWindow into a GSModalWindow wrapper.

One deprecation suppression to rule them all! 🎉

aaime · 2023-12-08T08:37:22Z

Been trying to work on the dialog switch but it's too big, it's the type of rewrite that requires several hours of uninterrupted work, which I just can't carve out of my spare time now. I'd rebase the branch on top of main and propose a build for manual tests. I should be able to help with the (hopefully) small things that come out of the testing.

bradh · 2023-12-08T08:47:12Z

I'd rebase the branch on top of main and propose a build for manual tests.

Will do. I have some summer break time coming up soon, and should be able to do that.

I should be able to help with the (hopefully) small things that come out of the testing.

Appreciated.

bradh · 2023-12-09T01:05:40Z

Perhaps a simpler approach is to just assume that in 2023, no one should be using IE 8, Firefox less than 3, Safari less than 5, or Opera less than 9. Which seems to be a fair assumption based on the browser version stats in these page:

So the replacement is just removal of the detection code.

This is implemented in 801f682

bradh · 2023-12-09T01:15:35Z

Wicket 9 has a strict (perhaps aggressive would be a better term) default policy for Content Security Policy (CSP) - see https://github.com/apache/wicket/blob/master/wicket-user-guide/src/main/asciidoc/security/security_6.adoc

That is currently completely disabled by c16c752

I'd like to "do it right", which will be a bit of work and the manual testing should come after that. Mitigating more XSS and data injection attacks is worth it.

bradh · 2023-12-09T02:26:21Z

Problems to fix:

Opening logs, produces: "Uncaught TypeError: textArea is null
http://localhost:8080/geoserver/web/wicket/bookmarkable/org.geoserver.web.admin.LogPage?8&filter=false&filter=false:42"

aaime · 2024-01-02T15:32:30Z

Is this good for a manual test round, involving users?

bradh · 2024-01-02T23:29:46Z

Is this good for a manual test round, involving users?

I think its premature, but will need help to bash it into shape. Will follow-up on the mailing list with details.

bradh · 2024-01-03T23:02:21Z

src/web/core/src/main/java/org/geoserver/web/GeoServerApplication.java

+ .add(
+ CSPDirective.STYLE_SRC,
+ CSPDirectiveSrcValue.SELF,
+ CSPDirectiveSrcValue.UNSAFE_INLINE)


This is the line to remove to see problems with inline styling.

Much of it moves to geoserver.css, some was duplicating existing classes.

… syling

bradh · 2024-05-22T01:45:35Z

Nasty rebase....

jodygarnett · 2024-05-22T02:54:44Z

@bradh I do not have time to write a blog post; but GeoCat is in position to handle the A/B testing to confirm the pages work (when this is ready for manual testing).

bradh force-pushed the wicket8 branch from 7929bbc to 615b493 Compare October 4, 2023 02:09

bradh force-pushed the wicket8 branch from a825e81 to 6dfce30 Compare November 4, 2023 06:27

bradh force-pushed the wicket8 branch from 7cc32c1 to b8731c6 Compare November 7, 2023 23:10

bradh commented Nov 7, 2023

View reviewed changes

aaime changed the title ~~wip: initial work on Wicket 8 upgrade~~ Wicket 9 upgrade Nov 9, 2023

bradh force-pushed the wicket8 branch from d82bc2b to e283a82 Compare December 8, 2023 21:55

bradh force-pushed the wicket8 branch from f520439 to 987046e Compare December 12, 2023 02:05

bradh force-pushed the wicket8 branch from 987046e to 46a86e6 Compare December 26, 2023 03:50

bradh force-pushed the wicket8 branch from 46a86e6 to 2562bf6 Compare January 3, 2024 10:05

bradh commented Jan 3, 2024

View reviewed changes

aaime and others added 26 commits May 22, 2024 08:29

Updating the RAT community module to Wicket 8

25eba8e

wicket8: tentative fix for AbstractedSecurityNamedServicePanelTest

415e3c3

wicket9: initial conversion work

18f4bba

wicket9: additional fixes

3be401e

wicket9: add JUnit 5 dependencies

e5475a1

wicket9: suppress deprecation warnings for now

3c5a18a

wicket9: fix deprecated Duration use and remove suppression

6289efe

wicket8: update newly added elasticsearch refresh to wicket8/9

e26c142

wicket9: temporarily disable CSP

98c5c08

wicket9: remove browser checks for codemirror editor

4c2f111

wicket9: initial not-quite-disabled CSP

f09f4b4

wicket9: move inline script to renderHead() for log page

604dc2a

web: remove inline style on KeywordsEditor

6d0bdaa

wicket9: remove most of the inline styling

a0819f0

Much of it moves to geoserver.css, some was duplicating existing classes.

wicket: partial work on removing inline scripts

c2171bd

wicket9: migrate more inline scripts and styling

8698edd

wicket9: migrate codemirror line numbering style to CSS file

2251c75

Hide ajax indicator using custom javascript, as CSP won't allow local…

3fbf40e

… syling

This one is always hidden, easily replaced with a CSS class then

84abef7

Fix SRSDescription display none and inline script

1775298

Hide dialog div using a class rather than a local style

3f6c9c2

Avoid inline scripts in table panel search

983dd80

Merging wicket dialog from Brad

b445766

Cleanups after re-alining with main

bf8f932

Add missing dependency and remove test ignore

4d13ce1

wicket9: fix up merge problems

dc1df05

bradh force-pushed the wicket8 branch from 3271f98 to dc1df05 Compare May 22, 2024 01:43

jodygarnett mentioned this pull request May 29, 2024

[GEOS-11390] Replace TestWfsPost with Javascript Demo Page #7672

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GEOS-11275] Wicket 9 upgrade #7154

[GEOS-11275] Wicket 9 upgrade #7154

bradh commented Oct 4, 2023 •

edited by pull-request-badge bot

aaime commented Nov 6, 2023 •

edited

bradh commented Nov 6, 2023

aaime commented Nov 7, 2023

bradh Nov 7, 2023

bradh commented Nov 7, 2023

aaime commented Nov 11, 2023

aaime commented Nov 11, 2023

aaime commented Nov 11, 2023

aaime commented Nov 11, 2023

bradh commented Nov 11, 2023

aaime commented Nov 12, 2023

bradh commented Nov 15, 2023

bradh commented Nov 16, 2023 •

edited

aaime commented Nov 16, 2023

aaime commented Dec 8, 2023 •

edited

bradh commented Dec 8, 2023

bradh commented Dec 9, 2023

bradh commented Dec 9, 2023

bradh commented Dec 9, 2023 •

edited

aaime commented Jan 2, 2024

bradh commented Jan 2, 2024

bradh Jan 3, 2024

bradh commented May 22, 2024

jodygarnett commented May 22, 2024

[GEOS-11275] Wicket 9 upgrade #7154

Are you sure you want to change the base?

[GEOS-11275] Wicket 9 upgrade #7154

Conversation

bradh commented Oct 4, 2023 • edited by pull-request-badge bot

Checklist

aaime commented Nov 6, 2023 • edited

bradh commented Nov 6, 2023

aaime commented Nov 7, 2023

bradh Nov 7, 2023

Choose a reason for hiding this comment

bradh commented Nov 7, 2023

aaime commented Nov 11, 2023

aaime commented Nov 11, 2023

aaime commented Nov 11, 2023

aaime commented Nov 11, 2023

bradh commented Nov 11, 2023

aaime commented Nov 12, 2023

bradh commented Nov 15, 2023

bradh commented Nov 16, 2023 • edited

aaime commented Nov 16, 2023

aaime commented Dec 8, 2023 • edited

bradh commented Dec 8, 2023

bradh commented Dec 9, 2023

bradh commented Dec 9, 2023

bradh commented Dec 9, 2023 • edited

aaime commented Jan 2, 2024

bradh commented Jan 2, 2024

bradh Jan 3, 2024

Choose a reason for hiding this comment

bradh commented May 22, 2024

jodygarnett commented May 22, 2024

bradh commented Oct 4, 2023 •

edited by pull-request-badge bot

aaime commented Nov 6, 2023 •

edited

bradh commented Nov 16, 2023 •

edited

aaime commented Dec 8, 2023 •

edited

bradh commented Dec 9, 2023 •

edited