gerlowskija commented on code in PR #4203: URL: https://github.com/apache/solr/pull/4203#discussion_r3017014391
########## changelog/unreleased/SOLR-18152-migrate-schemadesignerapi-to-v2-annotations.yml: ########## @@ -0,0 +1,8 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: Migrate Schema Designer API to JAX-RS. Fix bug preventing analysis of sample documents from running. Review Comment: [-0] This is user-facing text - why would a user care what framework we use in our server-side code? For most APIs I generally make the changelog entry about the new SolrJ SolrRequest/SolrResponse that'll be generated. That may make sense here as well? But also - these APIs are so UI-centric that it's hard to imagine anyone ever calling them from Java code. Without a use case there, even that sort of a changelog entry might not make sense... Maybe the changelog here should "just" highlight the bug fix and give a bit more detail on that? ########## 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: [Q] [This comment](https://github.com/apache/solr/pull/4203#issuecomment-4153829021) aims to summarize the cosmetic changes you've made here. But that comment itself looks off somehow. Looking at the actual code here, each API has a `/schema-designer` path prefix that just isn't mentioned in the proposal at all. Further the "Before" column doesn't appear to actually represent the APIs as they exist today on `main`. Rather the column appears to represent some intermediate state in your PR's development (i.e. after you moved the configset param in to the path). Which is kindof misleading in terms of understanding the "net" change being proposed. Was this intentional? (e.g. `/schema-designer` was omitted for concise-ness) Or am I maybe missing something here? -- 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]
