Justin, As you've suggested, I implemented the headers check to preserve backwards compatibility. This implies that Response implementations that provide Content-Disposition will not support the new client API that allows requesting an alternate disposition. I extracted the code to add headers to a separate method. As an added benefit, each OWS request that used the old header API now produces 2 less 2D String arrays per request ;)
I've added this new Dispatcher patch to the JIRA ticket. Please review and let me know how this looks. Finally, I apologize for the grief I caused Gabriel for making GWC break. -Ian On Wed, Jul 27, 2011 at 5:02 PM, Justin Deoliveira <[email protected]> wrote: > Hey Ian, > > On Wed, Jul 27, 2011 at 4:23 PM, Ian Schneider <[email protected]> > wrote: >> >> Hey Justin, >> >> I'm not sure I understand what you don't agree with ;) >> > Just that a "mostly backward compatible" approach will suffice when a > completely backward compatible one is possible. Not saying you were > advocating for that so "don't agree" was poor phrasing on my part. >> >> As for implementing the check for the headers, this seems like a >> reasonable approach, though at least for code in the tree, only one >> implementation remains that uses the headers. I hesitate here because >> it involves more dispatcher code that will mostly not be used. >> > As I said output formats are the most common way that geoserver is extended > so "out of tree" implementations should be considered. During my time people > trying to implement custom output formats have come up a number of times... > I imagine many of them don't get contributed back, often with good reason > because they are very custom in nature. So I generally operate under the > assumption that there lots of them out there. > >> >> The other approach I offered was to make the default implementation >> opt-out of the new behavior (100% backwards compatible) and adjust the >> current implementors to opt-in (remove getHeaders implementation and >> use the new API). > > Right, which is why I think opting out of the new behaviour (by not calling > the new methods) iff the header has been set by getHeaders() is a > reasonable middle ground. >> >> We _do_ agree that the current solution is not optimal for out-of-tree >> Response subclasses, 100% backwards compatibility is to be strived >> for, and that changes to the API should not be taken lightly. >> >> -Ian >> >> On Wed, Jul 27, 2011 at 2:58 PM, Justin Deoliveira <[email protected]> >> wrote: >> > Hmmm... not sure I agree. I tend to always prefer 100% >> > backward compatibility when its possible. I know in GeoServer we don't >> > publish formal api at all but the output format is the one that is most >> > commonly implemented. So I think updating the api should not be taken >> > lightly. >> > What about having the dispatcher check the result of >> > Response.getHeaders() >> > and if the content-disposition header was specified there don't worry >> > about >> > calling the specific methods to try and determine it? >> > >> > On Wed, Jul 27, 2011 at 9:52 AM, Ian Schneider <[email protected]> >> > wrote: >> >> >> >> Hey Justin, >> >> >> >> In the case you mention, it is mostly backwards compatible - the >> >> original preferred headers are respected if the Response returns null >> >> from getAttachmentFileName. This has to be implemented in the Response >> >> subclass, though, since the default is to compute a filename based on >> >> operation and mimetype. The latter could be changed, but it seemed >> >> like a balance between getting useful default behavior or preserving >> >> the old behavior - which in almost all of the cases was trivial to >> >> upgrade. In other words, change the Response base class behavior to >> >> opt-in versus opt-out. >> >> >> >> As for the client overriding the preferred type (or headers via old >> >> API), that was the intention of the changes. >> >> >> >> On Wed, Jul 27, 2011 at 9:11 AM, Justin Deoliveira >> >> <[email protected]> >> >> wrote: >> >> > Hi Ian, >> >> > So are you saying that the change is backward compatible with >> >> > existing >> >> > Responses/output formats that use getHeaders() to set the content >> >> > dispotiion. Looking at the patch (and maybe I am not looking close >> >> > enough) >> >> > it seems like the new api will overwrite in the case where the client >> >> > specifies the content-disposition flag? >> >> > Excuse the noise if i am missing something. >> >> > On Tue, Jul 26, 2011 at 8:58 PM, Ian Schneider >> >> > <[email protected]> >> >> > wrote: >> >> >> >> >> >> Hi everyone, >> >> >> >> >> >> While committing my patches for >> >> >> http://jira.codehaus.org/browse/GEOS-4683, I broke the build. This >> >> >> was >> >> >> resolved fairly quickly (note to self - run maven just like hudson >> >> >> does) but made me realize these changes were worth a heads up to >> >> >> anyone authoring OWS Response subclasses. >> >> >> >> >> >> For details see the OWS Dispatcher and the Response javadoc, but the >> >> >> gist is that previously each Response subclass was handling >> >> >> Content-Disposition headers via the getHeaders method. The new >> >> >> behavior is that the Dispatcher uses the new API to determine a >> >> >> filename, and if not null, the preferred disposition type >> >> >> (attachment >> >> >> or the default, inline). If the request provides the >> >> >> content-disposition header parameter (either inline or attachment >> >> >> with >> >> >> header injection checking) then this is respected over the Response >> >> >> preference. >> >> >> >> >> >> I handled most of the Response subclasses and the tests. This is a >> >> >> heads up for existing maintainers on the changes as well as for new >> >> >> implementations. >> >> >> >> >> >> On a related note, it is not clear whether the filename should be >> >> >> quoted or encoded, but this patch will at least make that >> >> >> cross-cutting concern easier to address at some point (though it >> >> >> requires a number of test case adjustments). >> >> >> >> >> >> Thanks for bearing with me and comments much appreciated, >> >> >> -Ian >> >> >> >> >> >> -- >> >> >> Ian Schneider >> >> >> OpenGeo - http://opengeo.org >> >> >> Enterprise support for open source geospatial. >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> ------------------------------------------------------------------------------ >> >> >> Got Input? Slashdot Needs You. >> >> >> Take our quick survey online. Come on, we don't ask for help often. >> >> >> Plus, you'll get a chance to win $100 to spend on ThinkGeek. >> >> >> http://p.sf.net/sfu/slashdot-survey >> >> >> _______________________________________________ >> >> >> 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. >> >> > >> >> >> >> >> >> >> >> -- >> >> Ian Schneider >> >> OpenGeo - http://opengeo.org >> >> Enterprise support for open source geospatial. >> > >> > >> > >> > -- >> > Justin Deoliveira >> > OpenGeo - http://opengeo.org >> > Enterprise support for open source geospatial. >> > >> >> >> >> -- >> Ian Schneider >> OpenGeo - http://opengeo.org >> Enterprise support for open source geospatial. > > > > -- > Justin Deoliveira > OpenGeo - http://opengeo.org > Enterprise support for open source geospatial. > -- Ian Schneider OpenGeo - http://opengeo.org Enterprise support for open source geospatial. ------------------------------------------------------------------------------ Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
