-
Notifications
You must be signed in to change notification settings - Fork 9
Wfs split #395
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
Wfs split #395
Conversation
|
Current state:
Last steps: (to know where to pick it up)
Next steps:
|
7c05611 to
a429044
Compare
a429044 to
6bf8023
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the WFS module by splitting it into three separate modules: wfs-core, wfs1_x, and wfs2_x. The changes separate version-specific WFS implementations from shared core functionality, improving modularity and maintainability.
Changes:
- Created new
wfs-coremodule containing shared WFS functionality across all versions - Created new
wfs1_xmodule for WFS 1.0 and 1.1 version-specific implementations - Updated all module dependencies throughout the codebase to reference the new artifact IDs
Reviewed changes
Copilot reviewed 86 out of 539 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/wfs1_x/src/main/java/org/geoserver/wfs/xml/v1_0_0/GetFeatureTypeBinding.java | Updated import to use new WFS1XSqlViewParamsHelper class |
| src/wfs1_x/src/main/java/org/geoserver/wfs/xml/WFS1XSqlViewParamsHelper.java | New helper class for WFS 1.x SQL view parameters handling |
| src/wfs1_x/src/main/java/org/geoserver/wfs/xml/GML3OutputFormat.java | New WFS 1.x specific GML3 output format implementation |
| src/wfs1_x/src/main/java/applicationContext.xml | New Spring configuration for WFS 1.x module |
| src/wfs1_x/pom.xml | New Maven POM for WFS 1.x module |
| src/wfs/src/test/java/org/geoserver/wfs/ExternalEntitiesTest.java | Test file removed from old location |
| src/wfs/src/main/java/applicationContext.xml | Configuration file removed from old location |
| src/wfs-core/src/test/java/org/geoserver/wfs/response/CSVOutputFormatTest.java | New test file for CSV output format |
| src/wfs-core/src/test/java/org/geoserver/wfs/WFSTestSupport.java | Updated test support class with simplified interface |
| src/wfs-core/src/test/java/org/geoserver/wfs/WFSServiceExceptionTestSupport.java | Refactored to abstract base class for exception testing |
| src/wfs-core/src/test/java/org/geoserver/wfs/ExternalEntitiesTest.java | Test file moved to new location |
| src/wfs-core/src/main/java/org/geoserver/wfs/xml/XmlSchemaEncoder.java | Moved to wfs-core with updated package and namespace handling |
| src/wfs-core/src/main/java/org/geoserver/wfs/xml/WFSXmlConfiguration.java | New interface for WFS XML configurations |
| src/wfs-core/src/main/java/org/geoserver/wfs/xml/SqlViewParamsExtractor.java | Removed version-specific method, kept shared functionality |
| src/wfs-core/src/main/java/org/geoserver/wfs/xml/BaseGML3OutputFormat.java | New abstract base class for GML3 output formats |
| src/wfs-core/src/main/java/org/geoserver/wfs/WFSConstants.java | New constants class for shared WFS values |
| src/wfs-core/src/main/java/org/geoserver/wfs/FeatureTypeUtils.java | New utility class for feature type operations |
| src/wfs-core/src/main/java/applicationContext.xml | New Spring configuration for wfs-core module |
| src/wfs-core/pom.xml | Updated Maven artifact ID and added test-jar goal |
| src/pom.xml | Updated to include new module references |
| Various pom.xml files | Updated dependencies from gs-wfs to gs-wfs-core, gs-wfs1_x, or gs-wfs2_x |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aaime
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. See comments below for minor issues found.
For the missing copyright headers please add this:
/* (c) 2026 Open Source Geospatial Foundation - all rights reserved
* This code is licensed under the GPL 2.0 license, available at the root
* application directory.
*/
Perform an update and then go straight to the public repository PR.
Please remember to create a Jira ticket for this, if it was not done already, and contribute as a single commit.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.geoserver</groupId> | ||
| <artifactId>gs-wfs1_x</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it's enough? I see mentions of wfs 2.0 requests too, e.g.:
https://github.com/geoserver/geoserver/blob/99ae245402fca51e306eaf9817faaa891bf89e73/src/community/features-templating/features-templating-ows/src/test/java/org/geoserver/featurestemplating/response/FlatGeoJSONComplexFeaturesResponseWFSTest.java#L52
There are also requests using 1.1 and 1.0, so what seems to be missing is 2.0.
At the same time, that would be a test dependency.
| <artifactId>gs-wfs-core</artifactId> | ||
| <version>${project.version}</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why, to be verified
| <dependency> | ||
| <groupId>org.geoserver</groupId> | ||
| <artifactId>gs-wfs</artifactId> | ||
| <artifactId>gs-wfs-core</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are tests that use WFS 1.X, which run only if the ogr2ogr utility is available in the path.
For the tests to run successfully, please add the following test dependency as well:
<dependency>
<groupId>org.geoserver</groupId>
<artifactId>gs-wfs1_x</artifactId>
<scope>test</scope>
</dependency>
| @@ -21,7 +21,8 @@ | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also needs a wfs 1.x dependency scoped as test
| <dependency> | ||
| <groupId>org.geoserver</groupId> | ||
| <artifactId>gs-wfs</artifactId> | ||
| <artifactId>gs-wfs-core</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also needs wfs 1.x (scoped as test dependency) to run tests
| @@ -0,0 +1,43 @@ | |||
| package org.geoserver.wfs.response; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs copyright header
| @@ -0,0 +1,23 @@ | |||
| package org.geoserver.wfs; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lacks copyright header
| @@ -0,0 +1,136 @@ | |||
| package org.geoserver.wfs; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs copyright header
| @@ -0,0 +1,124 @@ | |||
| package org.geoserver.wfs; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs copyright header
| @@ -1046,11 +1015,6 @@ public void testUserSuppliedDefaultNamespace() throws Exception { | |||
| + ")"); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why has it been removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, probably removed by accident
Checklist
mainbranch (backports managed later; ignore for branch specific issues).For core and extension modules:
[GEOS-XYZWV] Title of the Jira ticket.The PR will be merged when all the build checks are green (see automated QA checks), there is a code committer review, and the checklist has been fulfilled.