[WMS Client] "Invalid Hash table or key" error with WMS layers

[dmorissette]

In some cases, the WMS client code (and possibly other code) will call
msLookupHashTable() on a NULL metadata table, resulting in the message "Invalid
Hash table or key" being reported.

This happened mostly with Python scripts because the Python mapscript module is
set up so that the MapServer error stack is checked after method calls (such as
mapObj.draw), MapServer errors are converted to Python exceptions and are raised.

Here is a simplemapfile to reproduce this that was submitted by Howard B to
mapserver-dev:

MAP
NAME  "w2qpgi5qsg4k"
DEBUG on
SIZE 800 600
EXTENT 363411 4564460 365246 4565842

LAYER
   NAME cir
   METADATA
     "wms_title" "Iowa CIR"
     "wms_srs"   "EPSG:26915"
   END
   STATUS DEFAULT
   TYPE RASTER
   DEBUG ON
   MAXSCALE 100000
   CONNECTIONTYPE WMS
   CONNECTION
"http://cairo.gis.iastate.edu/cgi-bin/server.cgi?wmtver=1.0.0&layers=doqqs&format=jpg"
   PROJECTION
     "init=epsg:26915"
   END
END

END

[dmorissette]

I am able to reproduce and see the error in the debugger.

Lots of code (mine anyways) assumes that msLookupHashTable() will accept NULL
hash tables and return a NULL result, so I would propose that we replace the
msSetError() call that produces this error by a msDebug() call.

Here is the excerpt from msLookuphashTable():
---
  if(!table || !string) {
    msSetError(MS_HASHERR, "Invalid hash table or key", "msLookupHashTable");
    return(NULL);
  }
---

Any objections?

[sgillies@…]

Daniel,

I actually like that msLookupHashTable() throws an error.  Python users
expect an error when they try to look up in a dictionary the value of
a non-existent key, and so it has seemed natural to me that mapscript
users get an exception when they look up non-existent metadata keys.
If we were to make a change to msLookupHashTable, I'd suggest that it
would be only to include in the error message the value of the bogus
key as a string, and perhaps make a new MS_HASHKEYERR (but that could
wait till 4.4).

How about if we put the responsibility on code that calls msLookupHashTable()
to delete from the error stack these kind of benign errors?  It's more 
work, but maybe we could ameliorate it by a wrapper function, something
like a msLookupHashTableWithoutErrors?

[sgillies@…]

Daniel, here's a solution that will help us get 4.2 out the door quickly:

Let's do as you suggested and replace msSetError() with msDebug() in
msLookupHashTable().  This will fix the set of issues that prevent
Python Mapscript users from drawing maps with WMS layers.

I will write a new msGetHashTableValue() function that will stand in for
the old msLookupHashTable() underneath the getMetaData() methods of
mapObj, layerObj, and classObj.  I'll take care of this today and
commit it to the 4.2 and main branches.

[fwarmerdam]

Sean,

It is my humble opinion that the Python wrappers for getting a hash entry should
be where a NULL return result is turned into an exeception.  I too would expect
a hash table look function (from C, etc) to return NULL if the entry is missing. 
And it isn't as if the error is a sophisticated one where we need the best
diagnostics possible from down in the operator itself. 

So, I would vote for not even emitting a debug message in the hash table look
up function.  Just return NULL.  Add any exception semantics desired for
MapScript in mapscript.i.

My $0.02.

[sgillies@…]

Resolved as fixed.  I removed the call to msSetError from msLookupHashTable.
Tested using hobu's map file and it works.  I'd like to add this test case
to the Python mapscript unit tests but that may have to wait till 4.3 as
we would also need a semi-permanent WMS service to test against.

Frank, thanks for the comment, I'll think about it a bit more.

[dmorissette]

Thanks for fixing this Sean (I guess procrastination pays ;)

About the question of where the exception should be produced, it doesn't matter
much to me as long as I have access to a C function that silently returns NULL.
I don't like the suggestion of erasing the error stack in a wrapper since it is
a waste of cycles. If a wrapper is implemented then it should be the other way
around: it is the wrapper that should produce the exception, and the lower-level
function should silently returns NULL.

[sgillies@…]

Ack, forgot to commit this to main.  Is committed now.