Skip to content

Add optional Inspire extension support to GeoServer Cloud #617

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

Merged
merged 1 commit into from
Apr 24, 2025

Conversation

buehner
Copy link
Member

@buehner buehner commented Feb 19, 2025

Description: This PR introduces the optional activation of the Inspire extension in GeoServer Cloud. The Inspire extension enhances GetCapabilities documents for WMS, WFS, WCS, and WMTS to comply with metadata requirements from the European Union.

Key Changes:

  • Introduce a new configuration option: geoserver.extension.inspire.enabled=false (activate the integration with true) add config for inspire extension geoserver-cloud-config#21
  • Conditionally integrate the required beans into the respective Web-UI, WMS, WFS, WCS and GWC/WMTS apps.
  • Implemented numerous tests and extended existing ones
  • Fix for Deserialization Issues with pgconfig (see below)

Fix for Deserialization Issues with pgconfig:

When using pgconfig, deserialization issues arose due to JSONB column handling. Specifically, the LiteralDeserializer expected attributes to be parsed in a specific order (1. contentType, 2. value), which was not guaranteed with JSONB storage. This caused issues (especially in case of WCS and WMTS as they persist a spatialDatasetIdentifier property, see below). The PR implements a fallback to handle cases where the value attribute is parsed before contentType.

Example JSONB Snippet from the info column of the serviceinfo table:

"inspire.spatialDatasetIdentifier": {
  "Literal": {
    "type": "java.util.List",
    "value": [
      {
        "code": "25833",
        "namespace": "wcs",
        "metadataURL": "wcs"
      }
    ],
    "contentType": "org.geoserver.inspire.UniqueResourceIdentifier"
  }
}

Looking forward to your feedback @groldan 😄

@buehner
Copy link
Member Author

buehner commented Feb 19, 2025

Seems a test is failing now. I will try to address this soon

@buehner buehner force-pushed the add-gs-inspire-extension branch 3 times, most recently from 9d44d88 to f9d025d Compare February 20, 2025 00:17
@buehner buehner requested a review from groldan February 20, 2025 09:00
@buehner
Copy link
Member Author

buehner commented Feb 20, 2025

Seems a test is failing now. I will try to address this soon

I could fix the code to not break any existing unit tests (so make test is fine), but for some reason the acceptance also fail

Copy link
Member

@groldan groldan left a comment

Choose a reason for hiding this comment

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

Hey, this is good thanks.
I'm gonna ask you just a couple things:

  • rebase on top of main and use @ImportFilteredResource instead of @ImportResource as introduced in #621
  • extract the fix to LiteralDeserializer to a separate pull request with tests for it

The other thing is I'd rather have a single module for the Inspire extension instead of the autoconfigurations being spread out over the different apps. But I won't make it a blocker cause I'm planning on making it more clear both in code and documentation, so we'll probably move them to their own extensions/inspire module later on a single refactoring.

