Hi,
today I was following up on the bbox wfs kvp parsing issue cited a few days
ago. So I set myself up a new bbox parser parsing towards a filter,
and rolled out a bbox parser for WMS to parse an envelope instead.
It occurred to me to try out what would have happened if a request came
in without a service, but called from "/wfs?..."...
Boom! Exceptions, because the two bbox parsers are service specific now.
However, before it was working.
So I started to investigate what was happening in the dispatcher and
fixed issues
as I saw them... the changes started to cascade and I soon found myself
in research mode.
Oh well, nothing wrong with that but in the end I came up with a much larger
patch than I desired, and in the end the single blob I came up with is really
a mix of three patches: I'll eventually split it up in three parts but for now
take it as a draft that I'm making public for the sake of discussion.
And now let's talk about the three portions making up the patch (which is
attached).
BBOX parsing
---------------------
Nothing special here. The patch sets up a new bbox parser for WFS that
parses the bbox=... into a BBOX filter preserving the srsName verbatim.
So no need to jump through hoops in order to rebuild the srsName afterwards.
The old bbox parser has been moved to WMS, where it does the right thing,
and made service specific (specific to wms).
KVP/XML parsing, filtering, prioritizing, and heuristics (oh my!)
--------------------------------------------------------------------------------------------
The current dispatcher roughly works like this:
- parse the kvp (bzzt, wrong!)
- get the service, request and version from kvp or xml
- eventually parse the xml (bzzt, wrong!)
- apply heuristics to pick up the service and the version in case
they are not specified and we're not in cite compliance mode
- ... (lots of other stuff which is not relevant to this discussion)
The heuristics applied at the last step are basically:
- use the last part of the url as indication of the service if the
actual service is missing (that's the reason why we sometimes
see an error such as "can't find "ows" service when making
an invalid request)
- use the highest version if none was specified
Ok, so why the "bzzt, wrong!" thing? Well, because kvp parsers
and xml readers can be service, version, and even request specific.
But the heuristics to handle incomplete requests are handled too
late, when the kvp and xml parsing is long done.
So one big part of this patch moves the heuristics at the first
step in the chain instead, so that the kvp and xml parsing
can work against the chosen service and version, regardless
of how we came to that choice.
This basically means the choice of the Service object is now
done first, whilst before it was done later. No big deal, but to
preserve compatibility with DispatcherCallback, that exposes
the old order, the serviceDispatched() callback is fired exactly
at the same time as before (which is the reason why Request
now has a serviceBean field, to store the Service decided during
the init and provide it back when it's time to use it for callbacks
and dispatch).
Once the service and version are found they are fed back into
the Request and into kvp map so that KVPUtils.parseKVP(Map)
method, which is used in various other places, would not
need changes.
Cite compliance
--------------------------------------------------------------------------
Moving up the heuristics for locating the service and the
version broke the cite compliance
checks because they end up playing with the version and
service inferred from the heuristics.
This first made me add a "native" (or should I have said
"original") service and version to be used for those checks:
if the request did not actually have the service and/or version
that will result in an exception.
Now, looking around I noticed a mismatch, the cite compliance
mode was actually respected only for wfs using the "citecompliancehack"
interceptor, which then put the dispatcher into "strict" mode.
However the cite compliance configuration is exposed for all services,
not just wfs.
One way would have been to have a similar interceptor for the
other services, but why not use the dispatcher callback instead?
So what I did was to move the "citeCompliant" flag from the dispatcher
into the Request, and then create a generic callback that given
the service would look into the configuration and see if the cite
compliance flag was set for that specific service.
This had a repercussion on a WMS specific hack: in that protocol
for backwards compatibility the "wmtver" parameter is equated to the
"version". To have that keep on working I've rolled out another simple
callback that would copy the wmtver value into the service field.
Summing up
---------------------------------------------------
Two questions might arise:
- was all this really necessary?
- did you run the cite tests?
The answer to the second question is no, but if people think
the above changes are an improvement worth committing
I'll do run the cite tests, of course.
As for the first... hum... no, it is not strictly necessary.
However it makes kvp and xml parsers that are service and
version specific actually work as intended even when the request
is incomplete, and make the cite compliance flag work for all services,
not just for wfs.
Plan B
-------------------------------------------------
If changes in the patch are not well received or considered too risky,
is it still possible to
fix the original problem, that is, have the srsName of the bbox filter
preserved?
The answer is yes, a simpler, but hackier path is to have that parser return
a subclass of ReferencedEnvelope, let's call it SRSEnvelope, that also
stores the
srsName as is, and then use it in the wfs GetFeature kvp reader to
build the proper
BBOX filter.
It's probably 100loc total, so very small, quite well contained, but
it's clear it's a workaround
for a non fully working situation in the kvp parsing handling.
Long story short, it's not that I need the patch attached to this mail.
We can consider it an exploration of some of current dispatcher weaknesses and
put it apart as R&D for times where we have more time or we're
interested in taking
more risks.
Oh well, this mail has been long enough. I'll let you decide whether
the attached patch
falls into the "trick" or in the "treat" category :-p
Let me know!
Cheers
Andrea
-----------------------------------------------------
Ing. Andrea Aime
Senior Software Engineer
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584962313
fax: +39 0584962313
http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf
-----------------------------------------------------
(attachments)
dispatcher.patch (34 KB)
but thanks for laying it out in a very organized way. Basically imo yes these changes are worth trying to keep and integrate directly into the dispatcher. I think the patch looks good for the most part. One thing the did confuse me the term nativeService and nativeVersion, not knowing if “native” refereed to the original requested by the client or the actual service/version that gets matched. If I am not mistaken something like “requestService” and “requestedVersion” would be more explicit.