Copilot commented on code in PR #4203:
URL: https://github.com/apache/solr/pull/4203#discussion_r2910616825
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java:
##########
@@ -86,7 +88,8 @@
import org.slf4j.LoggerFactory;
/** All V2 APIs have a prefix of /api/schema-designer/ */
-public class SchemaDesignerAPI implements SchemaDesignerConstants {
+public class SchemaDesignerAPI extends JerseyResource
+ implements SchemaDesignerApi, SchemaDesignerConstants {
Review Comment:
The PR title/description focuses on a small unchecked-cast warning fix in
`DefaultSampleDocumentsLoader`, but this PR also includes a substantial Schema
Designer API migration to Jersey (new endpoint interface, changed CoreContainer
registration, UI endpoint changes, large test rewrites). Please update the PR
description/title to reflect the actual scope so reviewers can assess risk
appropriately.
##########
solr/webapp/web/js/angular/controllers/schema-designer.js:
##########
@@ -1526,7 +1526,7 @@ solrAdminApp.controller('SchemaDesignerController',
function ($scope, $timeout,
if (sessionStorage.getItem("auth.header")) {
var fileName = $scope.currentSchema+"_configset.zip";
var xhr = new XMLHttpRequest();
- xhr.open("GET",
"/api/schema-designer/download/"+fileName+"?wt=raw&configSet="+$scope.currentSchema,
true);
+ xhr.open("GET",
"/api/schema-designer/download?configSet="+$scope.currentSchema, true);
xhr.setRequestHeader('Authorization',
sessionStorage.getItem("auth.header"));
xhr.responseType = 'blob';
Review Comment:
The download request no longer includes a `wt` param or an explicit `Accept`
header. With the current `MediaTypeOverridingFilter`, requests with `Accept:
*/*` (common for browser navigations) get their `Content-Type` forcibly set to
`application/json`, which undermines this endpoint’s `application/zip`
response. Consider either re-adding a `wt` query param (as before) to bypass
the default override, or set `Accept: application/zip` on the XHR branch and
keep a `wt` param on the non-auth `location.href` branch.
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerSettings.java:
##########
@@ -25,7 +25,7 @@
import java.util.Optional;
import org.apache.solr.schema.ManagedIndexSchema;
-class SchemaDesignerSettings implements SchemaDesignerConstants {
+public class SchemaDesignerSettings implements SchemaDesignerConstants {
Review Comment:
This class was changed from package-private to `public`, which expands the
public API surface of `solr-core` without an obvious need (current usages
appear to stay within `org.apache.solr.handler.designer` and its tests). If
external access isn’t required, consider keeping it package-private to avoid
committing to its API long-term.
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java:
##########
@@ -1291,41 +1340,41 @@ protected String checkMutable(String configSet,
SolrQueryRequest req) throws IOE
final int schemaVersionInZk =
configSetHelper.getCurrentSchemaVersion(mutableId);
if (schemaVersionInZk != -1) {
// check the versions agree
- configSetHelper.checkSchemaVersion(
- mutableId, requireSchemaVersionFromClient(req), schemaVersionInZk);
+ configSetHelper.checkSchemaVersion(mutableId, clientSchemaVersion,
schemaVersionInZk);
} // else the stored is -1, can't really enforce here
return mutableId;
}
- protected int requireSchemaVersionFromClient(SolrQueryRequest req) {
- final int schemaVersion = req.getParams().getInt(SCHEMA_VERSION_PARAM, -1);
- if (schemaVersion == -1) {
+ protected void requireSchemaVersion(Integer schemaVersion) {
+ if (schemaVersion == null) {
throw new SolrException(
- SolrException.ErrorCode.BAD_REQUEST,
- SCHEMA_VERSION_PARAM + " is a required parameter for the " +
req.getPath() + " endpoint");
+ SolrException.ErrorCode.BAD_REQUEST, SCHEMA_VERSION_PARAM + " is a
required parameter!");
}
- return schemaVersion;
}
Review Comment:
`requireSchemaVersion()` now only rejects `null`, but previously the API
treated `-1` as “missing” (since `getInt(..., -1)` was used) and rejected it.
As written, a client can pass `schemaVersion=-1` and bypass this validation; it
would be safer to also reject negative values to keep the same contract.
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java:
##########
@@ -775,38 +808,53 @@ public void query(SolrQueryRequest req, SolrQueryResponse
rsp)
version,
currentVersion);
List<SolrInputDocument> docs =
configSetHelper.retrieveSampleDocs(configSet);
- ManagedIndexSchema schema = loadLatestSchema(mutableId);
- errorsDuringIndexing =
- indexSampleDocsWithRebuildOnAnalysisError(
- schema.getUniqueKeyField().getName(), docs, mutableId, true,
null);
- // the version changes when you index (due to field guessing URP)
- currentVersion = configSetHelper.getCurrentSchemaVersion(mutableId);
+ if (!docs.isEmpty()) {
+ ManagedIndexSchema schema = loadLatestSchema(mutableId);
+ errorsDuringIndexing =
+ indexSampleDocsWithRebuildOnAnalysisError(
+ schema.getUniqueKeyField().getName(), docs, mutableId, true,
null);
+ // the version changes when you index (due to field guessing URP)
+ currentVersion = configSetHelper.getCurrentSchemaVersion(mutableId);
+ }
indexedVersion.put(mutableId, currentVersion);
}
if (errorsDuringIndexing != null) {
- Map<String, Object> response = new HashMap<>();
- rsp.setException(
- new SolrException(
- SolrException.ErrorCode.BAD_REQUEST,
- "Failed to re-index sample documents after schema updated."));
- response.put(ERROR_DETAILS, errorsDuringIndexing);
- rsp.getValues().addAll(response);
- return;
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST,
+ "Failed to re-index sample documents after schema updated.");
}
Review Comment:
`query()` used to return an error response containing `errorDetails` when
re-indexing failed, which the Schema Designer UI surfaces (see its
`errorHandler`). This new implementation throws a `SolrException` without
attaching `errorDetails`, so clients will lose the per-doc failure details.
Consider instantiating a `FlexibleSolrJerseyResponse` early and setting an
`errorDetails` top-level property before throwing, or otherwise preserve the
prior error payload structure.
--
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]