if (null == contentType) {
value = parser.readValueAs(ArrayList.class);
// store the values to parse the collection later
Copy link
Member

Choose a reason for hiding this comment

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

Please create a separate pull request for LiteralDeserializer with test cases.
You can extend ExpressionSerializationTest

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 rebase is done and push-forced. I also switched to the @ImportFilteredResource

will start a separate branch for the LiteralDeserializer now

@buehner
Copy link
Member Author

buehner commented Mar 6, 2025

I created a separate PR for the LiteralDeserializer issue: #631

This also contains some more improvements compared to the previous changes. In case it can be merged, I would rebase and adjust this one here again.

@buehner buehner force-pushed the add-gs-inspire-extension branch from ca330fa to 2128f19 Compare March 7, 2025 09:52
@buehner
Copy link
Member Author

buehner commented Mar 7, 2025

The other thing is I'd rather have a single module for the Inspire extension instead of the autoconfigurations being spread out over the different apps. But I won't make it a blocker cause I'm planning on making it more clear both in code and documentation, so we'll probably move them to their own extensions/inspire module later on a single refactoring.

@groldan I am planning to add more extensions. Would be great to know how you want to have such extensions included. Is there any existing integration that I could learn from or are you planning a new approach here?

@groldan
Copy link
Member

groldan commented Apr 8, 2025

Hi @buehner, I've been organizing extensions in the src/extensions project, with some basic docs on how to add extensions

Basically all apps will depend on gs-cloud-starter-extensions, and it's the extensions themselves that implement mechanisms to load components based on the target platform (look at the new @ConditionalOnGeoServerWebUI, @ConditionalOnGeoServerWFS, etc.)

Please update this PR to follow that pattern instead of having to touch the applications themselves. I hope this helps in creating self-contained and self-explanatory extensions. Let me know if you have any question.

@buehner
Copy link
Member Author

buehner commented Apr 11, 2025

FYI I am currently rebasing and refactoring this against the latest changes. I will push force here when ready

@buehner buehner force-pushed the add-gs-inspire-extension branch from 2128f19 to 1dbfe20 Compare April 11, 2025 14:04
@buehner
Copy link
Member Author

buehner commented Apr 11, 2025

@groldan I made an update to your latest refactoring. Thank you for the explanations.

But for the time being, this is still WIP as I'd like to clarify two things:

  1. The new @ConditionalOnGeoServerWFS (etc.) do not work for me as I would expect it. I am using them, for example here, but this is only working because i dislabled this line So for some reason the wfsServiceTarget bean is not available (seems to be the same for the others, I also added a missing GWC conditional) ? Do you have an idea?

  2. I did not yet move any tests i created before, so they are still in the src/apps/geoserver/**/test subdirectories. They are still working fine. In a follow up, I can move the tests for bean existance to the new test in src/extensions , but other tests check basic INSPIRE functionality. Can these tests remain here as we depend on parent test classes?

@groldan
Copy link
Member

groldan commented Apr 11, 2025

I see. @ConditionalOnBean can be tricky because there could be a race condition on the AutoConfiguration processing order.
Now, AutoConfiguration classes can be @Ordered too, and that'd be a good way to avoid unnecessary coupling between the extensions and the application.
Can you try adding @Order(Ordered.LOWEST_PRECEDENCE) to the auto configuration classes and see if that causes them to be processed after the beans referenced on the @ConditionalOnWFS etc?

@groldan
Copy link
Member

groldan commented Apr 11, 2025

btw I think it's great to have extension's integration tests in the apps, but make them standalone, not extending other tests (e.g. WcsApplicationInspireExtensionTest extends WcsApplicationTest, is there a reason for the inheritance?)

@groldan
Copy link
Member

groldan commented Apr 14, 2025

@buehner I've made it mandatory to include file headers and avoid star imports, see #651

@groldan
Copy link
Member

groldan commented Apr 14, 2025

@buehner I think I've fixed the @ConditionalOnGeoServer<service> annotations for good. See #656

@buehner
Copy link
Member Author

buehner commented Apr 24, 2025

btw I think it's great to have extension's integration tests in the apps, but make them standalone, not extending other tests (e.g. WcsApplicationInspireExtensionTest extends WcsApplicationTest, is there a reason for the inheritance?)

The reason is that the INSPIRE extension just adds some elements to Capabilities XMLs (WMS, WFS, WCS, WMTS), so I just wanted to re-use the existing test setups in that context.

@buehner buehner force-pushed the add-gs-inspire-extension branch from 1dbfe20 to 4e8968b Compare April 24, 2025 08:53
@buehner
Copy link
Member Author

buehner commented Apr 24, 2025

FYI I am still refactoring/creating in the test context. Will push force soon

@buehner buehner force-pushed the add-gs-inspire-extension branch 2 times, most recently from 0557b4f to 819029f Compare April 24, 2025 13:34
@buehner buehner force-pushed the add-gs-inspire-extension branch 3 times, most recently from 36b4133 to 568f8f5 Compare April 24, 2025 13:55
@buehner
Copy link
Member Author

buehner commented Apr 24, 2025

@groldan I think this is ready for another review. I rebased, resolved the conflicts and refactored tests to the src/extension package. But I also removed the inspire capabilities tests (discussed above) in src/apps. We can still address this in a follow up.

So this is a clean PR now, making changes to src/extensions and src/starters only.

@buehner buehner force-pushed the add-gs-inspire-extension branch from 568f8f5 to bca815c Compare April 24, 2025 14:03
Co-authored-by: Daniel Koch <koch@terrestris.de>
Co-authored-by: Andreas Schmitz <schmitz@terrestris.de>
Co-authored-by: Michael Holthausen <holthausen@terrestris.de>
@buehner buehner force-pushed the add-gs-inspire-extension branch from bca815c to 283d93f Compare April 24, 2025 14:29
@groldan groldan merged commit ca5dc3a into geoserver:main Apr 24, 2025
6 checks passed
@groldan groldan added the feature New feature label Apr 24, 2025
@groldan
Copy link
Member

groldan commented Apr 24, 2025

Hi @buehner, I had already release 2.27.0.0 when I saw your updates, so I reverted the release and merged this pr to be included in 2.27.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants