Skip to content

Commit

Permalink
Merge pull request #10334 from rouault/fix_10332
Browse files Browse the repository at this point in the history 
GML writer: fix missing SRS in Featurecollection's boundedBy element (3.9.1 regression)
  • Loading branch information
@rouault
rouault authored Jul 6, 2024
2 parents 91b25d6 + 367c0bb commit 7c612da
Show file tree
Hide file tree
Showing 4 changed files with 161 additions and 71 deletions.
92 changes: 92 additions & 0 deletions autotest/ogr/ogr_gml.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4356,3 +4356,95 @@ def test_ogr_gml_geom_link_to_immediate_child():
"data/gml/link_to_immediate_child.gml", open_options=["WRITE_GFS=NO"]
)
assert ds


###############################################################################
# Test scenario of https://github.com/OSGeo/gdal/issues/10332


@pytest.mark.parametrize("use_create_geom_field", [False, True])
@pytest.mark.parametrize("has_srs", [False, True])
def test_ogr_gml_ogr2ogr_from_layer_with_name_geom_field(
tmp_vsimem, use_create_geom_field, has_srs
):

ds = gdal.GetDriverByName("Memory").Create("", 0, 0, 0, gdal.GDT_Unknown)
if has_srs:
srs = osr.SpatialReference()
srs.ImportFromEPSG(4326)
srs.SetAxisMappingStrategy(osr.OAMS_TRADITIONAL_GIS_ORDER)
else:
srs = None
if use_create_geom_field:
lyr = ds.CreateLayer("test", geom_type=ogr.wkbNone)
my_geom_field = ogr.GeomFieldDefn("my_geom", ogr.wkbUnknown)
my_geom_field.SetSpatialRef(srs)
lyr.CreateGeomField(my_geom_field)
else:
lyr = ds.CreateLayer("test", geom_type=ogr.wkbUnknown, srs=srs)
f = ogr.Feature(lyr.GetLayerDefn())
f.SetGeometryDirectly(ogr.CreateGeometryFromWkt("POINT(2 49)"))
lyr.CreateFeature(f)
f = ogr.Feature(lyr.GetLayerDefn())
f.SetGeometryDirectly(ogr.CreateGeometryFromWkt("POINT(3 50)"))
lyr.CreateFeature(f)

out_filename = str(tmp_vsimem / "out.gml")
gdal.VectorTranslate(out_filename, ds, format="GML")

f = gdal.VSIFOpenL(out_filename, "rb")
assert f
try:
data = gdal.VSIFReadL(1, 10000, f)
finally:
gdal.VSIFCloseL(f)

if has_srs:
assert (
b'<gml:boundedBy><gml:Envelope srsName="urn:ogc:def:crs:EPSG::4326"><gml:lowerCorner>49 2</gml:lowerCorner><gml:upperCorner>50 3</gml:upperCorner></gml:Envelope></gml:boundedBy>'
in data
)
else:
assert (
b"<gml:boundedBy><gml:Envelope><gml:lowerCorner>2 49</gml:lowerCorner><gml:upperCorner>3 50</gml:upperCorner></gml:Envelope></gml:boundedBy>"
in data
)


###############################################################################


@pytest.mark.parametrize("first_layer_has_srs", [False, True])
def test_ogr_gml_ogr2ogr_from_layers_with_inconsistent_srs(
tmp_vsimem, first_layer_has_srs
):

ds = gdal.GetDriverByName("Memory").Create("", 0, 0, 0, gdal.GDT_Unknown)
srs = osr.SpatialReference()
srs.ImportFromEPSG(4326)
srs.SetAxisMappingStrategy(osr.OAMS_TRADITIONAL_GIS_ORDER)
lyr = ds.CreateLayer(
"test", geom_type=ogr.wkbUnknown, srs=(srs if first_layer_has_srs else None)
)
f = ogr.Feature(lyr.GetLayerDefn())
f.SetGeometryDirectly(ogr.CreateGeometryFromWkt("POINT(2 49)"))
lyr.CreateFeature(f)

lyr = ds.CreateLayer(
"test2", geom_type=ogr.wkbUnknown, srs=(None if first_layer_has_srs else srs)
)
f = ogr.Feature(lyr.GetLayerDefn())
f.SetGeometryDirectly(ogr.CreateGeometryFromWkt("POINT(3 50)"))
lyr.CreateFeature(f)

out_filename = str(tmp_vsimem / "out.gml")
gdal.VectorTranslate(out_filename, ds, format="GML")

f = gdal.VSIFOpenL(out_filename, "rb")
assert f
try:
data = gdal.VSIFReadL(1, 10000, f)
finally:
gdal.VSIFCloseL(f)

assert b"<gml:boundedBy><gml:Null /></gml:boundedBy>" in data
18 changes: 15 additions & 3 deletions ogr/ogrsf_frmts/gml/ogr_gml.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ class OGRGMLLayer final : public OGRLayer
char *pszFIDPrefix;

bool bWriter;
bool bSameSRS;

OGRGMLDataSource *poDS;

Expand Down Expand Up @@ -137,8 +136,14 @@ class OGRGMLDataSource final : public OGRDataSource
OGRGMLSRSNameFormat eSRSNameFormat;
bool bWriteSpaceIndentation;

OGRSpatialReference *poWriteGlobalSRS;
bool bWriteGlobalSRS;
//! Whether all geometry fields of all layers have the same SRS (or no SRS at all)
bool m_bWriteGlobalSRS = true;

//! The global SRS (may be null), that is valid only if m_bWriteGlobalSRS == true
std::unique_ptr<OGRSpatialReference> m_poWriteGlobalSRS{};

//! Whether at least one geometry field has been created
bool m_bWriteGlobalSRSInit = false;

// input related parameters.
CPLString osFilename;
Expand Down Expand Up @@ -300,6 +305,13 @@ class OGRGMLDataSource final : public OGRDataSource
const char *GetSRSDimensionLoc() const;
bool GMLFeatureCollection() const;

void DeclareNewWriteSRS(const OGRSpatialReference *poSRS);

bool HasWriteGlobalSRS() const
{
return m_bWriteGlobalSRS;
}

virtual OGRLayer *ExecuteSQL(const char *pszSQLCommand,
OGRGeometry *poSpatialFilter,
const char *pszDialect) override;
Expand Down
88 changes: 50 additions & 38 deletions ogr/ogrsf_frmts/gml/ogrgmldatasource.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,8 @@ OGRGMLDataSource::OGRGMLDataSource()
nBoundedByLocation(-1), nSchemaInsertLocation(-1), bIsOutputGML3(false),
bIsOutputGML3Deegree(false), bIsOutputGML32(false),
eSRSNameFormat(SRSNAME_SHORT), bWriteSpaceIndentation(true),
poWriteGlobalSRS(nullptr), bWriteGlobalSRS(false), poReader(nullptr),
bOutIsTempFile(false), bExposeGMLId(false), bExposeFid(false),
bIsWFS(false), bUseGlobalSRSName(false),
poReader(nullptr), bOutIsTempFile(false), bExposeGMLId(false),
bExposeFid(false), bIsWFS(false), bUseGlobalSRSName(false),
m_bInvertAxisOrderIfLatLong(false), m_bConsiderEPSGAsURN(false),
m_eSwapCoordinates(GML_SWAP_AUTO), m_bGetSecondaryGeometryOption(false),
eReadMode(STANDARD), poStoredGMLFeature(nullptr),
Expand Down Expand Up @@ -126,13 +125,13 @@ OGRGMLDataSource::~OGRGMLDataSource()
if (!bFpOutputIsNonSeekable && nBoundedByLocation != -1 &&
VSIFSeekL(fpOutput, nBoundedByLocation, SEEK_SET) == 0)
{
if (bWriteGlobalSRS && sBoundingRect.IsInit() && IsGML3Output())
if (m_bWriteGlobalSRS && sBoundingRect.IsInit() && IsGML3Output())
{
bool bCoordSwap = false;
char *pszSRSName =
poWriteGlobalSRS
? GML_GetSRSName(poWriteGlobalSRS, eSRSNameFormat,
&bCoordSwap)
m_poWriteGlobalSRS
? GML_GetSRSName(m_poWriteGlobalSRS.get(),
eSRSNameFormat, &bCoordSwap)
: CPLStrdup("");
char szLowerCorner[75] = {};
char szUpperCorner[75] = {};
Expand Down Expand Up @@ -201,7 +200,7 @@ OGRGMLDataSource::~OGRGMLDataSource()
szLowerCorner, szUpperCorner);
CPLFree(pszSRSName);
}
else if (bWriteGlobalSRS && sBoundingRect.IsInit())
else if (m_bWriteGlobalSRS && sBoundingRect.IsInit())
{
if (bWriteSpaceIndentation)
VSIFPrintfL(fpOutput, " ");
Expand Down Expand Up @@ -268,8 +267,6 @@ OGRGMLDataSource::~OGRGMLDataSource()
delete poReader;
}

delete poWriteGlobalSRS;

delete poStoredGMLFeature;

if (osXSDFilename.compare(CPLSPrintf("/vsimem/tmp_gml_xsd_%p.xsd", this)) ==
Expand Down Expand Up @@ -2001,6 +1998,48 @@ void OGRGMLDataSource::WriteTopElements()
}
}

/************************************************************************/
/* DeclareNewWriteSRS() */
/************************************************************************/

// Check that all SRS passed to ICreateLayer() and CreateGeomField()
// are the same (or all null)

void OGRGMLDataSource::DeclareNewWriteSRS(const OGRSpatialReference *poSRS)
{
if (m_bWriteGlobalSRS)
{
if (!m_bWriteGlobalSRSInit)
{
m_bWriteGlobalSRSInit = true;
if (poSRS)
{
m_poWriteGlobalSRS.reset(poSRS->Clone());
m_poWriteGlobalSRS->SetAxisMappingStrategy(
OAMS_TRADITIONAL_GIS_ORDER);
}
}
else
{
if (m_poWriteGlobalSRS)
{
const char *const apszOptions[] = {
"IGNORE_DATA_AXIS_TO_SRS_AXIS_MAPPING=YES", nullptr};
if (!poSRS ||
!poSRS->IsSame(m_poWriteGlobalSRS.get(), apszOptions))
{
m_bWriteGlobalSRS = false;
}
}
else
{
if (poSRS)
m_bWriteGlobalSRS = false;
}
}
}
}

/************************************************************************/
/* ICreateLayer() */
/************************************************************************/
Expand Down Expand Up @@ -2037,37 +2076,9 @@ OGRGMLDataSource::ICreateLayer(const char *pszLayerName,
pszLayerName, pszCleanLayerName);
}

// Set or check validity of global SRS.
if (nLayers == 0)
{
WriteTopElements();
if (poSRS)
{
poWriteGlobalSRS = poSRS->Clone();
poWriteGlobalSRS->SetAxisMappingStrategy(
OAMS_TRADITIONAL_GIS_ORDER);
}
bWriteGlobalSRS = true;
}
else if (bWriteGlobalSRS)
{
if (poWriteGlobalSRS != nullptr)
{
const char *const apszOptions[] = {
"IGNORE_DATA_AXIS_TO_SRS_AXIS_MAPPING=YES", nullptr};
if (poSRS == nullptr ||
!poSRS->IsSame(poWriteGlobalSRS, apszOptions))
{
delete poWriteGlobalSRS;
poWriteGlobalSRS = nullptr;
bWriteGlobalSRS = false;
}
}
else
{
if (poSRS != nullptr)
bWriteGlobalSRS = false;
}
}

