epugh commented on code in PR #4101:
URL: https://github.com/apache/solr/pull/4101#discussion_r2763346261
##########
solr/core/src/java/org/apache/solr/handler/designer/DefaultSampleDocumentsLoader.java:
##########
@@ -148,7 +148,7 @@ protected List<SolrInputDocument> loadCsvDocs(
} else {
stream = new ContentStreamBase.ByteArrayStream(streamBytes, source,
"text/csv");
}
- return (new SampleCSVLoader(new CSVRequest(params),
maxDocsToLoad)).loadDocs(stream);
Review Comment:
Here we deleted a trivial class!
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java:
##########
@@ -45,14 +49,15 @@
* <p>The <code>close()</code> method must be called on any instance of this
class once it is no
* longer in use.
*/
-public abstract class SolrQueryRequestBase implements SolrQueryRequest,
Closeable {
+public class SolrQueryRequestBase implements SolrQueryRequest, Closeable {
Review Comment:
Do we eliminate `SolrQueryRequest` interface?
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java:
##########
@@ -45,14 +49,15 @@
* <p>The <code>close()</code> method must be called on any instance of this
class once it is no
* longer in use.
*/
-public abstract class SolrQueryRequestBase implements SolrQueryRequest,
Closeable {
+public class SolrQueryRequestBase implements SolrQueryRequest, Closeable {
protected final SolrCore core;
protected final SolrParams origParams;
protected volatile IndexSchema schema;
protected SolrParams params;
protected Map<Object, Object> context;
protected Iterable<ContentStream> streams;
protected Map<String, Object> json;
+ protected String userPrincipalName = null;
Review Comment:
I investigated why we had to preserve this here when only one class uses it
and:
**`DocExpirationUpdateProcessorFactory` is the ONLY built-in component
that:**
1. Creates background/scheduled requests (not triggered by HTTP)
2. Needs authentication (because it makes distributed deletes)
3. Must identify itself as a node-initiated operation
Other internal requests either:
- Don't need authentication (single-node operations)
- Are triggered by user requests (already have principal from HTTP)
- Use different mechanisms (like HTTP client with PKI headers for inter-node
communication)
This is why `setUserPrincipalName()` exists but is rarely used - it's a
special-purpose API for a rare use case!
I wondered about another home for this, or how do we handle secure internode
elsewhere...? Any ideas?
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java:
##########
@@ -191,7 +225,23 @@ public void setJSON(Map<String, Object> json) {
@Override
public Principal getUserPrincipal() {
- return null;
+ if (userPrincipalName == null) {
+ return null;
+ }
+ return new LocalPrincipal(userPrincipalName);
+ }
+
+ /**
+ * Allows setting the 'name' of the User Principal for the purposes of
creating local requests in
+ * a solr node when security is enabled. It is experimental and subject to
removal
+ *
+ * @see PKIAuthenticationPlugin#NODE_IS_USER
+ * @see #getUserPrincipal
+ * @lucene.internal
Review Comment:
these tags are copied over from oroginal implementation. Not sure what
they mean or why we have them at this point. If there was a better more
"supported" way to do this, I'd love to hear about it!
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java:
##########
@@ -137,7 +171,7 @@ public SolrCore getCore() {
// The index schema associated with this request
@Override
public IndexSchema getSchema() {
- // a request for a core admin will no have a core
+ // a request for a core admin will not have a core
Review Comment:
I am not sure what this comment is telling us..
##########
solr/core/src/java/org/apache/solr/request/SolrQueryRequestBase.java:
##########
@@ -72,6 +77,35 @@ public SolrQueryRequestBase(SolrCore core, SolrParams
params) {
this(core, params, new RTimerTree());
}
+ /**
+ * Utility method to build SolrParams from individual query components. This
is a convenience
+ * method for legacy code that needs to construct params from separate
query, qtype, start, limit,
Review Comment:
how strongly do we want to eliminate this pattern? Actually, on more
investigation, it's only used by five classes, though 27 times!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
