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.

Sign up for GitHub

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

Jump to bottom

add quoting to postgis raster driver #837

Merged
merged 3 commits into from
Aug 26, 2018
Merged

add quoting to postgis raster driver #837

merged 3 commits into from
Aug 26, 2018

Conversation

jef-n
Copy link
Contributor

@jef-n jef-n commented Aug 9, 2018

What does this PR do?

add quoting to the postgisraster driver - untested, here to trigger tests…

What are related issues/pull requests?

https://issues.qgis.org/issues/19583

Tasklist

  • ADD YOUR TASKS HERE
  • Add test case(s)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

Environment

Provide environment details, if relevant:

  • OS:
  • Compiler:

@jef-n
Revert "NAS: disable NAS_INDICATOR default to fix OSGeo#720"
7c7e86d 
This reverts commit 9ec6971.

Revert "Disable NAS tests as they are broken for now (refs OSGeo#720)"

This reverts commit 5a4d3f4.
rouault
rouault reviewed Aug 9, 2018
View reviewed changes
autotest/ogr/ogr_nas.py
@@ -342,9 +342,6 @@ def ogr_nas_5():
ogr_nas_4,
ogr_nas_5]

# NAS tests disabled for now
gdaltest_list = []
Copy link
Member

@rouault rouault Aug 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

this change is unrelated to the commit

rouault
rouault reviewed Aug 9, 2018
View reviewed changes
gdal/ogr/ogrsf_frmts/nas/ogrnasdriver.cpp
@@ -74,7 +74,11 @@ static int OGRNASDriverIdentify( GDALOpenInfo* poOpenInfo )
if( strstr(szPtr,"opengis.net/gml") == nullptr )
return FALSE;

char **papszIndicators = CSLTokenizeStringComplex(CPLGetConfigOption("NAS_INDICATOR", ""), ";", 0, 0 );
char **papszIndicators = CSLTokenizeStringComplex(
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change

rouault
rouault reviewed Aug 9, 2018
View reviewed changes
gdal/ogr/ogrsf_frmts/nas/drv_nas.html

<p>The former default of "NAS-Operationen;AAA-Fachschema;aaa.xsd;aaa-suite" was
removed and has now to be explicitly specified to enable detection.</p>
separated strings listed in the option <b>NAS_INDICATOR</b> (which defaults to
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change

@rouault
Copy link
Member

rouault commented Aug 9, 2018

oh, looking closer, I didn't see there was a separate commit for the change in NAS, so this is probably OK (although a bit confusing to be conflated in this PR)

@jef-n
Copy link
Contributor Author

jef-n commented Aug 10, 2018

Yes sorry. The NAS change is just one pending in my branch, that just reverts the recent changes back to how it was before (OGR_SKIP=NAS should be ok after all). With the PR I actually only wanted to trigger test - which miserably failed - although the failures seem unrelated to the changes.

@rouault
Copy link
Member

rouault commented Aug 10, 2018

I've restarted the failed jobs

rouault
rouault reviewed Aug 10, 2018
View reviewed changes
gdal/frmts/postgisraster/postgisrasterdataset.cpp Outdated
CPLString osColumnI(CPLQuotedSQLIdentifier(pszColumn));
CPLString osSrcColumnI(CPLQuotedSQLIdentifier(poSrcDS->pszSchema));
CPLString osSrcSchemaI(CPLQuotedSQLIdentifier(poSrcDS->pszTable));
CPLString osSrcTableI;(CPLQuotedSQLIdentifier(poSrcDS->pszColumn));
Copy link
Member

Choose a reason for hiding this comment

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

extraneous ; between osSrcTableI and CPLQuoted...

gdal/frmts/postgisraster/postgisrasterdataset.cpp Outdated
@@ -3526,7 +3553,7 @@ PostGISRasterDataset::CreateCopy( CPL_UNUSED const char * pszFilename,

osCommand.Printf("create index %s_%s_gist ON %s.%s USING gist "
"(st_convexhull(%s));", pszTable, pszColumn,
pszSchema, pszTable, pszColumn);
osSchemaI.c_str(), osTableI.c_str(), osColumnI.c_str());
Copy link
Member

Choose a reason for hiding this comment

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

the index name %s_%s_gist itself should be quoted

gdal/frmts/postgisraster/postgisrasterdataset.cpp Outdated
@@ -2104,12 +2120,12 @@ const char * pszValidConnectionString)
papszSubdatasets[2 * i] =
CPLStrdup(CPLSPrintf("SUBDATASET_%d_NAME=PG:%s schema=%s table=%s column=%s "
"where='%s = %s'", i+ 1, pszValidConnectionString,
pszSchema, pszTable, pszColumn, pszPrimaryKeyName,
pszSchema, pszTable, pszColumn, osPrimaryKeyNameI.c_str(),
Copy link
Member

Choose a reason for hiding this comment

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

pszSchema, pszTable, pszColumn should probably be quoted too, but I'm not sure the Open() parsing code is ready for it. Actually looking at autotest/gdrivers/postgisraster.py , it seems ready for single quoting, at least for table name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should work - Looks like what helped in qgis/QGIS@f4d08eb926

@rouault
Copy link
Member

rouault commented Aug 11, 2018

Looks good to me. I let you merge

@rouault rouault merged commit f5aa39d into OSGeo:master Aug 26, 2018
rouault added a commit that referenced this pull request Sep 15, 2018
@rouault
PostgisRaster: fix breakage of #837 (master only)
d903562
@qgib qgib mentioned this pull request May 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rouault rouault rouault left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jef-n @rouault