A lot to follow :) 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.
About the order of methods on the dispatcher callback interface. I wonder if
we should update the callback interface to reflect the new dispatcher life
cycle? Either deprecating the old method and adding a new one. Or perhaps
adding a new preServiceDispatched lifecycle or something. Not a blocker just
a point of discussion.
2c.
-Justin
On Sun, Oct 31, 2010 at 10:44 AM, Andrea Aime
<[email protected]>wrote:
> 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
>
> -----------------------------------------------------
>
>
> ------------------------------------------------------------------------------
> Nokia and AT&T present the 2010 Calling All Innovators-North America
> contest
> Create new apps & games for the Nokia N8 for consumers in U.S. and Canada
> $10 million total in prizes - $4M cash, 500 devices, nearly $6M in
> marketing
> Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
> http://p.sf.net/sfu/nokia-dev2dev
> _______________________________________________
> Geoserver-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/geoserver-devel
>
>
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
------------------------------------------------------------------------------
Nokia and AT&T present the 2010 Calling All Innovators-North America contest
Create new apps & games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
_______________________________________________
Geoserver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
