Property names carry over between layers causing query errors #1325

Closed
tnightingale opened this Issue Jul 18, 2012 · 6 comments

Projects

None yet

4 participants

@tnightingale

The python method mapnik.render_layer() is mapped to: render_layer_for_grid() (bindings/python/python_grid_utils.hpp)
which contains the following code:

    // convert python list to std::vector
    boost::python::ssize_t num_fields = boost::python::len(fields);
    for(boost::python::ssize_t i=0; i<num_fields; i++) {
        boost::python::extract<std::string> name(fields[i]);
        if (name.check()) {
            grid.add_property_name(name());
        }
        else
        {
          std::stringstream s;
          s << "list of field names must be strings";
          throw mapnik::value_error(s.str());    
        }
    }

    // copy property names
    std::set<std::string> attributes = grid.property_names();

From my understanding, the intention here is to validate the field names passed via a python list and store them in a std::set to be passed on to the grid renderer.

However when mapnik.render_layer()gets called subsequent times (to render different layers), the collection returned by grid.property_names()contains all the field names from the previous calls along with the new field names.

This is problematic as the incorrect field names are then used to query the datasources.

@springmeyer
Member

Good catch, thanks for the report. I'll take a closer look at this in the next couple days.

@tnightingale

Cool, it might be that this is by design and I'm misunderstanding something.
For the record, I encountered this when investigating these TileStache features/issues: TileStache/TileStache#77 TileStache/TileStache#59

@lightmare
Contributor

Could this work?

diff --git a/bindings/python/python_grid_utils.hpp b/bindings/python/python_grid_utils.hpp
index 62d83a9..cee4b0e 100644
--- a/bindings/python/python_grid_utils.hpp
+++ b/bindings/python/python_grid_utils.hpp
@@ -368,12 +368,14 @@ static void render_layer_for_grid(const mapnik::Map& map,
         throw std::runtime_error(s.str());
     }

-    // convert python list to std::vector
+    // convert python list to std::set
+    std::set<std::string> attributes;
     boost::python::ssize_t num_fields = boost::python::len(fields);
     for(boost::python::ssize_t i=0; i<num_fields; i++) {
         boost::python::extract<std::string> name(fields[i]);
         if (name.check()) {
             grid.add_property_name(name());
+            attributes.insert(name());
         }
         else
         {
@@ -383,8 +385,6 @@ static void render_layer_for_grid(const mapnik::Map& map,
         }
     }

-    // copy property names
-    std::set<std::string> attributes = grid.property_names();
     std::string const& key = grid.get_key();

     // if key is special __id__ keyword
@@ -394,13 +394,10 @@ static void render_layer_for_grid(const mapnik::Map& map,

         // if __id__ is requested to be dumped out
         // remove it so that datasource queries will not break
-        if (attributes.find(key) != attributes.end())
-        {
-            attributes.erase(key);
-        }
+        attributes.erase(key);
     }
     // if key is not the special __id__ keyword
-    else if (attributes.find(key) == attributes.end())
+    else
     {
         // them make sure the datasource query includes this field
         attributes.insert(key);

NB: I also removed redundant find()s, because std::set::insert/erase actually do a find first.

@artemp
Member
artemp commented Aug 16, 2012

@lightmare - thanks, applied in e6e32fc

@artemp artemp closed this Aug 16, 2012
@artemp artemp added a commit that referenced this issue Aug 16, 2012
@artemp artemp + applied patch from @lightmare - #1325 e6e32fc
@springmeyer springmeyer pushed a commit that referenced this issue Aug 17, 2012
Dane Springmeyer partial revert of e6e32fc and general cleanup to match node-mapnik gr…
…id api - refs #1325 and 1315
25a1643
@springmeyer springmeyer reopened this Aug 17, 2012
@springmeyer
Member

re-opened, e6e32fc looks good but in fact the original behavior is by design - sorry it took me so long to take a closer look at this. I've reverted in 25a1643.

So, @thegreat - basically mapnik.Grid objects are only designed to be rendered to once. I'd like to remove this limitation in the future but the current rendering code does not support this. So, if you want to render grids for two layers please create a new mapnik.Grid object per render. As far as grid merging code I'm aware there is something written in python in TileStache. Ideally we'd provide an api for this in C++ in Mapnik. I put that in the same bin as a larger move to bring json encoding in to the C++ core as well, see: #1197

Finally, I'm going to close this as "wontfix". If you think I'm overlooking something - just please comment further.

@tnightingale

Yeah, I had a feeling that was the case after I dug in a little further.
No worries, I'll keep an eye on #1197 - thanks for the follow-ups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment