Skip to content
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

Enhance bbox requests without geometry property to consider all geometry properties (3.6) #1732

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lgoltz
Copy link
Contributor

@lgoltz lgoltz commented Aug 13, 2024

Backport #1730

@tfr42 tfr42 added the enhancement enhancement or improvement label Aug 14, 2024
@tfr42 tfr42 added this to the 3.6 milestone Aug 14, 2024
@copierrj
Copy link
Member

copierrj commented Oct 9, 2024

The TMC discussed this PR and we have a few concerns:

This PR changes the behavior of deegree in a non-obvious (= to the user) way and we're not sure if that's appropriate (especially in case of the 3.5 backport) without proper documentation or even making the new behavior opt-in.

We also discussed the use of ST_Union in the PostGIS dialect and we are very concerned that the proposed solution results in unexpected performance regressions. Calculating ST_Union can be very expensive on large complex geometries. There are many ways to preventing the use of this (potentially) expensive union, such as using ST_Collect instead, rewrite the query generator to compute extents for every column first and aggregate them later, etc.

Another thing that came up during the discussion was what happens when someone is mixing different coordinate systems and/or geometry columns that can contain null values..

@tfr42 tfr42 added the needs discussion requires discussion with contributor label Oct 9, 2024
@lgoltz lgoltz force-pushed the feature/multipleGeomProperties-9885-32-3.6 branch from 2384006 to e3cb97c Compare October 24, 2024 07:30
@lgoltz
Copy link
Contributor Author

lgoltz commented Oct 24, 2024

Force push was required to remove a commit from PR #1729 / #1731 from this PR!
The comment #1732 (comment) regarding ST_Union is relevant for #1729 / #1731.

@dstenger
Copy link
Contributor

dstenger commented Nov 6, 2024

We consider the approach that all geometries are used when no geometry property is defined by the request as more intuitive and user friendly.

Note: The OGC specifications seem not to be clear about this. E.g. the OGC OWS Common 1.1.0 spec (06-121r3) describes the use of Bounding Boxes in chap. 10.2.5 (which is referenced by OGC WFS 2.0 spec), where OGC API Features 1.0.1 spec (17-069r4) allows both behaviors (see Requirement 24 /req/core/fc-bbox-response section B).

These are our reasons:

  • There is no way for users to find out which geometry property is used if multiple geometries are provided. E.g. the capabilities provide no information about prioritization of geometry properties. So, the users can only guess how the service behaves. Thus, it is more intuitive and user friendly to consider all geometry properties.
  • INSPIRE schemas (e.g. Building 3D) define feature types with multiple geometries. As deegree claims to fully support INSPIRE, the handling of INSPIRE data shall be as hassle free as possible.

Note: This change of behavior will mainly effect GetFeature GET KVP requests as POST XML requests define the used properties. Thus, most requests will not be affected by this change.

@tfr42 tfr42 added the breaking breaking change label Mar 12, 2025
@stephanr
Copy link
Member

Note: This change of behavior will mainly effect GetFeature GET KVP requests as POST XML requests define the used properties. Thus, most requests will not be affected by this change.

After revisiting this, I checked the OGC specification for Feature-Encoding 1.1/2.0 and for my understanding, post requests are not required (schema wise) to specify the geometry column in all cases. So this is only partially correct as BBOX type request, which should be pretty common, are not requiring a geometry column.

My understanding of the specification is, it is not requiring the service to include all geometry columns, as the specification uses the wording "shall" instead of "must".
Therefore, the change would better align deegree with the specification, but also GET and POST are changing behavior for the BBOX only case.

Reference example:
POST http://demo.deegree.org/inspire-workspace/services

<wfs:GetFeature
        xmlns:wfs="http://www.opengis.net/wfs"
        xmlns:ogc="http://www.opengis.net/ogc"
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xmlns:ad="urn:x-inspire:specification:gmlas:Addresses:3.0"
        xmlns:gml="http://www.opengis.net/gml"
        xsi:schemaLocation="http://www.opengis.net/wfs https://schemas.opengis.net/wfs/1.1.0/wfs.xsd"
        version="1.1.0"
        service="WFS"
        outputFormat="text/xml; subtype=gml/3.2.1">
    <wfs:Query typeName="ad:Address">
        <ogc:Filter>
            <ogc:BBOX>
                <gml:Envelope srsName="urn:ogc:def:crs:EPSG::4258">
                    <gml:lowerCorner>52.690 5.244</gml:lowerCorner>
                    <gml:upperCorner>52.692 5.245</gml:upperCorner>
                </gml:Envelope>
            </ogc:BBOX>
        </ogc:Filter>
    </wfs:Query>
</wfs:GetFeature>

Schema Fragments from the OGC specifications:

<!-- OGC 04-095 OpenGIS® Filter Encoding Implementation Specification -->
<xsd:complexType name="BBOXType">
    <xsd:complexContent>
        <xsd:extension base="ogc:SpatialOpsType">
            <xsd:sequence>
                <xsd:element ref="ogc:PropertyName" minOccurs="0" />
                <xsd:element ref="gml:Envelope" />
            </xsd:sequence>
        </xsd:extension>
    </xsd:complexContent>
</xsd:complexType>

<!-- OGC 09-026r1 OpenGIS Filter Encoding 2.0 Encoding Standard -->
<xsd:complexType name="BBOXType">
    <xsd:complexContent>
        <xsd:extension base="fes:SpatialOpsType">
            <xsd:sequence>
                <xsd:element ref="fes:expression" minOccurs="0" />
                <xsd:any namespace="##other" />
            </xsd:sequence>
        </xsd:extension>
    </xsd:complexContent>
</xsd:complexType>

@stephanr
Copy link
Member

Regarding the topic of possible influence of SQL runtime, as discussed in yesterdays TMC meeting
I had an exchange internally, and we came to the conclusion that this will most likely create issues with existing setups having a change in behavior of deegree (first geometry vs. all geometries).

The point, we discussed was, that there are multiple installations where deegree serves tables that contain more than one geometry columns where the initial setup only focused on/considered the first geometry.

As in the meeting, the question came up which scenarios could be influenced, these came up:

  1. The first geometry column is mapped, the secondary (and possible more) column are in the same CRS and are not indexed
    • On Query the Database (PostgreSQL / Oracle / Other) has to do a full table scan as no indexes can be used
    • This effect will worsen as the dataset grows, so probably large datasets (millions of records) are affected most
  2. The first geometry column is mapped, the secondary (and possible more) column are in a different CRS
    • Additional Coordinate transformations are required to query the dataset. I haven't checked the code if this is done by the query builder on the deegree side, or it is given to the database to be calculated on demand in the database (performance on large dataset).
    • If the secondary geometry was never explicitly queried before, and if the data was only simply written to output, that could result in coordinate transformation issues if deegree or the database could not transform CRS
    • I haven't checked if the secondary geometries are configured without srs/srid (-1) or auto/locale CRS could be an issue
  3. Behavior in Gazetteer applications where Web-Clients use WFS as Search / Geocoder service
    • This was a common use case reported to me regarding simple BBox GET/POST request.

I got the feedback that for this change, an option to either configure it on the dataset, where the geometries are mapped, or a way to opt-out (to the old behavior) would be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking breaking change enhancement enhancement or improvement needs discussion requires discussion with contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants