gerlowskija commented on code in PR #4203: URL: https://github.com/apache/solr/pull/4203#discussion_r3017706348
########## solr/api/src/java/org/apache/solr/client/api/endpoint/SchemaDesignerApi.java: ########## @@ -0,0 +1,184 @@ +/* + * 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 static org.apache.solr.client.api.util.Constants.RAW_OUTPUT_PROPERTY; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.extensions.Extension; +import io.swagger.v3.oas.annotations.extensions.ExtensionProperty; +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.Produces; +import jakarta.ws.rs.QueryParam; +import jakarta.ws.rs.core.Response; +import java.util.List; +import org.apache.solr.client.api.model.FlexibleSolrJerseyResponse; +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"}) + FlexibleSolrJerseyResponse getInfo(@PathParam("configSet") String configSet) throws Exception; Review Comment: [-0] Why use 'FlexibleSolrJerseyResponse' here as opposed to some more tailored POJO type? AFAICT from the implementing code, the response has 4 or 5 top-level properties and modeling as a POJO should be pretty doable. Ditto for several of the other endpoints here as well. ########## 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 Review Comment: [0] Naming scheme used elsewhere is that the interface w/ all the JAX-RS annotations is named FooApi, and the actual implementing class is "just" Foo. So by that logic this class should probably be: `SchemaDesigner` ########## solr/core/src/java/org/apache/solr/handler/designer/DefaultSchemaSuggester.java: ########## @@ -395,11 +394,7 @@ protected boolean isMultiValued(final List<Object> sampleValues) { } protected Map<String, String> guessFieldProps( - String fieldName, - FieldType fieldType, - List<Object> sampleValues, - boolean isMV, - IndexSchema schema) { + String fieldName, FieldType fieldType, boolean isMV, IndexSchema schema) { Review Comment: [0] Is this just an opportunistic refactor that removes an unused parameter (i.e. 'sampleValues')? I'm fine with this one, but in future I'd prefer if you leave these out of these "purely JAX-RS migration" PRs. Just makes the diff harder to read and opens the door to subtle bugs slipping in, is all. ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -431,17 +452,19 @@ public void addSchemaObject(SolrQueryRequest req, SolrQueryResponse rsp) Map<String, Object> response = buildResponse(configSet, schema, null, configSetHelper.retrieveSampleDocs(configSet)); response.put(action, objectName); - rsp.getValues().addAll(response); + return buildFlexibleResponse(response); } - @EndPoint(method = PUT, path = "/schema-designer/update", permission = CONFIG_EDIT_PERM) - public void updateSchemaObject(SolrQueryRequest req, SolrQueryResponse rsp) - throws IOException, SolrServerException { - final String configSet = getRequiredParam(CONFIG_SET_PARAM, req); - final String mutableId = checkMutable(configSet, req); + @Override + @PermissionName(CONFIG_EDIT_PERM) + public FlexibleSolrJerseyResponse updateSchemaObject(String configSet, Integer schemaVersion) + throws Exception { + requireNotEmpty(CONFIG_SET_PARAM, configSet); + requireSchemaVersion(schemaVersion); + final String mutableId = checkMutable(configSet, schemaVersion); // Updated field definition is in the request body as JSON - Map<String, Object> updateField = readJsonFromRequest(req); + Map<String, Object> updateField = readJsonFromRequest(); Review Comment: [-0] Ditto, re: using a `@RequestBody` parameter to represent this parameter. ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -1260,8 +1316,7 @@ protected Map<String, Object> readJsonFromRequest(SolrQueryRequest req) throws I return (Map<String, Object>) json; } - protected void addSettingsToResponse( - SchemaDesignerSettings settings, final Map<String, Object> response) { + void addSettingsToResponse(SchemaDesignerSettings settings, final Map<String, Object> response) { Review Comment: [Q] Why the visibility-changes on some of these methods? ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -222,14 +237,15 @@ public void getFileContents(SolrQueryRequest req, SolrQueryResponse rsp) throws } String stringData = data != null && data.length > 0 ? new String(data, StandardCharsets.UTF_8) : ""; - rsp.getValues().addAll(Collections.singletonMap(file, stringData)); + return buildFlexibleResponse(Collections.singletonMap(file, stringData)); Review Comment: [0] You're porting the existing functionality correctly, but the whole approach of encoding the file data in a larger response structure seems a little fraught. We have a number of other APIs that aim to return "raw" file contents: a configset download API, some of the filestore APIs, our "zookeeper-read-proxy" API, etc. At some point in the future (not this PR) we should probably switch this over to work similarly ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -774,38 +807,60 @@ 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) { Review Comment: [Q] Can you add some context on this part of the PR please? ########## solr/core/src/java/org/apache/solr/handler/designer/SampleDocuments.java: ########## @@ -56,7 +56,7 @@ public List<SolrInputDocument> appendDocs( return id != null && !ids.contains(id); // doc has ID, and it's not already in the set }) - .collect(Collectors.toList()); + .toList(); Review Comment: Ditto, re: unrelated tweaks/refactors making this harder to review and vet. ########## solr/core/src/java/org/apache/solr/handler/designer/DefaultSampleDocumentsLoader.java: ########## @@ -177,6 +170,19 @@ protected List<SolrInputDocument> loadJsonLines( return docs.stream().map(JsonLoader::buildDoc).collect(Collectors.toList()); } + private void parseStringToJson(List<Map<String, Object>> docs, String line) throws IOException { + line = line.trim(); + if (line.startsWith("{") && line.endsWith("}")) { Review Comment: [Q] Previous version had `!line.isEmpty()` condition here. Is there a reason that was removed? ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -386,9 +402,10 @@ protected Map<String, Integer> listEnabledConfigs() throws IOException { return configs; } - @EndPoint(method = GET, path = "/schema-designer/download/*", permission = CONFIG_READ_PERM) - public void downloadConfig(SolrQueryRequest req, SolrQueryResponse rsp) throws IOException { - final String configSet = getRequiredParam(CONFIG_SET_PARAM, req); + @Override + @PermissionName(CONFIG_READ_PERM) + public Response downloadConfig(String configSet) throws Exception { Review Comment: [-1] Currently, the preferred way to support these "stream arbitrary content back" APIs in JAX-RS is to have them return a `StreamingOutput` object. See [SOLR-17562](https://issues.apache.org/jira/browse/SOLR-17562) for some discussion and context on this. Check out ZooKeeperReadApis and its implementing class - those are returning configset bytes very similarly to what this method should be doing so some code or pattern reuse there should be very do-able! ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -1106,7 +1162,7 @@ protected long waitToSeeSampleDocs(String collectionName, long numAdded) return numFound; } - protected Map<String, Object> buildResponse( + Map<String, Object> buildResponse( Review Comment: [Q] Why the visibility change? ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -407,21 +424,25 @@ public void downloadConfig(SolrQueryRequest req, SolrQueryResponse rsp) throws I throw new IOException("Error reading config from ZK", SolrZkClient.checkInterrupted(e)); } - ContentStreamBase content = - new ContentStreamBase.ByteArrayStream( - configSetHelper.downloadAndZipConfigSet(configId), - configSet + ".zip", - "application/zip"); - rsp.add(RawResponseWriter.CONTENT, content); + final byte[] zipBytes = configSetHelper.downloadAndZipConfigSet(configId); + // Sanitize configSet to safe filename characters to prevent header injection + final String safeConfigSet = configSet.replaceAll("[^a-zA-Z0-9_\\-.]", "_"); + final String fileName = safeConfigSet + "_configset.zip"; + return Response.ok((StreamingOutput) outputStream -> outputStream.write(zipBytes)) + .type("application/zip") + .header("Content-Disposition", "attachment; filename=\"" + fileName + "\"") + .build(); } - @EndPoint(method = POST, path = "/schema-designer/add", permission = CONFIG_EDIT_PERM) - public void addSchemaObject(SolrQueryRequest req, SolrQueryResponse rsp) - throws IOException, SolrServerException { - final String configSet = getRequiredParam(CONFIG_SET_PARAM, req); - final String mutableId = checkMutable(configSet, req); + @Override + @PermissionName(CONFIG_EDIT_PERM) + public FlexibleSolrJerseyResponse addSchemaObject(String configSet, Integer schemaVersion) + throws Exception { + requireNotEmpty(CONFIG_SET_PARAM, configSet); + requireSchemaVersion(schemaVersion); + final String mutableId = checkMutable(configSet, schemaVersion); - Map<String, Object> addJson = readJsonFromRequest(req); + Map<String, Object> addJson = readJsonFromRequest(); Review Comment: [-0] As mentioned elsewhere, if this API requires a request-body that needs to be reflected in the method parameters. ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -503,14 +526,25 @@ public void updateSchemaObject(SolrQueryRequest req, SolrQueryResponse rsp) addErrorToResponse(mutableId, solrExc, errorsDuringIndexing, response, updateError); response.put("rebuild", needsRebuild); - rsp.getValues().addAll(response); + return buildFlexibleResponse(response); } - @EndPoint(method = PUT, path = "/schema-designer/publish", permission = CONFIG_EDIT_PERM) - public void publish(SolrQueryRequest req, SolrQueryResponse rsp) - throws IOException, SolrServerException { - final String configSet = getRequiredParam(CONFIG_SET_PARAM, req); - final String mutableId = checkMutable(configSet, req); + @Override + @PermissionName(CONFIG_EDIT_PERM) + public FlexibleSolrJerseyResponse publish( + String configSet, + Integer schemaVersion, + String newCollection, + Boolean reloadCollections, Review Comment: [-1] This Boolean param will be 'null' if not provided, which I believe will trigger an NPE on L590 where it's used in a conditional (i.e. `if (reloadCollections) {`). At least, that's my understanding of what would happen at runtime. Have you tested this? Assuming I'm not off-base on this one, it's probably worth checking the other Integer/Boolean params here as well to make sure they don't cause issues when absent. ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -965,8 +1021,8 @@ ManagedIndexSchema loadLatestSchema(String configSet) { return configSetHelper.loadLatestSchema(configSet); } - protected ContentStream extractSingleContentStream(final SolrQueryRequest req, boolean required) { - Iterable<ContentStream> streams = req.getContentStreams(); + protected ContentStream extractSingleContentStream(boolean required) { + Iterable<ContentStream> streams = solrQueryRequest.getContentStreams(); Review Comment: [-1] The JAX-RS way to get at the "body" of an HTTP request is to use the `@RequestBody` parameter on a method param of type `InputStream`, `String`, `Object`, etc. IMO that's what we should do here, rather than trying to pull a ContentStream off of the SolrQueryRequest (which are all very "v1" concepts) ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java: ########## @@ -567,10 +599,8 @@ && zkStateReader().getClusterState().hasCollection(newCollection)) { // create new collection Map<Object, Throwable> errorsDuringIndexing = null; if (StrUtils.isNotNullOrEmpty(newCollection)) { - int numShards = req.getParams().getInt("numShards", 1); - int rf = req.getParams().getInt("replicationFactor", 1); - configSetHelper.createCollection(newCollection, configSet, numShards, rf); - if (req.getParams().getBool(INDEX_TO_COLLECTION_PARAM, false)) { + configSetHelper.createCollection(newCollection, configSet, numShards, replicationFactor); Review Comment: [-1] Ditto, re: triggering an NPE if 'numShards', 'replicationFactor', etc. aren't specified. The existing code uses particular defaults (`1`) here as well that we should probably be making sure the newer code uses ########## solr/api/src/java/org/apache/solr/client/api/endpoint/SchemaDesignerApi.java: ########## @@ -0,0 +1,184 @@ +/* + * 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 static org.apache.solr.client.api.util.Constants.RAW_OUTPUT_PROPERTY; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.extensions.Extension; +import io.swagger.v3.oas.annotations.extensions.ExtensionProperty; +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.Produces; +import jakarta.ws.rs.QueryParam; +import jakarta.ws.rs.core.Response; +import java.util.List; +import org.apache.solr.client.api.model.FlexibleSolrJerseyResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** V2 API definitions for the Solr Schema Designer. */ +@Path("/schema-designer") Review Comment: I'm going to largely skip review of this file for now given some of the uncertainties above, but happy to review those later on. ########## solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerConfigSetHelper.java: ########## @@ -959,9 +934,9 @@ protected ManagedIndexSchema restoreLanguageSpecificObjectsAndFiles( List<SchemaField> addDynFields = Arrays.stream(copyFromSchema.getDynamicFields()) - .filter(df -> langFieldTypeNames.contains(df.getPrototype().getType().getTypeName())) - .filter(df -> !existingDynFields.contains(df.getPrototype().getName())) .map(IndexSchema.DynamicField::getPrototype) + .filter(prototype -> langFieldTypeNames.contains(prototype.getType().getTypeName())) Review Comment: [Q] What's the rationale for moving this down below the `map(...)`. It seems unrelated to the JAX-RS migration unless I'm missing something? -- 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]
