On Mon, May 17, 2021 at 8:48 AM Frank Gasdorf via GeoTools-Devel < geotools-devel@lists.sourceforge.net> wrote:
> Hi folks, > > I stumbled about the SLDParser while I wass looking for final field > initializations with CommonFactoryFinder.getXXX > > However, I'm not sure about whether it was by intension but internally > ther is a cast from FilterFactory to FilterFactory2 on initailization an > ExpressionDOMParser. This looks strange because other implementations (and > contributions via SPI) might implement FilterFactory but not FilterFactory2. > Hard to say, even going back before the Java 11 compatibility work, the cast was already there at the beginning of the git history, 11 years ago. Did a very quick check in SVN. the earliest I can find CommonFactoryFinder is in 2.7.x, the cast was already like that: https://svn.osgeo.org/geotools/branches/2.7.x/modules/library/api/src/main/java/org/geotools/factory/CommonFactoryFinder.java We might not find someone that remembers why things were done this way. > > In addition a filed expressionDomParser is initialized with static field > FF but not used in every method the same way. Sometimes an > ExpressionDOMParseris created while needed with the mentioned cast of > internal FilterFactory, sometimes it uses > CommonFactoryFinder.getFilterFactory2() and some methods using the internal > field expressionDomParser > > What do you think about changing the constructor as follows > > public SLDParser(StyleFactory factory, FilterFactory filterFactory) { > this.factory = factory; > this.ff = filterFactory; > if (filterFactory instanceof FilterFactory2) { > this.filterFactory2 = (FilterFactory2)filterFactory; > } else { > this.filterFactory2 = CommonFactoryFinder.getFilterFactory2(); > } > this.expressionDOMParser = new ExpressionDOMParser(filterFactory2); > this.onlineResourceLocator = new DefaultResourceLocator(); > } > > and change the member expressionDOMParser to a final field. All other > places where ExpressionDOMParsers ar created should be changed to use final > field expressionDOMParser instead. > The class has a setter, which would have to be removed (instead of just being deprecated). That's not our normal API removal cycle (first deprecate, then remove), but it is allowed in exceptional cases. In this case I don't see a valid enough reason to break downstream apps that might be using the setter, so -1 on the straight removal of the method. As noted in https://github.com/geotools/geotools/pull/3516 I'm skeptical about the deprecation too, but not strongly against it, if some other core dev takes responsibility for it I'm not going to oppose. Regards, Andrea Aime == GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it ------------------------------------------------------- *Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni circostanza inerente alla presente email (il suo contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le sarei comunque grato se potesse darmene notizia. This email is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. We remind that - as provided by European Regulation 2016/679 “GDPR” - copying, dissemination or use of this e-mail or the information herein by anyone other than the intended recipient is prohibited. If you have received this email by mistake, please notify us immediately by telephone or e-mail.*
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
