Hey Justin, I'm not sure I understand what you don't agree with ;)
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. 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). 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. ------------------------------------------------------------------------------ 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
