From 83e9eeb9c6f1d9e278af55a8b8ef610f1896ffb2 Mon Sep 17 00:00:00 2001 From: Bradley J Chambers Date: Fri, 16 May 2014 09:27:58 -0400 Subject: [PATCH 1/3] revert the explicitly deleted behavior of these functions, unavailable on MSVC2012 --- include/pdal/filters/Scaling.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pdal/filters/Scaling.hpp b/include/pdal/filters/Scaling.hpp index b2353c2085..e06b085da7 100644 --- a/include/pdal/filters/Scaling.hpp +++ b/include/pdal/filters/Scaling.hpp @@ -80,8 +80,8 @@ class PDAL_DLL Scaling: public Filter Scaling(Stage& prevStage, const Options&); - Scaling& operator=(const Scaling&) = delete; - Scaling(const Scaling&) = delete; + Scaling& operator=(const Scaling&); + Scaling(const Scaling&); static Options getDefaultOptions(); virtual void initialize(); From b9dfa798181720ebf886ee553911ae1be5f714df Mon Sep 17 00:00:00 2001 From: Pete Gadomski Date: Wed, 14 May 2014 09:25:11 -0600 Subject: [PATCH 2/3] Check for null pointers when using pg_query_once `pdal::drivers::pgpointcloud::pg_query_once` is C-ish -- it returns a raw `char *` from `strdup`. In our driver code, we were not checking that pointer against `NULL`, which lead to PDAL crashing when the target postgres database existed, but did not have the [`pointcloud` extension](https://github.com/pramsey/pointcloud) installed. This patch updates all uses of `pg_query_once` in the codebase with a check for pointer validity, throwing a `pdal_error` if the returned pointer is `NULL`. Note that other functions in `include/pdal/drivers/pgpointcloud/common.hpp` also return raw pointers, but their usage has not been checked or modified for this patch, due primarily to developer laziness but rationalized as commendable commit cleanliness/avoidance of mission creep. This patch also adds a test harness for the pgpointcloud drivers, which helped verify the correctness of the fix. Since the unit tests require a running postgres instance with a already-created database, the postgres unit tests are disabled by default. They can be enabled by enabling the dependent cmake option `WITH_PGPOINTCLOUD_TESTS`. The database connection parameters can then be configured with CMake variables. I think it would be better to allow configuration of the postgres test database with some sort of configuration file (so a change to the postgres test database configuration didn't require a rebuild), but leveraging the existing CMake configuration system was quicker than doing my own config parsing. Fixes #292. --- CMakeLists.txt | 10 ++ src/drivers/pgpointcloud/Reader.cpp | 4 + src/drivers/pgpointcloud/Writer.cpp | 16 ++ test/unit/CMakeLists.txt | 24 +++ .../pgpointcloud/PgpointcloudWriterTest.cpp | 156 ++++++++++++++++++ test/unit/drivers/pgpointcloud/Support.hpp.in | 63 +++++++ 6 files changed, 273 insertions(+) create mode 100644 test/unit/drivers/pgpointcloud/PgpointcloudWriterTest.cpp create mode 100644 test/unit/drivers/pgpointcloud/Support.hpp.in diff --git a/CMakeLists.txt b/CMakeLists.txt index 63f000b2fa..7f075f89cf 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -20,6 +20,9 @@ mark_as_advanced(CMAKE_VERBOSE_MAKEFILE) # Path to additional CMake modules set(CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake/modules" ${CMAKE_MODULE_PATH}) +# Provided modules +include(CMakeDependentOption) + #------------------------------------------------------------------------------ # PDAL general settings @@ -374,6 +377,13 @@ if (WITH_SQLITE) endif() option(WITH_PGPOINTCLOUD "Choose if PostgreSQL PointCloud support should be built" TRUE) +cmake_dependent_option( + WITH_PGPOINTCLOUD_TESTS + "Choose if PostgreSQL PointCloud tests should be built" + OFF + WITH_PGPOINTCLOUD + OFF + ) if (WITH_PGPOINTCLOUD) find_package(PostgreSQL) if (POSTGRESQL_FOUND) diff --git a/src/drivers/pgpointcloud/Reader.cpp b/src/drivers/pgpointcloud/Reader.cpp index eac8d840ea..c3fbe57360 100644 --- a/src/drivers/pgpointcloud/Reader.cpp +++ b/src/drivers/pgpointcloud/Reader.cpp @@ -275,6 +275,10 @@ pdal::Schema Reader::fetchSchema() const oss << "SELECT schema FROM pointcloud_formats WHERE pcid = " << pcid; char *xml_str = pg_query_once(m_session, oss.str()); + if (!xml_str) + { + throw pdal_error("Unable to fetch schema from `pointcloud_formats`"); + } std::string xml = std::string(xml_str); free(xml_str); diff --git a/src/drivers/pgpointcloud/Writer.cpp b/src/drivers/pgpointcloud/Writer.cpp index 60c2893efc..905a04bdb9 100644 --- a/src/drivers/pgpointcloud/Writer.cpp +++ b/src/drivers/pgpointcloud/Writer.cpp @@ -269,6 +269,10 @@ boost::uint32_t Writer::SetupSchema(Schema const& buffer_schema, boost::uint32_t { oss << "SELECT Count(pcid) FROM pointcloud_formats WHERE pcid = " << m_pcid; char *count_str = pg_query_once(m_session, oss.str()); + if (!count_str) + { + throw pdal_error("Unable to count pcid's in table `pointcloud_formats`"); + } schema_count = atoi(count_str); free(count_str); oss.str(""); @@ -285,6 +289,10 @@ boost::uint32_t Writer::SetupSchema(Schema const& buffer_schema, boost::uint32_t bool bCreatePCPointSchema = true; oss << "SELECT Count(pcid) FROM pointcloud_formats"; char *schema_count_str = pg_query_once(m_session, oss.str()); + if (!schema_count_str) + { + throw pdal_error("Unable to count pcid's in table `pointcloud_formats`"); + } schema_count = atoi(schema_count_str); free(schema_count_str); oss.str(""); @@ -321,6 +329,10 @@ boost::uint32_t Writer::SetupSchema(Schema const& buffer_schema, boost::uint32_t else { char *pcid_str = pg_query_once(m_session, "SELECT Max(pcid)+1 AS pcid FROM pointcloud_formats"); + if (!pcid_str) + { + throw pdal_error("Unable to get the max pcid from `pointcloud_formats`"); + } pcid = atoi(pcid_str); } @@ -417,6 +429,10 @@ bool Writer::CheckTableExists(std::string const& name) log()->get(logDEBUG) << "checking for table '" << name << "' existence ... " << std::endl; char *count_str = pg_query_once(m_session, oss.str()); + if (!count_str) + { + throw pdal_error("Unable to check for the existence of `pg_table`"); + } int count = atoi(count_str); free(count_str); diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt index 8ce974ec41..17813b73bd 100644 --- a/test/unit/CMakeLists.txt +++ b/test/unit/CMakeLists.txt @@ -166,6 +166,29 @@ if (WITH_HDF5) ENDFOREACH(file) endif(WITH_HDF5) +if (WITH_PGPOINTCLOUD AND WITH_PGPOINTCLOUD_TESTS) + set(PDAL_PGPOINTCLOUD_TEST_CPP + drivers/pgpointcloud/PgpointcloudWriterTest.cpp + ) + + set(PGPOINTCLOUD_TEST_DB_HOST localhost CACHE STRING "Postgres test database host") + set(PGPOINTCLOUD_TEST_DB_PORT 5432 CACHE STRING "Postgres test database port") + set(PGPOINTCLOUD_TEST_DB_NAME pdal_test CACHE STRING + "Postgres test database name, must exist and must be able to create databases") + set(PGPOINTCLOUD_TEST_DB_TEMPNAME pdal_test_tmp CACHE STRING "Postgres test database temp database name") + + configure_file( + drivers/pgpointcloud/Support.hpp.in + ${CMAKE_CURRENT_BINARY_DIR}/drivers/pgpointcloud/Support.hpp + ) + + set(PDAL_UNITTEST_TEST_SRC + ${PDAL_UNITTEST_TEST_SRC} + ${PDAL_PGPOINTCLOUD_TEST_CPP} + CACHE INTERNAL "source files for test" + ) +endif (WITH_PGPOINTCLOUD AND WITH_PGPOINTCLOUD_TESTS) + set(PDAL_UNITTEST_SOURCES "") FOREACH(file ${PDAL_UNITTEST_TEST_SRC}) SET(PDAL_UNITTEST_SOURCES "${PDAL_UNITTEST_SOURCES};${file}" @@ -194,6 +217,7 @@ source_group("Source Files\\config" FILES ${PDAL_UNITTEST_CONFIG_SRC}) INCLUDE_DIRECTORIES( . + ${CMAKE_CURRENT_BINARY_DIR} ../../include ${GDAL_INCLUDE_DIR} ${GEOTIFF_INCLUDE_DIR} diff --git a/test/unit/drivers/pgpointcloud/PgpointcloudWriterTest.cpp b/test/unit/drivers/pgpointcloud/PgpointcloudWriterTest.cpp new file mode 100644 index 0000000000..5fc2fe877d --- /dev/null +++ b/test/unit/drivers/pgpointcloud/PgpointcloudWriterTest.cpp @@ -0,0 +1,156 @@ +/****************************************************************************** +* Copyright (c) 2014, Pete Gadomski (pete.gadomski@gmail.com) +* +* All rights reserved. +* +* Redistribution and use in source and binary forms, with or without +* modification, are permitted provided that the following +* conditions are met: +* +* * Redistributions of source code must retain the above copyright +* notice, this list of conditions and the following disclaimer. +* * Redistributions in binary form must reproduce the above copyright +* notice, this list of conditions and the following disclaimer in +* the documentation and/or other materials provided +* with the distribution. +* * Neither the name of Hobu, Inc. or Flaxen Geo Consulting nor the +* names of its contributors may be used to endorse or promote +* products derived from this software without specific prior +* written permission. +* +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS +* FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE +* COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, +* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, +* BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS +* OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED +* AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY +* OF SUCH DAMAGE. +****************************************************************************/ + +#include + +#include +#include + +#include "Support.hpp" +#include "drivers/pgpointcloud/Support.hpp" + + +pdal::Options getWriterOptions() +{ + pdal::Options options; + + options.add(pdal::Option("connection", pdal::drivers::pgpointcloud::testDbTempConn)); + options.add(pdal::Option("table", "pdal_test_table")); + options.add(pdal::Option("srid", "4326")); + options.add(pdal::Option("capacity", "10000")); + + return options; +} + +struct PgpointcloudWriterTestFixture +{ + PgpointcloudWriterTestFixture() + : m_masterConnection( + pdal::drivers::pgpointcloud::pg_connect( + pdal::drivers::pgpointcloud::testDbConn)) + , m_testConnection(NULL) + { + // Silence those pesky notices + executeOnMasterDb("SET client_min_messages TO WARNING"); + + dropTestDb(); + + std::stringstream createDbSql; + createDbSql << "CREATE DATABASE " << + pdal::drivers::pgpointcloud::testDbTempname << " TEMPLATE template0"; + executeOnMasterDb(createDbSql.str()); + m_testConnection = pdal::drivers::pgpointcloud::pg_connect( + pdal::drivers::pgpointcloud::testDbTempConn); + + executeOnTestDb("CREATE EXTENSION pointcloud"); + } + + void executeOnTestDb(const std::string& sql) + { + pdal::drivers::pgpointcloud::pg_execute(m_testConnection, sql); + } + + ~PgpointcloudWriterTestFixture() + { + if (m_testConnection) + { + PQfinish(m_testConnection); + } + dropTestDb(); + if (m_masterConnection) + { + PQfinish(m_masterConnection); + } + } + +private: + + void executeOnMasterDb(const std::string& sql) + { + pdal::drivers::pgpointcloud::pg_execute(m_masterConnection, sql); + } + + void execute(PGconn* connection, const std::string& sql) + { + if (connection) + { + pdal::drivers::pgpointcloud::pg_execute(connection, sql); + } + else + { + throw std::runtime_error("Not connected to database for testing"); + } + } + + void dropTestDb() + { + std::stringstream dropDbSql; + dropDbSql << "DROP DATABASE IF EXISTS " << pdal::drivers::pgpointcloud::testDbTempname; + executeOnMasterDb(dropDbSql.str()); + } + + PGconn* m_masterConnection; + PGconn* m_testConnection; + +}; + +BOOST_FIXTURE_TEST_SUITE(PgpointcloudWriterTest, PgpointcloudWriterTestFixture) + + +BOOST_AUTO_TEST_CASE(testWrite) +{ + pdal::drivers::las::Reader reader(Support::datapath("1.2-with-color.las")); + pdal::drivers::pgpointcloud::Writer writer(reader, getWriterOptions()); + + writer.initialize(); + + boost::uint64_t numPointsWritten = writer.write(0); + BOOST_CHECK_EQUAL(numPointsWritten, 1065); +} + + +BOOST_AUTO_TEST_CASE(testNoPointcloudExtension) +{ + executeOnTestDb("DROP EXTENSION pointcloud"); + + pdal::drivers::las::Reader reader(Support::datapath("1.2-with-color.las")); + pdal::drivers::pgpointcloud::Writer writer(reader, getWriterOptions()); + + writer.initialize(); + + BOOST_CHECK_THROW(writer.write(0), pdal::pdal_error); +} + + +BOOST_AUTO_TEST_SUITE_END() diff --git a/test/unit/drivers/pgpointcloud/Support.hpp.in b/test/unit/drivers/pgpointcloud/Support.hpp.in new file mode 100644 index 0000000000..54376dd40b --- /dev/null +++ b/test/unit/drivers/pgpointcloud/Support.hpp.in @@ -0,0 +1,63 @@ +/****************************************************************************** +* Copyright (c) 2014, Peter J. Gadomski (pete.gadomski@gmail.com) +* +* All rights reserved. +* +* Redistribution and use in source and binary forms, with or without +* modification, are permitted provided that the following +* conditions are met: +* +* * Redistributions of source code must retain the above copyright +* notice, this list of conditions and the following disclaimer. +* * Redistributions in binary form must reproduce the above copyright +* notice, this list of conditions and the following disclaimer in +* the documentation and/or other materials provided +* with the distribution. +* * Neither the name of Hobu, Inc. or Flaxen Geo Consulting nor the +* names of its contributors may be used to endorse or promote +* products derived from this software without specific prior +* written permission. +* +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS +* FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE +* COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, +* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, +* BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS +* OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED +* AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY +* OF SUCH DAMAGE. +****************************************************************************/ + +#ifndef UNITTEST_DRIVERS_PGPOINTCLOUD_SUPPORT_INCLUDED +#define UNITTEST_DRIVERS_PGPOINTCLOUD_SUPPORT_INCLUDED + + +namespace pdal +{ +namespace drivers +{ +namespace pgpointcloud +{ + + +static const std::string testDbHost("@PGPOINTCLOUD_TEST_DB_HOST@"); +static const std::string testDbPort("@PGPOINTCLOUD_TEST_DB_PORT@"); +static const std::string testDbName("@PGPOINTCLOUD_TEST_DB_NAME@"); +static const std::string testDbTempname("@PGPOINTCLOUD_TEST_DB_TEMPNAME@"); + +static const std::string testDbConn(std::string("dbname='@PGPOINTCLOUD_TEST_DB_NAME@' ") + + "host='@PGPOINTCLOUD_TEST_DB_HOST@' port='@PGPOINTCLOUD_TEST_DB_PORT@'"); +static const std::string testDbTempConn(std::string("dbname='@PGPOINTCLOUD_TEST_DB_TEMPNAME@'") + + "host='@PGPOINTCLOUD_TEST_DB_HOST@' port='@PGPOINTCLOUD_TEST_DB_PORT@'"); + + +} +} +} // namespace pdal::drivers::pgpointcloud + + +#endif // UNITTEST_DRIVERS_PGPOINTCLOUD_SUPPORT_INCLUDED From 5a3eebf289c17057869ba276953a0ad45e98bcb9 Mon Sep 17 00:00:00 2001 From: Pete Gadomski Date: Mon, 19 May 2014 15:29:49 -0600 Subject: [PATCH 3/3] Remove unused variable m_pclblockFilter clang-503.0.40 was complaining about an unused member variable in `pdal::filters::iterators::sequential::PCLBlock`. --- include/pdal/filters/PCLBlock.hpp | 2 -- src/filters/PCLBlock.cpp | 1 - 2 files changed, 3 deletions(-) diff --git a/include/pdal/filters/PCLBlock.hpp b/include/pdal/filters/PCLBlock.hpp index 5466f53e6e..b8d303b402 100644 --- a/include/pdal/filters/PCLBlock.hpp +++ b/include/pdal/filters/PCLBlock.hpp @@ -104,8 +104,6 @@ class PDAL_DLL PCLBlock : public pdal::FilterSequentialIterator boost::uint64_t skipImpl(boost::uint64_t); boost::uint32_t readBufferImpl(PointBuffer&); bool atEndImpl() const; - - const pdal::filters::PCLBlock& m_pclblockFilter; }; diff --git a/src/filters/PCLBlock.cpp b/src/filters/PCLBlock.cpp index d607de47d7..e54f6ec225 100644 --- a/src/filters/PCLBlock.cpp +++ b/src/filters/PCLBlock.cpp @@ -185,7 +185,6 @@ namespace sequential PCLBlock::PCLBlock(const pdal::filters::PCLBlock& filter, PointBuffer& buffer) : pdal::FilterSequentialIterator(filter, buffer) - , m_pclblockFilter(filter) { return; }