Hi Alan, Would you open issue, I will take it!?
Maybe also post your opinion about think fix #1 or fix #2 is better. I tend to go for fix #1. getDeclaredField() should theoretically be faster, but that won't matter here: If it goes the slow path (going up to superclass) it will fail anyways and that's the exceptional case. A correct factory should have a NAME field and its lookup is fast and the additional check introduced for the class is cheap. Uwe ----- Uwe Schindler Achterdiek 19, D-28357 Bremen https://www.thetaphi.de eMail: u...@thetaphi.de > -----Original Message----- > From: Alan Woodward <romseyg...@gmail.com> > Sent: Monday, September 6, 2021 3:48 PM > To: dev@lucene.apache.org > Subject: Re: getField vs getDeclaredField in analysis SPI > > Thanks Uwe! > > > On 6 Sep 2021, at 13:11, Uwe Schindler <u...@thetaphi.de> wrote: > > > > Hi Alan, > > > >> LUCENE-9281 moved the `lookupSPIName` method from > >> AbstractAnalysisFactory to AnalysisSPILoader; the method is mostly the > same, > >> but one line has been changed from Class.getField() to > Class.getDeclaredField(). > >> This can fall foul of the Security Manager, which wants a higher level of > >> permission for getDeclaredField. Was this an intentional change? As I > > > > This was intentional because the previous code wasn't fully correct, > > because I > had some safety check in mind: The main reason for the getDeclaredField() is > to > lookup the field only in this class; while getField() also looks into > superclasses. > E.g. if the superclass has a NAME field because of a programming error it > would > pick that up, which would be wrong. When investigating other > implementations using "named" lookups out there (even in JDK), they used > getDeclaredField() when accessing a static member. > > > > There are 2 solutions: > > - Change to getField(), but in the if statement below check the actual > > class: > (field.getDeclaringClass()==service) (see https://github.com/apache/lucene- > solr/pull/1360/files#diff- > 6a65d91199a18bc4ee2d00a1e9dc283aedc4134846e0d7aafdc484f8263e250bR > 159-R162) > > - Wrap with doPrivileged in Lucene code. As far as I remember Lucene needs > the permission anyways. With doPrivileged you would delegate responsibility. > > > > I'd open a JIRA issue, I can fix this. It only affects Lucene 9.0. > > > >> understand it it’s looking for a NAME static field on the class in > >> question, > which > >> should always be public. I’m in the process of upgrading elasticsearch to > >> use > a > >> lucene 9 snapshot, and this change means that I need to wrap SPI reloading > >> code in doPrivileged() blocks, which is a bit of a pain. > > > > Thansk for doing this. Is Elasticsearch now using the Analysis Factory > framework instead of their own factories? > > > >> Thanks, Alan > > > > No problem, > > Uwe > > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > > For additional commands, e-mail: dev-h...@lucene.apache.org > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: dev-h...@lucene.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
