You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "cpoerschke (via GitHub)" <gi...@apache.org> on 2023/02/22 12:16:25 UTC
[GitHub] [solr] cpoerschke commented on a diff in pull request #1234: SOLR-15981: Add logExcludedParamsList parameter to support specifying params not to log
cpoerschke commented on code in PR #1234:
URL: https://github.com/apache/solr/pull/1234#discussion_r1114215511
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -2902,35 +2902,43 @@ public static void preDecorateResponse(SolrQueryRequest req, SolrQueryResponse r
toLog.add(PATH, req.getContext().get(PATH));
final SolrParams params = req.getParams();
- final String lpList = params.get(CommonParams.LOG_PARAMS_LIST);
- if (lpList == null) {
- toLog.add("params", "{" + req.getParamString() + "}");
- } else if (lpList.length() > 0) {
+ final String logParamsList = params.get(CommonParams.LOG_PARAMS_LIST);
+ final String logExcludedParamsList = params.get(CommonParams.LOG_EXCLUDED_PARAMS_LIST);
+
+ // Filter params by those in LOG_PARAMS_LIST (only if not empty) and in LOG_EXCLUDED_PARAMS_LIST
+ // so that we can then call toString
+ HashSet<String> shortSet =
+ logParamsList == null
+ ? new HashSet<>()
+ : new HashSet<>(Arrays.asList(logParamsList.split(",")));
+ HashSet<String> excludedSet =
+ logExcludedParamsList == null
+ ? new HashSet<>()
+ : new HashSet<>(Arrays.asList(logExcludedParamsList.split(",")));
Review Comment:
Wondering if `Collections.emptySet()` might also work here?
```suggestion
Set<String> shortSet =
logParamsList == null
? Collections.emptySet()
: new HashSet<>(Arrays.asList(logParamsList.split(",")));
Set<String> excludedSet =
logExcludedParamsList == null
? Collections.emptySet()
: new HashSet<>(Arrays.asList(logExcludedParamsList.split(",")));
```
##########
solr/solrj/src/java/org/apache/solr/common/params/CommonParams.java:
##########
@@ -236,6 +236,8 @@ public static EchoParamStyle get(String v) {
/** which parameters to log (if not supplied all parameters will be logged) * */
String LOG_PARAMS_LIST = "logParamsList";
+ String LOG_EXCLUDED_PARAMS_LIST = "logExcludedParamsList"; // which parameters not to log
Review Comment:
minor: for javadocs
```suggestion
/** which parameters not to log */
String LOG_EXCLUDED_PARAMS_LIST = "logExcludedParamsList";
```
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -2902,35 +2902,43 @@ public static void preDecorateResponse(SolrQueryRequest req, SolrQueryResponse r
toLog.add(PATH, req.getContext().get(PATH));
final SolrParams params = req.getParams();
- final String lpList = params.get(CommonParams.LOG_PARAMS_LIST);
- if (lpList == null) {
- toLog.add("params", "{" + req.getParamString() + "}");
- } else if (lpList.length() > 0) {
+ final String logParamsList = params.get(CommonParams.LOG_PARAMS_LIST);
+ final String logExcludedParamsList = params.get(CommonParams.LOG_EXCLUDED_PARAMS_LIST);
+
+ // Filter params by those in LOG_PARAMS_LIST (only if not empty) and in LOG_EXCLUDED_PARAMS_LIST
+ // so that we can then call toString
+ HashSet<String> shortSet =
+ logParamsList == null
+ ? new HashSet<>()
+ : new HashSet<>(Arrays.asList(logParamsList.split(",")));
+ HashSet<String> excludedSet =
+ logExcludedParamsList == null
+ ? new HashSet<>()
+ : new HashSet<>(Arrays.asList(logExcludedParamsList.split(",")));
+ SolrParams filteredParams =
+ new SolrParams() {
+ private static final long serialVersionUID = -643991638344314066L;
- // Filter params by those in LOG_PARAMS_LIST so that we can then call toString
- HashSet<String> lpSet = new HashSet<>(Arrays.asList(lpList.split(",")));
- SolrParams filteredParams =
- new SolrParams() {
- private static final long serialVersionUID = -643991638344314066L;
-
- @Override
- public Iterator<String> getParameterNamesIterator() {
- return Iterators.filter(params.getParameterNamesIterator(), lpSet::contains);
- }
+ @Override
+ public Iterator<String> getParameterNamesIterator() {
+ return Iterators.filter(
+ params.getParameterNamesIterator(),
+ p -> (shortSet.isEmpty() || shortSet.contains(p)) && !excludedSet.contains(p));
Review Comment:
Hmm, I'm not sure about the `shortSet.isEmpty() ||` bit here i.e. would it not result in `logParamsList=` (on its own) no longer suppressing all logging?
Thinking aloud ...
```
- shortSet.contains(p)
+ shortSet.contains(p) && !excludedSet.contains(p)
```
would cover the case where both sets are non-empty.
```
(shortSet.contains(p) && !excludedSet.contains(p)) ||
(shortSet.isEmpty() && !excludedSet.isEmpty() && !excludedSet.contains(p))
```
would also cover the case where only the excluded set is non-empty.
```
(shortSet.contains(p) || (shortSet.isEmpty() && !excludedSet.isEmpty())) &&
!excludedSet.contains(p)
```
as a shortened version of it, something like that?
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -2902,35 +2902,43 @@ public static void preDecorateResponse(SolrQueryRequest req, SolrQueryResponse r
toLog.add(PATH, req.getContext().get(PATH));
final SolrParams params = req.getParams();
- final String lpList = params.get(CommonParams.LOG_PARAMS_LIST);
- if (lpList == null) {
- toLog.add("params", "{" + req.getParamString() + "}");
- } else if (lpList.length() > 0) {
+ final String logParamsList = params.get(CommonParams.LOG_PARAMS_LIST);
+ final String logExcludedParamsList = params.get(CommonParams.LOG_EXCLUDED_PARAMS_LIST);
+
+ // Filter params by those in LOG_PARAMS_LIST (only if not empty) and in LOG_EXCLUDED_PARAMS_LIST
+ // so that we can then call toString
+ HashSet<String> shortSet =
+ logParamsList == null
+ ? new HashSet<>()
+ : new HashSet<>(Arrays.asList(logParamsList.split(",")));
+ HashSet<String> excludedSet =
+ logExcludedParamsList == null
+ ? new HashSet<>()
+ : new HashSet<>(Arrays.asList(logExcludedParamsList.split(",")));
+ SolrParams filteredParams =
+ new SolrParams() {
+ private static final long serialVersionUID = -643991638344314066L;
- // Filter params by those in LOG_PARAMS_LIST so that we can then call toString
- HashSet<String> lpSet = new HashSet<>(Arrays.asList(lpList.split(",")));
- SolrParams filteredParams =
- new SolrParams() {
- private static final long serialVersionUID = -643991638344314066L;
-
- @Override
- public Iterator<String> getParameterNamesIterator() {
- return Iterators.filter(params.getParameterNamesIterator(), lpSet::contains);
- }
+ @Override
+ public Iterator<String> getParameterNamesIterator() {
+ return Iterators.filter(
+ params.getParameterNamesIterator(),
+ p -> (shortSet.isEmpty() || shortSet.contains(p)) && !excludedSet.contains(p));
Review Comment:
Or another way could be to disallow the combination of the two parameters e.g. `logParamsList=Foo¬LogParamsList=Bar` generally or `logParamsList=¬LogParamsList=FooBar` specifically. What do you think?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org