Spring 7 and Jackson 3 update

[vuilleumierc]

No description provided.

[aaime]

Overall looks good.
On the GeoServer side I'm also upgrading Spring security, and I've found that this PR would still build with these changes in the main pom:

+    <spring.version>7.0.2</spring.version>
+    <spring.security.version>7.0.2</spring.security.version>

Could you include them in the PR?

[aaime]

Temporary... as in, during development, or needed for the PR to work?

[vuilleumierc]

It is needed for the PR to work, otherwise version 2.19.0 is being pulled transitively.
I can remove it if we merge the geotools PR before this one, what do you think?

[aaime]

I also noticed that the changes to the seed controller, while apparently an improvement, will cause regressions on existing automation scripts.

Check this test in GeoServer, already modified to pass:

@Test
    public void testPostSeedJson() throws Exception {
        final String layerName = getLayerId(MockData.BASIC_POLYGONS);
        final String url = "gwc/rest/seed/" + layerName + ".json";

        final String json = "{ \"seedRequest\": {\n"
                + "  \"name\": \""
                + layerName
                + "\",\n"
                + "  \"gridSetId\": \"EPSG:4326\",\n"
                + "  \"zoomStart\": 0,\n"
                + "  \"zoomStop\": 12,\n"
                + "  \"type\": \"seed\",\n"
                + "  \"threadCount\": 1,\n"
                + "}}";

        MockHttpServletResponse sr = postAsServletResponse(url, json, "application/json");
        assertEquals(200, sr.getStatus());
        assertSeedJob(layerName);
    }

The POST request passes only if the client specifies the content type to be "application/json", something that should be implied by the use of layername.json. The older version of the test did not, and was getting handled by this controller method instead:

 @RequestMapping(
            value = "/seed/{layer:.+}",
            method = RequestMethod.POST,
            consumes = {MediaType.APPLICATION_FORM_URLENCODED_VALUE, MediaType.ALL_VALUE})
    public ResponseEntity<?> doPost(
            HttpServletRequest request,
            InputStream inputStream,
            @PathVariable String layer,
            @RequestParam Map<String, String> params) {

I would consider reverting the changes to preserve compatibility.

[vuilleumierc]

I also noticed that the changes to the seed controller, while apparently an improvement, will cause regressions on existing automation scripts.

I'm looking into this, thank you for testing

@aaime I tried reverting the changes in the SeedController, but they are needed for the path mapping, otherwise these tests are failing:

[ERROR]   RestIntegrationTest.testSeedGet:1050 expected:<200> but was:<405>
[ERROR]   RestIntegrationTest.testSeedGetJson:1057 expected:<200> but was:<405>
[ERROR]   RestIntegrationTest.testSeedPost:1029 expected:<200> but was:<500>

When removing the consumes parameters (see latest commit), then Spring does not check the content type header and the test you mention passes.
But maybe you had something else in mind?

However, two other ones are failing and I cannot figure out what's happening exactly:

[ERROR] Tests run: 23, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 8.279 s <<< FAILURE! -- in org.geoserver.gwc.RESTIntegrationTest
[ERROR] org.geoserver.gwc.RESTIntegrationTest.testPost -- Time elapsed: 0.074 s <<< FAILURE!
java.lang.AssertionError: expected:<200> but was:<415>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
	at org.geoserver.gwc.RESTIntegrationTest.testPost(RESTIntegrationTest.java:493)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

[ERROR] org.geoserver.gwc.RESTIntegrationTest.testPostLegacyAutoStyles -- Time elapsed: 0.003 s <<< FAILURE!
java.lang.AssertionError: expected:<200> but was:<415>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
	at org.geoserver.gwc.RESTIntegrationTest.testPostLegacyAutoStyles(RESTIntegrationTest.java:550)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   RESTIntegrationTest.testPost:493 expected:<200> but was:<415>
[ERROR]   RESTIntegrationTest.testPostLegacyAutoStyles:550 expected:<200> but was:<415>
[INFO] 
[ERROR] Tests run: 23, Failures: 2, Errors: 0, Skipped: 0

Note that they test an endpoint that has been marked as deprecated since 2018..

[aaime]

A note about the integration testing here... we need to have the 3 pull requests working as one, because they will land together. So this one needs to fit toghether with:

• [GEOT-7866] Update GeoTools to Jackson 3 geotools/geotools#5514
• [GEOS-12039] Upgrade to Spring 7 geoserver/geoserver#9074

I did the builds, and this PR builds fine without the temporary jackson version override, indeed, once built on top of the GeoTools jackson upgrade. I've also verified that the GeoServer build works fine in the current state of this PR, no more failures.

Long story short, I believe the temporary override can be removed, and it would be nice if you could verify that the 3 PRs are working in harmony on your machine too (there is no way to verify that on PRs alone, they need to land together).

[jodygarnett]

update: never mind, the warning is present in spring6 also

---

I have been able to verify that the three PRs are working on my machine.

I note one GWC related warning on shutdown:

30 Jan. 11:15:41 WARN [support.DisposableBeanAdapter] - Custom destroy method 'destroy' on bean with name 'gwcMemoryBlobStore' propagated an exception: java.util.concurrent.RejectedExecutionException: Task java.util.concurrent.FutureTask@6c5c8c30[Not completed, task = org.geowebcache.storage.blobstore.memory.MemoryBlobStore$BlobStoreTask@13b45afe] rejected from java.util.concurrent.ThreadPoolExecutor@7a6a5eae[Terminated, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 1]

[vuilleumierc]

OK the dependency override is removed. I have also tested the 3 PRs together:

• build of the 3 projects (geotools -> GWC -> GeoServer) is OK
• build and tests of GWC works fine
• on the GeoServer branch, I tested the gwc-rest integration after re-building everything and I still have the two tests I mentioned failing (RESTIntegrationTest.testPost and RESTIntegrationTest.testPostLegacyAutoStyles). But if this happens only my machine, I would write it off as some local mishap on my part

[aaime]

@vuilleumierc I just rebuilt the 3 projects without problems, but the difference is worrysome.
Could it be that one of the builds on your end did not have "-nsu" to avoid downloading jars from remote repositories?
Or is it a JDK difference, are you using JDK 17?

[vuilleumierc]

@vuilleumierc I just rebuilt the 3 projects without problems, but the difference is worrysome. Could it be that one of the builds on your end did not have "-nsu" to avoid downloading jars from remote repositories? Or is it a JDK difference, are you using JDK 17?

I rebuilt the whole thing with -nsu and it works - thank you for the hint :) (and I am indeed on JDK 17)

[aaime]

Merging the 3 PRs and then checking if everything is fine on the build server