// Create the layer object.
Expand All @@ -2081,6 +2092,7 @@ OGRGMLDataSource::ICreateLayer(const char *pszLayerName,
pszGeomFieldName = "geometryProperty";
poGeomFieldDefn->SetName(pszGeomFieldName);
poGeomFieldDefn->SetNullable(poSrcGeomFieldDefn->IsNullable());
DeclareNewWriteSRS(poSRS);
if (poSRS != nullptr)
{
auto poSRSClone = poSRS->Clone();
Expand Down
34 changes: 4 additions & 30 deletions ogr/ogrsf_frmts/gml/ogrgmllayer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ OGRGMLLayer::OGRGMLLayer(const char *pszName, bool bWriterIn,
: poFeatureDefn(new OGRFeatureDefn(
pszName + (STARTS_WITH_CI(pszName, "ogr:") ? 4 : 0))),
iNextGMLId(0), bInvalidFIDFound(false), pszFIDPrefix(nullptr),
bWriter(bWriterIn), bSameSRS(false), poDS(poDSIn),
bWriter(bWriterIn), poDS(poDSIn),
poFClass(!bWriter ? poDS->GetReader()->GetClass(pszName) : nullptr),
// Reader's should get the corresponding GMLFeatureClass and cache it.
hCacheSRS(GML_BuildOGRGeometryFromList_CreateCache()),
Expand Down Expand Up @@ -708,33 +708,6 @@ OGRErr OGRGMLLayer::ICreateFeature(OGRFeature *poFeature)
poDS->PrintLine(fp, "<gml:featureMember>");
}

if (iNextGMLId == 0)
{
bSameSRS = true;
for (int iGeomField = 1;
iGeomField < poFeatureDefn->GetGeomFieldCount(); iGeomField++)
{
OGRGeomFieldDefn *poFieldDefn0 = poFeatureDefn->GetGeomFieldDefn(0);
OGRGeomFieldDefn *poFieldDefn =
poFeatureDefn->GetGeomFieldDefn(iGeomField);
const OGRSpatialReference *poSRS0 = poFieldDefn0->GetSpatialRef();
const OGRSpatialReference *poSRS = poFieldDefn->GetSpatialRef();
if (poSRS0 != nullptr && poSRS == nullptr)
{
bSameSRS = false;
}
else if (poSRS0 == nullptr && poSRS != nullptr)
{
bSameSRS = false;
}
else if (poSRS0 != nullptr && poSRS != nullptr && poSRS0 != poSRS &&
!poSRS0->IsSame(poSRS))
{
bSameSRS = false;
}
}
}

if (poFeature->GetFID() == OGRNullFID)
poFeature->SetFID(iNextGMLId++);

Expand Down Expand Up @@ -794,7 +767,7 @@ OGRErr OGRGMLLayer::ICreateFeature(OGRFeature *poFeature)
const int nCoordDimension = poGeom->getCoordinateDimension();

poGeom->getEnvelope(&sGeomBounds);
if (bSameSRS)
if (poDS->HasWriteGlobalSRS())
poDS->GrowExtents(&sGeomBounds, nCoordDimension);

if (poGeom->getSpatialReference() == nullptr &&
Expand Down Expand Up @@ -1246,7 +1219,8 @@ OGRErr OGRGMLLayer::CreateGeomField(const OGRGeomFieldDefn *poField,
/* Enforce XML naming semantics on element name. */
/* -------------------------------------------------------------------- */
OGRGeomFieldDefn oCleanCopy(poField);
auto poSRSOri = poField->GetSpatialRef();
const auto poSRSOri = poField->GetSpatialRef();
poDS->DeclareNewWriteSRS(poSRSOri);
if (poSRSOri)
{
auto poSRS = poSRSOri->Clone();
Expand Down

0 comments on commit 7c612da

Please sign in to comment.