nick-boss-tech commented on code in PR #5009:
URL: https://github.com/apache/solr/pull/5009#discussion_r4190185268
##########
solr/solr-ref-guide/modules/query-guide/pages/json-facet-api.adoc:
##########
@@ -407,6 +407,10 @@ By default, the ranges used to compute range faceting
between `start` and `end`
Refer <<Arbitrary Range>>
|===
+The range facet does not support the `limit`, `offset`, `sort`, `prelim_sort`,
`overrequest`, `overrefine`, or `refine` parameters that terms facets accept.
+Using any of them in a range facet is rejected with a 400 error naming the
parameter.
Review Comment:
Done, removed in 49ca9099d8e.
##########
solr/core/src/java/org/apache/solr/search/facet/FacetRangeParser.java:
##########
@@ -23,6 +23,13 @@
import org.apache.solr.search.SyntaxError;
class FacetRangeParser extends FacetParser<FacetRange> {
+
+ // Terms-facet parameters that have no effect on range facets. They used to
be silently
+ // ignored; now they are rejected so users notice the mistake (SOLR-18482).
The order is
+ // fixed so the error names the same parameter when several are present.
+ private static final List<String> UNSUPPORTED_PARAMS =
+ List.of("limit", "offset", "sort", "prelim_sort", "overrequest",
"overrefine", "refine");
Review Comment:
🤖 *AI text below* 🤖 *(posted on behalf of Nick Shanin)*
I looked into this. Facet parsing is pull-based: each parser reads the keys
it knows out of the args map and never looks at what is left over, so unknown
keys are ignored by construction, and there is no single place where a check
would go. A general version means a supported-key set per facet type (terms,
query, range, heatmap) plus the shared domain keys, validated at the end of
each parse; stats and sub-facet labels stay open because their keys are
user-chosen. It is feasible, and the JSON request parser already does the same
thing one level up, where unknown top-level keys are rejected. But it is a
behavior change beyond this ticket: roughly 100 to 200 lines plus tests, and
anything relying on the current leniency starts failing.
My suggestion: keep this PR to the range facet as filed, and I will open a
follow-up ticket for the general check and take it on. If you would rather see
it here, I can do that instead.
--
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]
