Skip to content

WFS PageResults operation review#2

Open
nmco wants to merge 1 commit intonsg-wfs-profilefrom
page-results-final-review
Open

WFS PageResults operation review#2
nmco wants to merge 1 commit intonsg-wfs-profilefrom
page-results-final-review

Conversation

@nmco
Copy link
Owner

@nmco nmco commented Oct 23, 2017

No description provided.

@nmco nmco changed the title WFs PageResults oepration review WFS PageResults operation review Oct 23, 2017
@aaime
Copy link

aaime commented Oct 23, 2017

I don't see a profile to add this module into the build (web/app/pom.xml and community/pom.xml)

Copy link

@aaime aaime left a comment

Choose a reason for hiding this comment

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

Did a first round of checks.

#. request page four and use the provided next URI to retrieve page five

This is not an ideal solution to access random pages, which is common action.
PageResults operation will improve this by allowing clients to request random pages directly.
Copy link

Choose a reason for hiding this comment

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

In GeoServer it actually adds nothing, as one can modify count and startindex as they want (assuming the response is not stored, that is), but it adds a standard way for the client to do the same without being trapped in vendor behavior.

:header-rows: 1

* - Name
- Mandotry
Copy link

Choose a reason for hiding this comment

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

Mandatory

#. outputFormat
#. resultType

and finally the GetFeature response is returned.
Copy link

Choose a reason for hiding this comment

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

I see UI and code apparently related to versioning that is not mentioned here. Should it be documented?

Resource resource = dd.get(MODULE_DIR + "/" + PROPERTY_FILENAME);
if (loader != null) {
File directory = loader.findOrCreateDirectory(MODULE_DIR);
File file = new File(directory, PROPERTY_FILENAME);
Copy link

Choose a reason for hiding this comment

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

The code should not be using File directly, is this a leftover?

/**
* Helper method that create a new table on DB to store resource informations
*/
private void createTable(DataStore dataStore, boolean forceDelete) throws Exception {
Copy link

Choose a reason for hiding this comment

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

createFeatureType maybe?

SimpleFeatureCollection toRemoved = store.getFeatures(filter);
// Remove file
Resource currentResource = indexConfiguration.getStorageResource();
SimpleFeatureIterator iterator = toRemoved.features();
Copy link

Choose a reason for hiding this comment

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

try with resources please, SimpleFeatureIterator is a Closeable

}
}
} catch (Throwable t) {
session.rollback();
Copy link

Choose a reason for hiding this comment

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

No exception report logged anywhere? Also, the way it's structured seems to indicate that a single non transient failure will block cleanup until the admin manages to resolve it. For something like a cleanup maybe better be tolerant to errors, log them, and move on instead?

Resource storageResource = this.indexConfiguration.getStorageResource();

// Serialize KVP parameters and the POST content
Map kvp = request.getKvp();
Copy link

Choose a reason for hiding this comment

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

Where is the POST content stored? This seems to only serialize GET requests properly. Which is a problem, the specification is memory serves me right tells that server have to implement POST, and may implement GET, meaning non hand written clients often use POST to get maximum compatibility, at least that was the case with older implementations, the WFS 2.0 does not define a preference it seems, says that a server can implement either GET or POST... but the GeoServer capabilities document does contain both.

<constructor-arg ref="geoServer"/>
<constructor-arg ref="indexConfiguration"/>
</bean>
<bean class="org.geoserver.platform.Service" id="wfsServicePageResults">
Copy link

Choose a reason for hiding this comment

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

Confused, how does this work? It acts as a separate service that triggers only when version 2.0.2 is used? But doesn't this prevent to have 2.0.2 implemented in core? (which eventually we might want to do, even if today it's not a problem, it declares version 2.0).

<constructor-arg index="1" value="http://www.opengis.net/wfs/2.0"/>
<constructor-arg index="2" ref="pageResultsTarget"/>
<constructor-arg index="3" value="2.0.2"/>
<constructor-arg index="4">
Copy link

Choose a reason for hiding this comment

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

Confused also here... so version 2.0.2 is only usable for PageResults, but won't support GetCapabilities (btw, don't you have to declare NSW support and the PagedResults operation in the capabilities document too?)

nmco pushed a commit that referenced this pull request Dec 5, 2018
* Add direction on building and running on windows

* Update base on comments

* Update based on comments #2

* Moved issue regarding wcs1_1 not building into general section.

* Update underline to match phrase length
nmco pushed a commit that referenced this pull request May 9, 2020
Add config for deployment to boundless-server repo to pom
nmco pushed a commit that referenced this pull request Apr 11, 2021
* [GEOS-9817] Add LayerGroup support to Geofence

* reviewer's suggestion applied

* review feedback #2
nmco pushed a commit that referenced this pull request Apr 12, 2021
* [GEOS-9817] Add LayerGroup support to Geofence

* reviewer's suggestion applied

* review feedback #2
nmco pushed a commit that referenced this pull request Jan 30, 2022
- delete of the logs in the cite test Makefile.
- README.md modified for the new steps.
- Documentation added with the manual and automated tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants