gerlowskija commented on code in PR #4203: URL: https://github.com/apache/solr/pull/4203#discussion_r3208863995
########## solr/api/src/java/org/apache/solr/client/api/endpoint/SchemaDesignerApi.java: ########## @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.DefaultValue; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.PUT; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.QueryParam; +import java.util.List; +import org.apache.solr.client.api.model.FlexibleSolrJerseyResponse; +import org.apache.solr.client.api.model.SchemaDesignerCollectionsResponse; +import org.apache.solr.client.api.model.SchemaDesignerConfigsResponse; +import org.apache.solr.client.api.model.SchemaDesignerInfoResponse; +import org.apache.solr.client.api.model.SchemaDesignerPublishResponse; +import org.apache.solr.client.api.model.SchemaDesignerResponse; +import org.apache.solr.client.api.model.SchemaDesignerSchemaDiffResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** V2 API definitions for the Solr Schema Designer. */ +@Path("/schema-designer") +public interface SchemaDesignerApi { Review Comment: Rather than commenting on each method or annotation line here, I'll combine all my "cosmetic" suggestions into a single comment Here's what the PR currently proposes in terms of JAX-RS APIs... 1. GET /api/schema-designer/{configSet}/info 2. POST /api/schema-designer/{configSet}/prep 3. DELETE /api/schema-designer/{configSet} 4. PUT /api/schema-designer/{configSet}/file 5. GET /api/schema-designer/{configSet}/sample 6. GET /api/schema-designer/{configSet}/collectionsForConfig 7. GET /api/schema-designer/configs 8. POST /api/schema-designer/{configSet}/add 9. PUT /api/schema-designer/{configSet}/update 10. PUT /api/schema-designer/{configSet}/publish 11. POST /api/schema-designer/{configSet}/analyze 12. GET /api/schema-designer/{configSet}/query 13. GET /api/schema-designer/{configSet}/diff My notes/suggestions: > GET /api/schema-designer/configs I love the "plural noun" configs here. Very common REST-ful idiom or listing out resources, that we use throughout the v2 API (e.g. `/api/collections`, `/api/aliases`). IMO we should add this path-segments to all of these schema-designer APIs > POST /api/schema-designer/{configSet}/add > PUT /api/schema-designer/{configSet}/update IMO it's more REST-ful and consistent with our other APIs to drop the `/add` and `/update` path segments. Doing `PUT` or `POST` at a particular path is a pretty common idiom for create/update. Dropping `/add` and `/update` would keep things more consistent with our other v2 CRUD APIs. > GET /api/schema-designer/{configSet}/info Similarly, I'd suggest dropping `/info` from this path. ########## solr/api/src/java/org/apache/solr/client/api/endpoint/SchemaDesignerApi.java: ########## @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.DefaultValue; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.PUT; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.QueryParam; +import java.util.List; +import org.apache.solr.client.api.model.FlexibleSolrJerseyResponse; +import org.apache.solr.client.api.model.SchemaDesignerCollectionsResponse; +import org.apache.solr.client.api.model.SchemaDesignerConfigsResponse; +import org.apache.solr.client.api.model.SchemaDesignerInfoResponse; +import org.apache.solr.client.api.model.SchemaDesignerPublishResponse; +import org.apache.solr.client.api.model.SchemaDesignerResponse; +import org.apache.solr.client.api.model.SchemaDesignerSchemaDiffResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** V2 API definitions for the Solr Schema Designer. */ +@Path("/schema-designer") +public interface SchemaDesignerApi { + + @GET + @Path("/{configSet}/info") + @Operation( + summary = "Get info about a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerInfoResponse getInfo(@PathParam("configSet") String configSet) throws Exception; + + @POST + @Path("/{configSet}/prep") + @Operation( + summary = "Prepare a mutable configSet copy for schema design.", + tags = {"schema-designer"}) + SchemaDesignerResponse prepNewSchema( + @PathParam("configSet") String configSet, @QueryParam("copyFrom") String copyFrom) + throws Exception; + + @DELETE + @Path("/{configSet}") + @Operation( + summary = "Clean up temporary resources for a schema being designed.", + tags = {"schema-designer"}) + SolrJerseyResponse cleanupTempSchema(@PathParam("configSet") String configSet) throws Exception; + + @PUT + @Path("/{configSet}/file") + @Operation( + summary = "Update the contents of a file in a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse updateFileContents( + @PathParam("configSet") String configSet, @QueryParam("file") String file) throws Exception; + + @GET + @Path("/{configSet}/sample") + @Operation( + summary = "Get a sample value and analysis for a field.", + tags = {"schema-designer"}) + FlexibleSolrJerseyResponse getSampleValue( + @PathParam("configSet") String configSet, + @QueryParam("field") String fieldName, + @QueryParam("uniqueKeyField") String idField, + @QueryParam("docId") String docId) + throws Exception; + + @GET + @Path("/{configSet}/collectionsForConfig") + @Operation( + summary = "List collections that use a given configSet.", + tags = {"schema-designer"}) + SchemaDesignerCollectionsResponse listCollectionsForConfig( + @PathParam("configSet") String configSet) throws Exception; + + @GET + @Path("/configs") + @Operation( + summary = "List all configSets available for schema design.", + tags = {"schema-designer"}) + SchemaDesignerConfigsResponse listConfigs() throws Exception; + + @POST + @Path("/{configSet}/add") + @Operation( + summary = "Add a new field, field type, or dynamic field to the schema being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse addSchemaObject( + @PathParam("configSet") String configSet, @QueryParam("schemaVersion") Integer schemaVersion) + throws Exception; + + @PUT + @Path("/{configSet}/update") + @Operation( + summary = "Update an existing field or field type in the schema being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse updateSchemaObject( + @PathParam("configSet") String configSet, @QueryParam("schemaVersion") Integer schemaVersion) Review Comment: [Q/-1] Ditto, re: the schema-object itself isn't represented in the method signature at all. ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesigner.java: ########## @@ -1184,25 +1196,41 @@ protected Map<String, Object> buildResponse( List<String> sortedFiles = new ArrayList<>(stripPrefix); Collections.sort(sortedFiles); - response.put("files", sortedFiles); + response.files = sortedFiles; // info about the sample docs if (docs != null) { final String uniqueKeyField = schema.getUniqueKeyField().getName(); - response.put( - "docIds", + response.docIds = docs.stream() .map(d -> (String) d.getFieldValue(uniqueKeyField)) .filter(Objects::nonNull) .limit(100) - .collect(Collectors.toList())); + .collect(Collectors.toList()); } - response.put("numDocs", docs != null ? docs.size() : -1); + response.numDocs = docs != null ? docs.size() : -1; return response; } + /** Sets the named schema-object field on {@code response} based on the action type. */ + private static void setSchemaObjectField( + SchemaDesignerResponse response, String action, Object value) { + // Handles both bare camelCase names used internally ('field', 'fieldType') and the + // kebab-case prefixed names that come directly from Schema API request JSON + // ('add-field', 'add-field-type', 'add-dynamic-field'). + switch (action) { + case "field", "add-field" -> response.field = value; + case "type", "add-type" -> response.type = value; + case "dynamicField", "add-dynamic-field" -> response.dynamicField = value; + case "fieldType", "add-field-type" -> response.fieldType = value; + default -> { + /* unknown action type — silently ignore */ Review Comment: [-1] Let's throw an error or have an assertion or something here. ########## solr/api/src/java/org/apache/solr/client/api/endpoint/SchemaDesignerApi.java: ########## @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.DefaultValue; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.PUT; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.QueryParam; +import java.util.List; +import org.apache.solr.client.api.model.FlexibleSolrJerseyResponse; +import org.apache.solr.client.api.model.SchemaDesignerCollectionsResponse; +import org.apache.solr.client.api.model.SchemaDesignerConfigsResponse; +import org.apache.solr.client.api.model.SchemaDesignerInfoResponse; +import org.apache.solr.client.api.model.SchemaDesignerPublishResponse; +import org.apache.solr.client.api.model.SchemaDesignerResponse; +import org.apache.solr.client.api.model.SchemaDesignerSchemaDiffResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** V2 API definitions for the Solr Schema Designer. */ +@Path("/schema-designer") +public interface SchemaDesignerApi { + + @GET + @Path("/{configSet}/info") + @Operation( + summary = "Get info about a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerInfoResponse getInfo(@PathParam("configSet") String configSet) throws Exception; + + @POST + @Path("/{configSet}/prep") + @Operation( + summary = "Prepare a mutable configSet copy for schema design.", + tags = {"schema-designer"}) + SchemaDesignerResponse prepNewSchema( + @PathParam("configSet") String configSet, @QueryParam("copyFrom") String copyFrom) + throws Exception; + + @DELETE + @Path("/{configSet}") + @Operation( + summary = "Clean up temporary resources for a schema being designed.", + tags = {"schema-designer"}) + SolrJerseyResponse cleanupTempSchema(@PathParam("configSet") String configSet) throws Exception; + + @PUT + @Path("/{configSet}/file") + @Operation( + summary = "Update the contents of a file in a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse updateFileContents( + @PathParam("configSet") String configSet, @QueryParam("file") String file) throws Exception; + + @GET + @Path("/{configSet}/sample") + @Operation( + summary = "Get a sample value and analysis for a field.", + tags = {"schema-designer"}) + FlexibleSolrJerseyResponse getSampleValue( + @PathParam("configSet") String configSet, + @QueryParam("field") String fieldName, + @QueryParam("uniqueKeyField") String idField, + @QueryParam("docId") String docId) + throws Exception; + + @GET + @Path("/{configSet}/collectionsForConfig") Review Comment: [0] This API is kindof interesting - seemingly there's nothing schema-designer specific about it at all? You could imagine it moving to be a part of our existing "list-collections" API exposed by a query param, e.g. `GET /api/collections?configSet=foo` Not work for this PR; just thinking aloud a bit.... ########## solr/packaging/test/test_schema_designer.bats: ########## @@ -0,0 +1,181 @@ +#!/usr/bin/env bats Review Comment: [-0] Is there a particular reason to do a BATS test in this case? Is there functionality that can't be tested using a normal Java-based integration test? IMO our default should be to cover things in a Java-based test, since the BATS tests can't be parallelized, run on Windows, etc. ########## solr/api/src/java/org/apache/solr/client/api/model/SchemaDesignerCollectionsResponse.java: ########## @@ -0,0 +1,27 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.model; + +import com.fasterxml.jackson.annotation.JsonProperty; +import java.util.List; + +/** Response body for the Schema Designer list-collections-for-config endpoint. */ +public class SchemaDesignerCollectionsResponse extends SolrJerseyResponse { Review Comment: [-0] This is identical to the `ListCollectionsResponse` that's used for `GET /api/collections`...could we just reuse that class in schema-designer and nuke this new response class? ########## solr/api/src/java/org/apache/solr/client/api/endpoint/SchemaDesignerApi.java: ########## @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.DefaultValue; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.PUT; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.QueryParam; +import java.util.List; +import org.apache.solr.client.api.model.FlexibleSolrJerseyResponse; +import org.apache.solr.client.api.model.SchemaDesignerCollectionsResponse; +import org.apache.solr.client.api.model.SchemaDesignerConfigsResponse; +import org.apache.solr.client.api.model.SchemaDesignerInfoResponse; +import org.apache.solr.client.api.model.SchemaDesignerPublishResponse; +import org.apache.solr.client.api.model.SchemaDesignerResponse; +import org.apache.solr.client.api.model.SchemaDesignerSchemaDiffResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** V2 API definitions for the Solr Schema Designer. */ +@Path("/schema-designer") +public interface SchemaDesignerApi { + + @GET + @Path("/{configSet}/info") + @Operation( + summary = "Get info about a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerInfoResponse getInfo(@PathParam("configSet") String configSet) throws Exception; + + @POST + @Path("/{configSet}/prep") + @Operation( + summary = "Prepare a mutable configSet copy for schema design.", + tags = {"schema-designer"}) + SchemaDesignerResponse prepNewSchema( + @PathParam("configSet") String configSet, @QueryParam("copyFrom") String copyFrom) + throws Exception; + + @DELETE + @Path("/{configSet}") + @Operation( + summary = "Clean up temporary resources for a schema being designed.", + tags = {"schema-designer"}) + SolrJerseyResponse cleanupTempSchema(@PathParam("configSet") String configSet) throws Exception; + + @PUT + @Path("/{configSet}/file") + @Operation( + summary = "Update the contents of a file in a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse updateFileContents( Review Comment: [Q/-1] Where are the actual file contents in this API definition? I assume they're in the request-body, but they're not represented in the method signature at all. Is that intentional? ########## solr/api/src/java/org/apache/solr/client/api/endpoint/SchemaDesignerApi.java: ########## @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.DefaultValue; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.PUT; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.QueryParam; +import java.util.List; +import org.apache.solr.client.api.model.FlexibleSolrJerseyResponse; +import org.apache.solr.client.api.model.SchemaDesignerCollectionsResponse; +import org.apache.solr.client.api.model.SchemaDesignerConfigsResponse; +import org.apache.solr.client.api.model.SchemaDesignerInfoResponse; +import org.apache.solr.client.api.model.SchemaDesignerPublishResponse; +import org.apache.solr.client.api.model.SchemaDesignerResponse; +import org.apache.solr.client.api.model.SchemaDesignerSchemaDiffResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** V2 API definitions for the Solr Schema Designer. */ +@Path("/schema-designer") +public interface SchemaDesignerApi { + + @GET + @Path("/{configSet}/info") + @Operation( + summary = "Get info about a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerInfoResponse getInfo(@PathParam("configSet") String configSet) throws Exception; + + @POST + @Path("/{configSet}/prep") + @Operation( + summary = "Prepare a mutable configSet copy for schema design.", + tags = {"schema-designer"}) + SchemaDesignerResponse prepNewSchema( + @PathParam("configSet") String configSet, @QueryParam("copyFrom") String copyFrom) + throws Exception; + + @DELETE + @Path("/{configSet}") + @Operation( + summary = "Clean up temporary resources for a schema being designed.", + tags = {"schema-designer"}) + SolrJerseyResponse cleanupTempSchema(@PathParam("configSet") String configSet) throws Exception; + + @PUT + @Path("/{configSet}/file") + @Operation( + summary = "Update the contents of a file in a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse updateFileContents( + @PathParam("configSet") String configSet, @QueryParam("file") String file) throws Exception; + + @GET + @Path("/{configSet}/sample") + @Operation( + summary = "Get a sample value and analysis for a field.", + tags = {"schema-designer"}) + FlexibleSolrJerseyResponse getSampleValue( + @PathParam("configSet") String configSet, + @QueryParam("field") String fieldName, + @QueryParam("uniqueKeyField") String idField, + @QueryParam("docId") String docId) + throws Exception; + + @GET + @Path("/{configSet}/collectionsForConfig") + @Operation( + summary = "List collections that use a given configSet.", + tags = {"schema-designer"}) + SchemaDesignerCollectionsResponse listCollectionsForConfig( + @PathParam("configSet") String configSet) throws Exception; + + @GET + @Path("/configs") + @Operation( + summary = "List all configSets available for schema design.", + tags = {"schema-designer"}) + SchemaDesignerConfigsResponse listConfigs() throws Exception; + + @POST + @Path("/{configSet}/add") + @Operation( + summary = "Add a new field, field type, or dynamic field to the schema being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse addSchemaObject( + @PathParam("configSet") String configSet, @QueryParam("schemaVersion") Integer schemaVersion) Review Comment: [Q/-1] Where is the "schema object" being modified or added? I assume it's in the request-body of the API. If that's the case it should be represented in the method signature here as an `InputStream` param or something similar. ########## solr/api/src/java/org/apache/solr/client/api/model/SchemaDesignerInfoResponse.java: ########## @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.model; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; +import java.util.List; + +/** Response body for the Schema Designer get-info endpoint. */ +@JsonInclude(JsonInclude.Include.NON_NULL) Review Comment: [0] I think this is the default and could be omitted. (Applies here and to other model classes where you've added this annotation) ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesigner.java: ########## @@ -1273,7 +1360,40 @@ protected void addSettingsToResponse( } } - protected String checkMutable(String configSet, SolrQueryRequest req) throws IOException { + void addSettingsToResponse( + SchemaDesignerSettings settings, final SchemaDesignerInfoResponse response) { + response.languages = settings.getLanguages(); Review Comment: [-1] There's gotta be a better way to handle all of these method overloads. I'd guess this gets much much cleaner if we create a parent model class to contain all of these common properties that SchemaDesignerInfoResponse and friends extend from. ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesigner.java: ########## @@ -347,25 +348,31 @@ public void getSampleValue(SolrQueryRequest req, SolrQueryResponse rsp) throws I if (textValue != null) { var analysis = configSetHelper.analyzeField(configSet, fieldName, textValue); - rsp.getValues().addAll(Map.of(idField, docId, fieldName, textValue, "analysis", analysis)); + return buildFlexibleResponse( + Map.of(idField, docId, fieldName, textValue, "analysis", analysis)); } + return instantiateJerseyResponse(FlexibleSolrJerseyResponse.class); } - @EndPoint( - method = GET, - path = "/schema-designer/collectionsForConfig", - permission = CONFIG_READ_PERM) - public void listCollectionsForConfig(SolrQueryRequest req, SolrQueryResponse rsp) { - final String configSet = getRequiredParam(CONFIG_SET_PARAM, req); - rsp.getValues() - .addAll(Map.of("collections", configSetHelper.listCollectionsForConfig(configSet))); + @Override + @PermissionName(CONFIG_READ_PERM) + public SchemaDesignerCollectionsResponse listCollectionsForConfig(String configSet) { + requireNotEmpty(CONFIG_SET_PARAM, configSet); + SchemaDesignerCollectionsResponse response = + instantiateJerseyResponse(SchemaDesignerCollectionsResponse.class); + response.collections = configSetHelper.listCollectionsForConfig(configSet); + return response; } // CONFIG_EDIT_PERM is required here since this endpoint is used by the UI to determine if the // user has access to the Schema Designer UI - @EndPoint(method = GET, path = "/schema-designer/configs", permission = CONFIG_EDIT_PERM) - public void listConfigs(SolrQueryRequest req, SolrQueryResponse rsp) throws IOException { - rsp.getValues().addAll(Map.of("configSets", listEnabledConfigs())); + @Override + @PermissionName(CONFIG_EDIT_PERM) + public SchemaDesignerConfigsResponse listConfigs() throws Exception { + SchemaDesignerConfigsResponse response = + instantiateJerseyResponse(SchemaDesignerConfigsResponse.class); + response.configSets = listEnabledConfigs(); Review Comment: [Q] Is there a difference between these "enabled" configs and the configsets returned by `GET /api/configsets`? ########## solr/api/src/java/org/apache/solr/client/api/endpoint/SchemaDesignerApi.java: ########## @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.DefaultValue; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.PUT; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.QueryParam; +import java.util.List; +import org.apache.solr.client.api.model.FlexibleSolrJerseyResponse; +import org.apache.solr.client.api.model.SchemaDesignerCollectionsResponse; +import org.apache.solr.client.api.model.SchemaDesignerConfigsResponse; +import org.apache.solr.client.api.model.SchemaDesignerInfoResponse; +import org.apache.solr.client.api.model.SchemaDesignerPublishResponse; +import org.apache.solr.client.api.model.SchemaDesignerResponse; +import org.apache.solr.client.api.model.SchemaDesignerSchemaDiffResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** V2 API definitions for the Solr Schema Designer. */ +@Path("/schema-designer") +public interface SchemaDesignerApi { + + @GET + @Path("/{configSet}/info") + @Operation( + summary = "Get info about a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerInfoResponse getInfo(@PathParam("configSet") String configSet) throws Exception; + + @POST + @Path("/{configSet}/prep") + @Operation( + summary = "Prepare a mutable configSet copy for schema design.", + tags = {"schema-designer"}) + SchemaDesignerResponse prepNewSchema( + @PathParam("configSet") String configSet, @QueryParam("copyFrom") String copyFrom) + throws Exception; + + @DELETE + @Path("/{configSet}") + @Operation( + summary = "Clean up temporary resources for a schema being designed.", + tags = {"schema-designer"}) + SolrJerseyResponse cleanupTempSchema(@PathParam("configSet") String configSet) throws Exception; + + @PUT + @Path("/{configSet}/file") + @Operation( + summary = "Update the contents of a file in a configSet being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse updateFileContents( + @PathParam("configSet") String configSet, @QueryParam("file") String file) throws Exception; + + @GET + @Path("/{configSet}/sample") + @Operation( + summary = "Get a sample value and analysis for a field.", + tags = {"schema-designer"}) + FlexibleSolrJerseyResponse getSampleValue( + @PathParam("configSet") String configSet, + @QueryParam("field") String fieldName, + @QueryParam("uniqueKeyField") String idField, + @QueryParam("docId") String docId) + throws Exception; + + @GET + @Path("/{configSet}/collectionsForConfig") + @Operation( + summary = "List collections that use a given configSet.", + tags = {"schema-designer"}) + SchemaDesignerCollectionsResponse listCollectionsForConfig( + @PathParam("configSet") String configSet) throws Exception; + + @GET + @Path("/configs") + @Operation( + summary = "List all configSets available for schema design.", + tags = {"schema-designer"}) + SchemaDesignerConfigsResponse listConfigs() throws Exception; + + @POST + @Path("/{configSet}/add") + @Operation( + summary = "Add a new field, field type, or dynamic field to the schema being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse addSchemaObject( + @PathParam("configSet") String configSet, @QueryParam("schemaVersion") Integer schemaVersion) + throws Exception; + + @PUT + @Path("/{configSet}/update") + @Operation( + summary = "Update an existing field or field type in the schema being designed.", + tags = {"schema-designer"}) + SchemaDesignerResponse updateSchemaObject( + @PathParam("configSet") String configSet, @QueryParam("schemaVersion") Integer schemaVersion) + throws Exception; + + @PUT + @Path("/{configSet}/publish") + @Operation( + summary = "Publish the designed schema to a live configSet.", + tags = {"schema-designer"}) + SchemaDesignerPublishResponse publish( + @PathParam("configSet") String configSet, + @QueryParam("schemaVersion") Integer schemaVersion, + @QueryParam("newCollection") String newCollection, + @QueryParam("reloadCollections") @DefaultValue("false") Boolean reloadCollections, + @QueryParam("numShards") @DefaultValue("1") Integer numShards, + @QueryParam("replicationFactor") @DefaultValue("1") Integer replicationFactor, + @QueryParam("indexToCollection") @DefaultValue("false") Boolean indexToCollection, + @QueryParam("cleanupTemp") @DefaultValue("true") Boolean cleanupTempParam, + @QueryParam("disableDesigner") @DefaultValue("false") Boolean disableDesigner) + throws Exception; + + @POST + @Path("/{configSet}/analyze") + @Operation( + summary = "Analyze sample documents and suggest a schema.", + tags = {"schema-designer"}) + SchemaDesignerResponse analyze( + @PathParam("configSet") String configSet, + @QueryParam("schemaVersion") Integer schemaVersion, + @QueryParam("copyFrom") String copyFrom, + @QueryParam("uniqueKeyField") String uniqueKeyField, + @QueryParam("languages") List<String> languages, + @QueryParam("enableDynamicFields") Boolean enableDynamicFields, + @QueryParam("enableFieldGuessing") Boolean enableFieldGuessing, + @QueryParam("enableNestedDocs") Boolean enableNestedDocs) + throws Exception; + + @GET + @Path("/{configSet}/query") + @Operation( + summary = "Query the temporary collection used during schema design.", + tags = {"schema-designer"}) + FlexibleSolrJerseyResponse query(@PathParam("configSet") String configSet) throws Exception; Review Comment: [Q] Is "configSet" really the only query-param exposed here? What query does this API even execute, if it's not specified by users as a param? ########## solr/core/src/java/org/apache/solr/handler/configsets/DownloadConfigSet.java: ########## @@ -63,20 +63,34 @@ public Response downloadConfigSet(String configSetName) throws Exception { throw new SolrException( SolrException.ErrorCode.NOT_FOUND, "ConfigSet " + configSetName + " not found!"); } - return buildZipResponse(configSetService, configSetName); + return buildZipResponse(configSetService, configSetName, deriveDisplayName(configSetName)); + } + + // This is to support the schema designer's internal name and + // lets us not duplicate the download endpoint. + static String deriveDisplayName(String configSetName) { + if (configSetName.startsWith("._designer_")) { + return configSetName.substring("._designer_".length()); + } + return configSetName; } /** * Build a ZIP download {@link Response} for the given configset. * * @param configSetService the service to use for downloading the configset files - * @param configSetName the name of the configset to download + * @param configSetName the name of the configset to download (internal id) + * @param displayName the sanitized name to use in the Content-Disposition filename */ - public static Response buildZipResponse(ConfigSetService configSetService, String configSetName) + public static Response buildZipResponse( + ConfigSetService configSetService, String configSetName, String displayName) throws IOException { final byte[] zipBytes = zipConfigSet(configSetService, configSetName); + final String safeName = displayName.replaceAll("[^a-zA-Z0-9_\\-.]", "_"); + final String fileName = safeName + "_configset.zip"; return Response.ok((StreamingOutput) outputStream -> outputStream.write(zipBytes)) .type("application/zip") + .header("Content-Disposition", "attachment; filename=\"" + fileName + "\"") Review Comment: [Q] I saw the "Content-Disposition stuff in #4264, but you were able to remove it with more 'modern' JS I thought. (This commit in particular: https://github.com/apache/solr/pull/4264/changes/1cf5545f2d7cf679241034f9458cbaca9e8eb72b) Can you give a bit more context on why it's reappearing here? Or should it be gone from here as well? ########## solr/api/src/java/org/apache/solr/client/api/model/SchemaDesignerInfoResponse.java: ########## @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.model; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; +import java.util.List; + +/** Response body for the Schema Designer get-info endpoint. */ +@JsonInclude(JsonInclude.Include.NON_NULL) +public class SchemaDesignerInfoResponse extends SolrJerseyResponse { + + @JsonProperty("configSet") + public String configSet; + + /** Whether the configSet has a published (live) version. */ + @JsonProperty("published") + public Boolean published; + + @JsonProperty("schemaVersion") + public Integer schemaVersion; + + /** Collections currently using this configSet. */ + @JsonProperty("collections") + public List<String> collections; + + /** Number of sample documents stored for this configSet, if available. */ + @JsonProperty("numDocs") + public Integer numDocs; + + // --- designer settings --- + + @JsonProperty("languages") + public List<String> languages; + + @JsonProperty("enableFieldGuessing") Review Comment: [0] A lot of these fields are duplicated in other response objects? Is it worth pulling them into a parent class that then gets sub-classed, to avoid the duplication? ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesigner.java: ########## @@ -240,7 +240,7 @@ public void updateFileContents(SolrQueryRequest req, SolrQueryResponse rsp) } byte[] data; - try (InputStream in = extractSingleContentStream(req, true).getStream()) { + try (InputStream in = extractSingleContentStream(true).getStream()) { Review Comment: [-1] `extractSingleContentStream` is a very "v1-API" approach to getting the request body. The more v2/JAX-RS friendly way to do this is do put a `@RequestBody InputStream newFileContents` in the interface method signature (and here as well, obviously, minus the annotation). I'll avoid commenting on this elsewhere, but other places in the class are using the same v1 idiom and should be updated. ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesigner.java: ########## @@ -772,57 +767,80 @@ 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( + Map<String, Object> errorResponse = new HashMap<>(); + addErrorToResponse( + mutableId, 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; + "Failed to re-index sample documents after schema updated."), + errorsDuringIndexing, + errorResponse, + "Failed to re-index sample documents after schema updated."); + return buildFlexibleResponse(errorResponse); } // execute the user's query against the temp collection - QueryResponse qr = cloudClient().query(mutableId, req.getParams()); - rsp.getValues().addAll(qr.getResponse()); + QueryResponse qr = cloudClient().query(mutableId, solrQueryRequest.getParams()); + Map<String, Object> responseMap = new HashMap<>(); + qr.getResponse() + .forEach( + (name, val) -> { + if ("response".equals(name) && val instanceof SolrDocumentList) { + // SolrDocumentList extends ArrayList, so Jackson would serialize it as a plain + // array, losing numFound/start metadata that the UI expects at data.response.docs + SolrDocumentList docList = (SolrDocumentList) val; + Map<String, Object> responseObj = new HashMap<>(); + responseObj.put("numFound", docList.getNumFound()); + responseObj.put("start", docList.getStart()); + responseObj.put("docs", new ArrayList<>(docList)); + responseMap.put(name, responseObj); + } else { + responseMap.put(name, val); + } + }); + return buildFlexibleResponse(responseMap); } /** * Return the diff of designer schema with the source schema (either previously published or the * copyFrom). */ - @EndPoint(method = GET, path = "/schema-designer/diff", permission = CONFIG_READ_PERM) - public void getSchemaDiff(SolrQueryRequest req, SolrQueryResponse rsp) throws IOException { - final String configSet = getRequiredParam(CONFIG_SET_PARAM, req); + @Override + @PermissionName(CONFIG_READ_PERM) + public SchemaDesignerSchemaDiffResponse getSchemaDiff(String configSet) throws Exception { + requireNotEmpty(CONFIG_SET_PARAM, configSet); SchemaDesignerSettings settings = getMutableSchemaForConfigSet(configSet, -1, null); // diff the published if found, else use the original source schema String sourceSchema = configExists(configSet) ? configSet : settings.getCopyFrom(); - Map<String, Object> response = new HashMap<>(); - response.put( - "diff", ManagedSchemaDiff.diff(loadLatestSchema(sourceSchema), settings.getSchema())); - response.put("diff-source", sourceSchema); + SchemaDesignerSchemaDiffResponse response = Review Comment: [0] `final var response = ...` generally makes this look much nicer IMO. ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesigner.java: ########## @@ -1230,6 +1258,66 @@ protected void addErrorToResponse( } } + protected void addErrorToResponse( Review Comment: [0] I'm a little confused about the purpose of this method. We're converting indexing errors into a response that can be returned to the user? It takes a SolrException, but also a Map of Throwables keyed off of generic Object? I know this isn't public, but maybe it's worth some javadocs anyway as the signature is pretty confusing at a glance. ########## solr/core/src/test/org/apache/solr/handler/configsets/DeleteConfigSetAPITest.java: ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.handler.configsets; + +import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito; +import static org.mockito.Mockito.mock; + +import org.apache.solr.SolrTestCase; +import org.apache.solr.common.SolrException; +import org.apache.solr.core.CoreContainer; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Unit tests for {@link DeleteConfigSet}. + * + * <p>Note: This test focuses on input validation. Full deletion workflow is tested in integration + * tests like {@code TestConfigSetsAPI} since actual deletion requires ZooKeeper interaction. + */ +public class DeleteConfigSetAPITest extends SolrTestCase { Review Comment: [Q] This is a nice unit test and all, but why is it in this PR? This PR is about Schema-Designer and makes only the tiniest one-line change to DeleteConfigSet... -- 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]
