You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "gerlowskija (via GitHub)" <gi...@apache.org> on 2023/07/18 20:26:07 UTC
[GitHub] [solr] gerlowskija opened a new pull request, #1793: SOLR-16825: Generate v2 API SolrRequest bindings
gerlowskija opened a new pull request, #1793:
URL: https://github.com/apache/solr/pull/1793
https://issues.apache.org/jira/browse/SOLR-16825
# Description
SolrJ contains many SolrRequest implementations (which correspond to individual Solr APIs), but keeping them at parity and in sync with API changes made to solr-core has proved difficult and time consuming. Despite our best efforts, SolrJ lags 'core' on many APIs and lacks parity entirely on others. And that's not even considering Solr's v2 API surface, which SolrJ doesn't even attempt to cover.
We can use code-generation to solve this problem. SOLR-16346 gave us the ability to generate an "OpenAPI Spec" for Solr - a comprehensive, machine-readable description of our v2 APIs. We should start leveraging this spec, using it with one of the many OpenAPI-compatible code generators out there, to produce `SolrRequest` implementations for us "for free".
# Solution
The solution presented in this PR integrates with the [OpenAPI Generator](https://openapi-generator.tech/) tooling to generate `SolrRequest` and `SolrResponse` pairs for each v2 API. Some notes on the implementation:
1. Our OpenAPI Spec ("OAS") has historically been produced by "solr-core", since that's where the API code itself lived. This was a problem for generating SolrJ code unfortunately: "core" depends on "solrj", so the OpenAPI spec wasn't even available until _after_ the solrj build had already finished! To address this, this PR proposes we move our OpenAPI integration to a new gradle module called 'solrj-model'. This new gradle module holds interfaces and request/response POJOs needed to represent our APIs and generate an OpenAPI spec.
2. 'solrj' depends on the new 'solrj-model' dependency and uses a new task, `openApiGenerate` to produce the generated SolrJ classes. SolrJ's `compileJava` task now "depends on" `openApiGenerate`, so this should all integrate pretty seamlessly with any IDE that loads Solr as a gradle project.
3. Currently the generated SolrJ classes use jackson-databind to serialize requests to Solr and deserialize responses. This avoids the NamedList-inspection prevalent elsewhere in SolrJ. I know we've shied away from a direct Jackson dependency in the past, but I think the idea is worth considering given the huge amount Jackson would be getting us "for free" with this approach. If there aren't strong objections, I'll plan on shading the Jackson dependency so users wouldn't have to worry about version conflicts with their own code.
4. This PR only adds a few v2 API interfaces to 'solrj-model', as a way to control scope and exhibit the approach without bloating the PR. Once the main idea has been proven, subsequent PRs can add in more APIs.
5. The generated code is _NOT_ committed to version control, as that appears to be an anti-pattern. (At least, from the poking around I did online).
# Tests
Existing tests continue to pass.
I don't think we should add new tests for the generate code. (Openapi-generator does support this as an option, but the tests that it produces don't provide tons of value IMO.). Rather, I think we'd be better served modifying existing solr-solrj and solr-core tests to randomize between the v1 and v2 SolrRequest implementations, where both are available.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1793:
URL: https://github.com/apache/solr/pull/1793#discussion_r1275177471
##########
solr/solrj/build.gradle:
##########
@@ -15,13 +15,82 @@
* limitations under the License.
*/
+plugins {
+ id "org.openapi.generator" version "6.6.0"
+}
apply plugin: 'java-library'
+apply plugin: 'com.diffplug.spotless'
+
+import com.diffplug.gradle.spotless.JavaExtension
+
description = 'Solrj - Solr Java Client'
+configurations {
+ openApiSpecFile {
+ canBeConsumed = false
+ }
+ generatedCode
+}
+
+tasks.register("copySpec", Copy) {
+ from("../solrj-model/build/generated/openapi")
+ into(layout.buildDirectory.dir("openapispec"))
+}
+
+def generatedExt = new JavaExtension(spotless)
+project.ext.spotlessJavaSetup.execute(generatedExt)
+generatedExt.target("build/generated/src/**/*.java")
+def generatedSpotlessTask = generatedExt.createIndependentApplyTask("generatedSpotless")
+generatedSpotlessTask.group("build")
+generatedSpotlessTask.description("Apply formatting for generated code")
+
+openApiGenerate {
+ generatorName = "java"
+ // TODO - pretty hacky to specify inputSpec this way...ideally I'd just rely on a property defined in
+ // solrj-model, as below, but it doesn't appear to be working
+ //inputSpec = project(":solr:solrj-model").ext.openapispecfile
+ inputSpec = "${buildDir}/openapispec/openapi.json"
+
+ // Add 'debugModels: ""' or 'debugOperations: ""' to get the JSON input to mustache templating for those components
+ globalProperties.set([modelDocs: "false", apis: "", models: ""])
+ templateDir = "$projectDir/src/resources/java-template"
+ apiPackage = "org.apache.solr.client.solrj.request"
+ modelPackage = "org.apache.solr.client.solrj.model"
+ outputDir = "${buildDir}/generated/"
+ generateApiTests = false
+ generateModelTests = false
+}
+
+tasks.getByName("copySpec").dependsOn configurations.getByName("openApiSpecFile")
+tasks.getByName("openApiGenerate").dependsOn tasks.copySpec
+tasks.getByName("openApiGenerate").finalizedBy generatedSpotlessTask
+generatedSpotlessTask.dependsOn tasks.openApiGenerate
+
+sourceSets {
+ main {
+ java {
+ srcDir "${buildDir}/generated/src/main/java"
+ }
+ }
+}
+tasks.compileJava.dependsOn(configurations.generatedCode)
+
+
dependencies {
+ openApiSpecFile(project(path: ":solr:solrj-model", configuration: "openapiSpec"))
+ generatedCode files("${buildDir}/generated/main/java") {
+ builtBy tasks.openApiGenerate
+ }
+
+ // TODO Most likely temporary, due to historical pushback on adding Jackson deps to solrj, but it does _hugely_
+ // simplify the parsing code and there are new compelling reasons, given the claims around CBOR performance relative
+ // to JSON, javabin, etc.
+ implementation 'com.fasterxml.jackson.core:jackson-databind' // Came from a testImplementation line below.
Review Comment:
Great!
In terms of how to handle this, do you think it'd be better to shade the Jackson dep (increasing solrj's size but avoiding the jackson-dep-conflict issue), or include Jackson as a "normal" (i.e. unshaded) dependency?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16825: Generate v2 API SolrRequest bindings [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-2080334301
Quoting me:
> At least the use of Jackson is optional. Which is to say, if there is a client that is just doing index/query then it need not include our new "api" module either
I have found this to be false. Locally I deployed SolrJ to a service and forgot the "api" JAR, and there was a ClassNotFound on ReflectWritable, which is sad because it's just a marker interface.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1654436038
> My motivation is to provide an optimal experience to users -- a single JAR.
I'm not sure I understand -- SolrJ has never been a single jar. It uses a bunch of Jetty libs, SLF4J, Apache HttpComponents, Jackson (pending, with this PR). Do you see the 'api' dep being more onerous than those in some way? Or are you envisioning a future where SolrJ is truly dependency-less?
> Google search turned up this promising information
Thanks for the link! Actually, we're already doing something similar to the accepted answer here, in that we're declaring compileJava to be dependent on our generation task (openApiGenerate).
But the applicability stops there unfortunately: it's just two very different kinds of generation.
The SO question is trying to do FMPP generation, which essentially acts like a C-preprocessor and does some primitive string-manipulation and macro-expansion. The input is raw source files - or really any text you feed it, it doesn't even have to be code. OTOH, the swagger-gradle-plugin we're using requires already-compiled class files as input. They're just very different animals.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1793:
URL: https://github.com/apache/solr/pull/1793#discussion_r1270165715
##########
solr/solrj/src/java/org/apache/solr/common/util/ModelConstants.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.common.util;
+
+// TODO This probably shouldn't exist in a final product, but it's fine for a POC
Review Comment:
Our norm has been the magic string "nocommit" which we prevent merging but I see you use "TODO"... could go unnoticed as we don't have tooling to point these out. I use "TODO" in a loose way but here you meant nocommit I think
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1652388788
> [Houston] SolrJ core also doesn't need to depend on this solrj-model or solrj-api module, so that doesn't need to be opt-in or opt-out
😬 As things are today, SolrJ **is** dependent on the 'api' module. The generated SolrRequest files use POJO classes that live in 'api' to represent request and response bodies.
As an example, here's the ctor of the generated SolrRequest class called 'AddReplicaProperty'. The `AddReplicaPropertyRequestBody` class is defined in the 'api' module.
```
public AddReplicaProperty(
String collName, String shardName, String replicaName, String propName) {
super(
SolrRequest.METHOD.valueOf("PUT"),
"/collections/{collName}/shards/{shardName}/replicas/{replicaName}/properties/{propName}"
.replace("{" + "collName" + "}", collName)
.replace("{" + "shardName" + "}", shardName)
.replace("{" + "replicaName" + "}", replicaName)
.replace("{" + "propName" + "}", propName));
...<snip>...
this.requestBody = new AddReplicaPropertyRequestBody();
}
```
----
We don't have to do this if we don't want to. We can drop the solrj->api dependency if we really want - we'd just need to tweak our openapi-generator invocation to also generate "model" classes that are near duplicates of the POJOs that already live in the 'api' submodule. I actually had that working earlier on in this POC but dropped it because at the time I didn't see a reason to generate classes that were such close duplicates of Java code we already had. If you guys think generating models makes sense though, I'm OK with reversing course.
Would we really need to make the 'api' module "opt-in" for 9.x? I'm sure I'm showing my naiveté here, but client libraries change their deps all the time in non-major releases, don't they? What benefit does "opt-in" get our users?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1655987671
> The "api" module could depend-on SolrJ, and thus the Jackson stuff added could be in "api" as well. No?
If the "api" module were to have the API classes and the generated code, we would need multiple class generation steps, so that the API classes would be compiled, then the generated code would be generated from those class files, which would then be compiled. That's definitely more complicated than the workflow we have so far.
> OTOH, the swagger-gradle-plugin we're using requires already-compiled class files as input.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1661072747
I think I'm not going to attempt what I proposed.
At least the use of Jackson is optional. Which is to say, if there is a client that is just doing index/query then it need not include our new "api" module either. We still have our current hand-written APIs for typical administrative things too. I could imagine 10.0 embracing the new API & Jackson but what I said (about Jackson & "api" being kind of optional) is for 9.x. I don't think we should bother shading Jackson unless we get input to the contrary. If we don't use tons of Jackson's surface area, might as well just expose the dependency and let the user choose the version they want, probably a future version that doesn't even exist right now.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1652481090
> Would we really need to make the 'api' module "opt-in" for 9.x? I'm sure I'm showing my naiveté here, but client libraries change their deps all the time in non-major releases, don't they? What benefit does "opt-in" get our users?
I don't think it's possible to make the `api` module "opt-in". This is a first class feature, that's going to be intertwined with SolrJ.
> 😬 As things are today, SolrJ **is** dependent on the 'api' module. The generated SolrRequest files are copied over to SolrJ and built into that jar, but those SolrRequest files rely on POJO classes from 'api' to represent the request and response bodies.
Ahh that makes sense, I guess having `api` be a dependency is of very little consequence.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1654677473
> So the part used by users, being the actual clients that is generated code, will be packaged in SolrJ core (not a separate solrj module), so it can't really be opt-out.
It can't be generated into the "api" module? Forgive me if this is a stupid question or already covered... I'm new to this.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1654744305
> > So the part used by users, being the actual clients that is generated code, will be packaged in SolrJ core (not a separate solrj module), so it can't really be opt-out.
>
> It can't be generated into the "api" module? Forgive me if this is a stupid question or already covered... I'm new to this.
Correct, the generated classes import stuff from SolrJ, so they have to be generated in SolrJ.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1656362394
If I spend some time figuring out how to iterate on this PR to keep SolrJ free of this stuff (particularly Jackson) by solving the "multi-step code generation" or whatever to keep "api" separate (and with no inward dependency from SolrJ) -- would that be appreciated? If it's too ugly / awkward to maintain, we don't have to keep it. I think there's value in keeping SolrJ more focused on being a useful client for query & indexing that doesn't require Jackson... and having an "api" module on the side for doing all the other (often "admin") requests into Solr.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1654467889
Okay fine. What's *new* here is we are insisting that a SolrJ client be two JARs from the Solr project, in additional to dependency JARs. Not ideal but if it's too hard to combine then okay. I don't like that it exposes internal structure project build matters to the user/client who shouldn't care / observe it from their perspective.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1652161175
> We could move the -API suffix to the interfaces and give the actual implementations an "-Impl" suffix
Yes I think we should do that. If our existing "API" classes *implement* the API, and we are introducing a new interface distinction that previously didn't exist then let's incorporate "Impl" into the name of the existing classes. Even if it means a bunch of renames. The "Impl" suffix is kinda common in such approaches.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1793:
URL: https://github.com/apache/solr/pull/1793#discussion_r1275180268
##########
solr/solrj-model/src/test/org/apache/solr/model/DummyTest.java:
##########
@@ -0,0 +1,11 @@
+package org.apache.solr.model;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.junit.Test;
+
+public class DummyTest extends SolrTestCaseJ4 {
Review Comment:
I initially added it as a placeholder when creating the gradle submodule, so that I could make sure some tasks (i.e. `compileTest`, `test` worked. But it shouldn't be here anymore, removing...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1652129601
One thing I don't think anyone has commented on yet is the structure of the generated SolrRequest classes - do those look reasonable as-is, or is there something missing? (e.g. Factory or builder methods like we have in CollectionAdminRequest?). I guess there's no pressure to lock in the exact SolrRequest code at this point, since the APIs are v2 and therefore experimental and subject to change anyways. But it feels remiss to not ask the question given the templates in this PR 🤷
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1654836277
> the generated classes import stuff from SolrJ, so they have to be generated in SolrJ
The "api" module could depend-on SolrJ, and thus the Jackson stuff added could be in "api" as well. No?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1640957209
`./gradlew clean solr:solrj:openApiGenerate` generates the v2 SolrRequest and SolrResponse objects and puts them in `solr/solrj/build/generated`.
That said, there's really no reason to run that command explicitly. It's a dependency-task for `compileJava` so it'll get triggered by an IDE or other build.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1656035802
+1 to what Houston said. There's no great way to have `api` depend on `solrj` without a solve for the "multiple class generation steps" problem. And if we find a way around that, we should just nuke 'api' altogether and move the code into SolrJ as David [suggested a few days back](https://github.com/apache/solr/pull/1793#issuecomment-1652534121).
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1654679695
I reviewed the SolrRequests and Responses and they look good. BTW I checked out the branch and generated them to look.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16825: Generate v2 API SolrRequest bindings [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1793:
URL: https://github.com/apache/solr/pull/1793#discussion_r1594858131
##########
solr/solrj/build.gradle:
##########
@@ -85,3 +94,73 @@ dependencies {
testImplementation 'org.apache.commons:commons-lang3'
testImplementation 'io.dropwizard.metrics:metrics-core'
}
+
+/**
+ * Java Code Generation for Solr API
+ */
+
+evaluationDependsOn(":solr:api")
+
+configurations {
+ openApiSpecFile {
+ canBeConsumed = false
+ }
+}
+
+ext {
+ generatedCodeDir = "${buildDir}/generated/"
+ javaTemplateDir = "${projectDir}/src/resources/java-template"
+}
+
+dependencies {
+ openApiSpecFile(project(path: ":solr:api", configuration: "openapiSpec"))
+}
+
+/**
+ * Code Generation task
+ */
+openApiGenerate {
+ generatorName = "java"
+ inputSpec = project(":solr:api").openApiSpecFile
+
+ // Add 'debugModels: ""' or 'debugOperations: ""' to get the JSON input to mustache templating for those components
+ globalProperties.set([apis: "", models: "false"])
+ templateDir = project.javaTemplateDir
+ apiPackage = "org.apache.solr.client.solrj.request"
+ outputDir = project.generatedCodeDir
+ generateApiTests = false
+ generateModelTests = false
+ generateApiDocumentation = false
+ generateModelDocumentation = false
+ additionalProperties = ["modelPackage": "org.apache.solr.client.api.model"]
+}
+
+tasks.openApiGenerate.dependsOn configurations.openApiSpecFile
+
+def generatedFiles = files("${project.generatedCodeDir}/src/main/java") {
+ builtBy tasks.openApiGenerate
+}
+
+/**
+ * Setup Spotless (Code formatting) for the generated java files
+ */
+def generatedExt = new JavaExtension(spotless)
+project.spotlessJavaSetup.execute(generatedExt)
+generatedExt.target(generatedFiles)
+def generatedSpotlessTask = generatedExt.createIndependentApplyTask("generatedSpotless")
+generatedSpotlessTask.group("build")
+generatedSpotlessTask.description("Apply formatting for generated code")
+
+tasks.openApiGenerate.finalizedBy generatedSpotlessTask
+
Review Comment:
Yeah agreed overall. I'm just concerned with build complexity and want to make recommendations to improve / simplify it, to include using a declarative style when possible. Assuming the `createIndependentApplyTask` piece is critical, nothing much to do 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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1652266356
+1 to your proposal Houston; I'm not a big fan of "Impl" either.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1793:
URL: https://github.com/apache/solr/pull/1793#discussion_r1275178612
##########
solr/solrj/src/java/org/apache/solr/common/util/ModelConstants.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.common.util;
+
+// TODO This probably shouldn't exist in a final product, but it's fine for a POC
Review Comment:
I am using it in a lot of places (but not always) as "NOCOMMIT". It's a bad habit, I know 😬
##########
solr/solrj/src/java/org/apache/solr/common/util/ModelConstants.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.common.util;
+
+// TODO This probably shouldn't exist in a final product, but it's fine for a POC
Review Comment:
I am using it in a lot of places (but not always) as "NOCOMMIT". It's a bad habit, I know 😬
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1653728521
> Then could we embed "api"/model if it's inextricably a part of SolrJ?
SolrJ needs to have the generated SolrRequest source code by the time 'compileJava' runs. To generate those classes, we need to generate the OpenAPI spec first. But to generate the OpenAPI spec, we need to have class files for the api/model classes already produced.
So to have everything in the same module, you'd need a way to tell gradle to compile some partition of its sourceSet, then run a bunch of other generation tasks, and then compile the remainder of its sourceSet. Maybe that's possible? If you can find a way to do it, I've got no objections.
But to a non-expert in gradle having separate modules feels like a reasonably intuitive way to represent the relationship between these two chunks of code though 🤷.
Can you elaborate at all on the benefits of avoiding the additional module? Is your concern around maintenance on the developer end? Complexity on the user end? Maybe there's something we can do to address those underlying concerns, even if we need to end up keeping the 'api' module around...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1656474286
What about having a SolrJ-Slim and SolrJ, the slim being the query/update interface.. Similar to our two Docker images?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1665751760
OK, thanks for closing the loop @dsmiley. If we're OK with things as they stand, I guess I'll start cleaning this up and aim to commit sometime next week then.
Once this PR lands, I'll have some follow-up work to extract API interfaces and POJOs over to the 'api' module. As more moves over to 'api', we'll have more generated SolrRequest implementations to look at, and I imagine that'll spin out into some improvements or fixes to the template as well.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1658408834
> I think there's value in keeping SolrJ more focused on being a useful client for query & indexing that doesn't require Jackson... and having an "api" module on the side for doing all the other (often "admin") requests into Solr.
I guess I just don't see a ton of user value in making that distinction. Is this all just to save users the pain of a Jackson version conflict, or the few extra MB it'd take to shade Jackson in SolrJ?
The way I see it, there aren't tons and tons of users out there who query and index, but never need to create a collection. So whatever division we make to isolate the Jackson dep will be meaningless to a majority of our users.
And the few users who _can_ take advantage of such a distinction, probably shouldn't anyway. Queries, etc. will probably be faster if they have Jackson available to use CBOR, the new transfer format Solr partially supports in 9.3. Depending how much you trust the [perf tests done here](https://issues.apache.org/jira/browse/SOLR-16812)), CBOR appears to be Solr's most efficient transfer format - a serious upgrade over javabin that anyone concerned about latency is going to want to try.
But that's just my take 🤷
> If I spend some time figuring out how to iterate on this PR to keep SolrJ free of this stuff (particularly Jackson) by solving the "multi-step code generation" or whatever to keep "api" separate (and with no inward dependency from SolrJ) -- would that be appreciated?
For sure! If you find a reasonably elegant way around the "multi-step code generation" thing, or find a way to avoid the extra module, I'm all for that. I've been trying to bait folks into double-checking my sketchy gradle work throughout 😛 (Thanks to Houston for all the help so far!)
As I said, I'm personally skeptical of the value prop in creating a new user-facing module just for the purpose of segregating our Jackson surface area. But if there's solid consensus around that, and you're willing to look into it, I'm open to being the minority there as well.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1654192331
My motivation is to provide an optimal experience to users -- a single JAR. I recognize there isn't a strict requirement here. For a how-to, a quick Google search turned up this promising information: https://stackoverflow.com/questions/29602095/gradle-how-to-generate-source-code-before-compilation-in-android-app
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1654598386
Alright - made some decent enough progress addressing review feedback, etc. that I'm taking this out of "Draft" mode!
There's still one big unanswered question that's come up so far: if we're adding Jackson to SolrJ do we want to add it as a "normal" dependency, or do we want to shade to save SolrJ users any potential version conflicts?
But of course there's still lots I'd appreciate feedback. Especially on the template for the generated files - do these SolrRequests and Responses look reasonable to folks?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16825: Generate v2 API SolrRequest bindings [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1793:
URL: https://github.com/apache/solr/pull/1793#discussion_r1581719562
##########
solr/solrj/build.gradle:
##########
@@ -85,3 +94,73 @@ dependencies {
testImplementation 'org.apache.commons:commons-lang3'
testImplementation 'io.dropwizard.metrics:metrics-core'
}
+
+/**
+ * Java Code Generation for Solr API
+ */
+
+evaluationDependsOn(":solr:api")
+
+configurations {
+ openApiSpecFile {
+ canBeConsumed = false
+ }
+}
+
+ext {
+ generatedCodeDir = "${buildDir}/generated/"
+ javaTemplateDir = "${projectDir}/src/resources/java-template"
+}
+
+dependencies {
+ openApiSpecFile(project(path: ":solr:api", configuration: "openapiSpec"))
+}
+
+/**
+ * Code Generation task
+ */
+openApiGenerate {
+ generatorName = "java"
+ inputSpec = project(":solr:api").openApiSpecFile
+
+ // Add 'debugModels: ""' or 'debugOperations: ""' to get the JSON input to mustache templating for those components
+ globalProperties.set([apis: "", models: "false"])
+ templateDir = project.javaTemplateDir
+ apiPackage = "org.apache.solr.client.solrj.request"
+ outputDir = project.generatedCodeDir
+ generateApiTests = false
+ generateModelTests = false
+ generateApiDocumentation = false
+ generateModelDocumentation = false
+ additionalProperties = ["modelPackage": "org.apache.solr.client.api.model"]
+}
+
+tasks.openApiGenerate.dependsOn configurations.openApiSpecFile
+
+def generatedFiles = files("${project.generatedCodeDir}/src/main/java") {
+ builtBy tasks.openApiGenerate
+}
+
+/**
+ * Setup Spotless (Code formatting) for the generated java files
+ */
+def generatedExt = new JavaExtension(spotless)
+project.spotlessJavaSetup.execute(generatedExt)
+generatedExt.target(generatedFiles)
+def generatedSpotlessTask = generatedExt.createIndependentApplyTask("generatedSpotless")
+generatedSpotlessTask.group("build")
+generatedSpotlessTask.description("Apply formatting for generated code")
+
+tasks.openApiGenerate.finalizedBy generatedSpotlessTask
+
Review Comment:
@gerlowskija this part of build.gradle is kind of sad. An ideal Gradle file is declarative yet this code is as imperative as can be. It need not be this way! BTW I played with ChatGPT briefly to convert this to a declarative style and it wasn't perfect but it was useful.
There is something missing here:
`generatedSpotlessTask.dependsOn(':checkJdkInternalsExportedToGradle')`
The point of that task is to get a helpful error about a build configuration issue. Without that, you get mysterious errors about Google Java Format not being able to access certain modules of Java that have not been exported. Perhaps there's a way for that to be applied in a general way so it doesn't need to be remembered; not sure.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16825: Generate v2 API SolrRequest bindings [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1793:
URL: https://github.com/apache/solr/pull/1793#discussion_r1588028324
##########
solr/solrj/build.gradle:
##########
@@ -85,3 +94,73 @@ dependencies {
testImplementation 'org.apache.commons:commons-lang3'
testImplementation 'io.dropwizard.metrics:metrics-core'
}
+
+/**
+ * Java Code Generation for Solr API
+ */
+
+evaluationDependsOn(":solr:api")
+
+configurations {
+ openApiSpecFile {
+ canBeConsumed = false
+ }
+}
+
+ext {
+ generatedCodeDir = "${buildDir}/generated/"
+ javaTemplateDir = "${projectDir}/src/resources/java-template"
+}
+
+dependencies {
+ openApiSpecFile(project(path: ":solr:api", configuration: "openapiSpec"))
+}
+
+/**
+ * Code Generation task
+ */
+openApiGenerate {
+ generatorName = "java"
+ inputSpec = project(":solr:api").openApiSpecFile
+
+ // Add 'debugModels: ""' or 'debugOperations: ""' to get the JSON input to mustache templating for those components
+ globalProperties.set([apis: "", models: "false"])
+ templateDir = project.javaTemplateDir
+ apiPackage = "org.apache.solr.client.solrj.request"
+ outputDir = project.generatedCodeDir
+ generateApiTests = false
+ generateModelTests = false
+ generateApiDocumentation = false
+ generateModelDocumentation = false
+ additionalProperties = ["modelPackage": "org.apache.solr.client.api.model"]
+}
+
+tasks.openApiGenerate.dependsOn configurations.openApiSpecFile
+
+def generatedFiles = files("${project.generatedCodeDir}/src/main/java") {
+ builtBy tasks.openApiGenerate
+}
+
+/**
+ * Setup Spotless (Code formatting) for the generated java files
+ */
+def generatedExt = new JavaExtension(spotless)
+project.spotlessJavaSetup.execute(generatedExt)
+generatedExt.target(generatedFiles)
+def generatedSpotlessTask = generatedExt.createIndependentApplyTask("generatedSpotless")
+generatedSpotlessTask.group("build")
+generatedSpotlessTask.description("Apply formatting for generated code")
+
+tasks.openApiGenerate.finalizedBy generatedSpotlessTask
+
Review Comment:
> An ideal Gradle file is declarative yet this code is as imperative as can be.
So this is imperative code that is creating declarative tasks. Which is something very well supported in Gradle.
If you want to share the chatGPT suggestions, I'm happy to take a look, but this was very hard to get working well in the first place, so I'd be surprised if chatGPT gives us something that works.
> There is something missing here:
`generatedSpotlessTask.dependsOn(':checkJdkInternalsExportedToGradle')`
Yes, but ideally we just fix `spotless.gradle` to not require that a spottless task be named a certain way. we can instead see all the tasks that are of type `SpotlessApply`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1793:
URL: https://github.com/apache/solr/pull/1793#discussion_r1270167635
##########
solr/solrj-model/src/test/org/apache/solr/model/DummyTest.java:
##########
@@ -0,0 +1,11 @@
+package org.apache.solr.model;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.junit.Test;
+
+public class DummyTest extends SolrTestCaseJ4 {
Review Comment:
Why was this added?
##########
solr/solrj/build.gradle:
##########
@@ -15,13 +15,82 @@
* limitations under the License.
*/
+plugins {
+ id "org.openapi.generator" version "6.6.0"
+}
apply plugin: 'java-library'
+apply plugin: 'com.diffplug.spotless'
+
+import com.diffplug.gradle.spotless.JavaExtension
+
description = 'Solrj - Solr Java Client'
+configurations {
+ openApiSpecFile {
+ canBeConsumed = false
+ }
+ generatedCode
+}
+
+tasks.register("copySpec", Copy) {
+ from("../solrj-model/build/generated/openapi")
+ into(layout.buildDirectory.dir("openapispec"))
+}
+
+def generatedExt = new JavaExtension(spotless)
+project.ext.spotlessJavaSetup.execute(generatedExt)
+generatedExt.target("build/generated/src/**/*.java")
+def generatedSpotlessTask = generatedExt.createIndependentApplyTask("generatedSpotless")
+generatedSpotlessTask.group("build")
+generatedSpotlessTask.description("Apply formatting for generated code")
+
+openApiGenerate {
+ generatorName = "java"
+ // TODO - pretty hacky to specify inputSpec this way...ideally I'd just rely on a property defined in
+ // solrj-model, as below, but it doesn't appear to be working
+ //inputSpec = project(":solr:solrj-model").ext.openapispecfile
+ inputSpec = "${buildDir}/openapispec/openapi.json"
+
+ // Add 'debugModels: ""' or 'debugOperations: ""' to get the JSON input to mustache templating for those components
+ globalProperties.set([modelDocs: "false", apis: "", models: ""])
+ templateDir = "$projectDir/src/resources/java-template"
+ apiPackage = "org.apache.solr.client.solrj.request"
+ modelPackage = "org.apache.solr.client.solrj.model"
+ outputDir = "${buildDir}/generated/"
+ generateApiTests = false
+ generateModelTests = false
+}
+
+tasks.getByName("copySpec").dependsOn configurations.getByName("openApiSpecFile")
Review Comment:
Why do this in a dynamic/reflection-like way; why not explicitly do this at the point that copySpec is declared?
##########
solr/solrj/src/java/org/apache/solr/common/util/ModelConstants.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.common.util;
+
+// TODO This probably shouldn't exist in a final product, but it's fine for a POC
Review Comment:
Out practice has been the magic string "nocommit" which we prevent merging but I see you use "TODO"... could go unnoticed as we don't have tooling to point these out. I use "TODO" in a loose way but here you meant nocommit I think
##########
solr/solrj/build.gradle:
##########
@@ -15,13 +15,82 @@
* limitations under the License.
*/
+plugins {
+ id "org.openapi.generator" version "6.6.0"
+}
apply plugin: 'java-library'
+apply plugin: 'com.diffplug.spotless'
+
+import com.diffplug.gradle.spotless.JavaExtension
+
description = 'Solrj - Solr Java Client'
+configurations {
+ openApiSpecFile {
+ canBeConsumed = false
+ }
+ generatedCode
+}
+
+tasks.register("copySpec", Copy) {
+ from("../solrj-model/build/generated/openapi")
+ into(layout.buildDirectory.dir("openapispec"))
+}
+
+def generatedExt = new JavaExtension(spotless)
+project.ext.spotlessJavaSetup.execute(generatedExt)
+generatedExt.target("build/generated/src/**/*.java")
+def generatedSpotlessTask = generatedExt.createIndependentApplyTask("generatedSpotless")
+generatedSpotlessTask.group("build")
+generatedSpotlessTask.description("Apply formatting for generated code")
+
+openApiGenerate {
+ generatorName = "java"
+ // TODO - pretty hacky to specify inputSpec this way...ideally I'd just rely on a property defined in
+ // solrj-model, as below, but it doesn't appear to be working
+ //inputSpec = project(":solr:solrj-model").ext.openapispecfile
+ inputSpec = "${buildDir}/openapispec/openapi.json"
+
+ // Add 'debugModels: ""' or 'debugOperations: ""' to get the JSON input to mustache templating for those components
+ globalProperties.set([modelDocs: "false", apis: "", models: ""])
+ templateDir = "$projectDir/src/resources/java-template"
+ apiPackage = "org.apache.solr.client.solrj.request"
+ modelPackage = "org.apache.solr.client.solrj.model"
+ outputDir = "${buildDir}/generated/"
+ generateApiTests = false
+ generateModelTests = false
+}
+
+tasks.getByName("copySpec").dependsOn configurations.getByName("openApiSpecFile")
+tasks.getByName("openApiGenerate").dependsOn tasks.copySpec
+tasks.getByName("openApiGenerate").finalizedBy generatedSpotlessTask
+generatedSpotlessTask.dependsOn tasks.openApiGenerate
+
+sourceSets {
+ main {
+ java {
+ srcDir "${buildDir}/generated/src/main/java"
+ }
+ }
+}
+tasks.compileJava.dependsOn(configurations.generatedCode)
+
+
dependencies {
+ openApiSpecFile(project(path: ":solr:solrj-model", configuration: "openapiSpec"))
+ generatedCode files("${buildDir}/generated/main/java") {
+ builtBy tasks.openApiGenerate
+ }
+
+ // TODO Most likely temporary, due to historical pushback on adding Jackson deps to solrj, but it does _hugely_
+ // simplify the parsing code and there are new compelling reasons, given the claims around CBOR performance relative
+ // to JSON, javabin, etc.
+ implementation 'com.fasterxml.jackson.core:jackson-databind' // Came from a testImplementation line below.
Review Comment:
I was certainly pushing back in years past but I can see there is a huge code maintenance win here so I'm in favor.
##########
solr/solrj/src/resources/java-template/pojo.mustache:
##########
@@ -0,0 +1,391 @@
+public class {{classname}} {
+
+ {{#vars}}
+ public static final String JSON_PROPERTY_{{nameInSnakeCase}} = "{{baseName}}";
+ {{#isContainer}}
+ private {{{datatypeWithEnum}}} {{name}}{{#defaultValue}} = {{{.}}}{{/defaultValue}};
+ {{/isContainer}}
+ {{^isContainer}}
+ {{#isDiscriminator}}protected{{/isDiscriminator}}{{^isDiscriminator}}private{{/isDiscriminator}} {{{datatypeWithEnum}}} {{name}}{{#defaultValue}} = {{{.}}}{{/defaultValue}};
+ {{/isContainer}}
+
+ {{/vars}}
+ public {{classname}}() {}
+
+ {{#vars}}
+ {{^isReadOnly}}
+ public {{classname}} {{name}}({{{datatypeWithEnum}}} {{name}}) {
+ this.{{name}} = {{name}};
+ return this;
+ }
+
+ {{/isReadOnly}}
+ /**
+ {{#description}}
+ * {{.}}
+ {{/description}}
+ {{^description}}
+ * Get {{name}}
+ {{/description}}
+ {{#minimum}}
+ * minimum: {{.}}
+ {{/minimum}}
+ {{#maximum}}
+ * maximum: {{.}}
+ {{/maximum}}
+ * @return {{name}}
+ {{#deprecated}}
+ * @deprecated
+ {{/deprecated}}
+ */
+{{#deprecated}}
+ @Deprecated
+{{/deprecated}}
+{{> jackson_annotations}} public {{{datatypeWithEnum}}} {{getter}}() {
+ return {{name}};
+ }
+
+ {{^isReadOnly}}
+{{> jackson_annotations}} public void {{setter}}({{{datatypeWithEnum}}} {{name}}) {
+ this.{{name}} = {{name}};
+ }
+ {{/isReadOnly}}
+
+ {{/vars}}
+ {{#parent}}
+ {{#allVars}}
+ {{#isOverridden}}
+ @Override
+ public {{classname}} {{name}}({{{datatypeWithEnum}}} {{name}}) {
+ {{#vendorExtensions.x-is-jackson-optional-nullable}}
+ this.{{setter}}(JsonNullable.<{{{datatypeWithEnum}}}>of({{name}}));
+ {{/vendorExtensions.x-is-jackson-optional-nullable}}
+ {{^vendorExtensions.x-is-jackson-optional-nullable}}
+ this.{{setter}}({{name}});
+ {{/vendorExtensions.x-is-jackson-optional-nullable}}
+ return this;
+ }
+
+ {{/isOverridden}}
+ {{/allVars}}
+ {{/parent}}
+ @Override
+ public boolean equals(Object o) {
+ {{#useReflectionEqualsHashCode}}
+ return EqualsBuilder.reflectionEquals(this, o, false, null, true);
+ {{/useReflectionEqualsHashCode}}
+ {{^useReflectionEqualsHashCode}}
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }{{#hasVars}}
+ {{classname}} {{classVarName}} = ({{classname}}) o;
+ return {{#vars}}{{#vendorExtensions.x-is-jackson-optional-nullable}}equalsNullable(this.{{name}}, {{classVarName}}.{{name}}){{/vendorExtensions.x-is-jackson-optional-nullable}}{{^vendorExtensions.x-is-jackson-optional-nullable}}{{#isByteArray}}Arrays{{/isByteArray}}{{^isByteArray}}Objects{{/isByteArray}}.equals(this.{{name}}, {{classVarName}}.{{name}}){{/vendorExtensions.x-is-jackson-optional-nullable}}{{^-last}} &&
+ {{/-last}}{{/vars}}{{#parent}} &&
+ super.equals(o){{/parent}};{{/hasVars}}{{^hasVars}}
+ return {{#parent}}super.equals(o){{/parent}}{{^parent}}true{{/parent}};{{/hasVars}}
+ {{/useReflectionEqualsHashCode}}
+ }{{#vendorExtensions.x-jackson-optional-nullable-helpers}}
+
+ private static <T> boolean equalsNullable(JsonNullable<T> a, JsonNullable<T> b) {
+ return a == b || (a != null && b != null && a.isPresent() && b.isPresent() && Objects.deepEquals(a.get(), b.get()));
+ }{{/vendorExtensions.x-jackson-optional-nullable-helpers}}
+
+ @Override
+ public int hashCode() {
+ {{#useReflectionEqualsHashCode}}
+ return HashCodeBuilder.reflectionHashCode(this);
+ {{/useReflectionEqualsHashCode}}
+ {{^useReflectionEqualsHashCode}}
+ return Objects.hash({{#vars}}{{#vendorExtensions.x-is-jackson-optional-nullable}}hashCodeNullable({{name}}){{/vendorExtensions.x-is-jackson-optional-nullable}}{{^vendorExtensions.x-is-jackson-optional-nullable}}{{^isByteArray}}{{name}}{{/isByteArray}}{{#isByteArray}}Arrays.hashCode({{name}}){{/isByteArray}}{{/vendorExtensions.x-is-jackson-optional-nullable}}{{^-last}}, {{/-last}}{{/vars}}{{#parent}}{{#hasVars}}, {{/hasVars}}super.hashCode(){{/parent}});
+ {{/useReflectionEqualsHashCode}}
+ }{{#vendorExtensions.x-jackson-optional-nullable-helpers}}
+
+ private static <T> int hashCodeNullable(JsonNullable<T> a) {
+ if (a == null) {
+ return 1;
+ }
+ return a.isPresent() ? Arrays.deepHashCode(new Object[]{a.get()}) : 31;
+ }{{/vendorExtensions.x-jackson-optional-nullable-helpers}}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("class {{classname}} {\n");
+ {{#parent}}
+ sb.append(" ").append(toIndentedString(super.toString())).append("\n");
+ {{/parent}}
+ {{#vars}}
+ sb.append(" {{name}}: ").append(toIndentedString({{name}})).append("\n");
+ {{/vars}}
+ sb.append("}");
+ return sb.toString();
+ }
+
+ /**
+ * Convert the given object to string with each line indented by 4 spaces
+ * (except the first line).
+ */
+ private{{#jsonb}} static{{/jsonb}} String toIndentedString(Object o) {
+ if (o == null) {
+ return "null";
+ }
+ return o.toString().replace("\n", "\n ");
+ }
+{{#supportUrlQuery}}
+
+ /**
+ * Convert the instance into URL query string.
+ *
+ * @return URL query string
+ */
+ public String toUrlQueryString() {
+ return toUrlQueryString(null);
+ }
+
+ /**
+ * Convert the instance into URL query string.
+ *
+ * @param prefix prefix of the query string
+ * @return URL query string
+ */
+ public String toUrlQueryString(String prefix) {
+ String suffix = "";
+ String containerSuffix = "";
+ String containerPrefix = "";
+ if (prefix == null) {
+ // style=form, explode=true, e.g. /pet?name=cat&type=manx
+ prefix = "";
+ } else {
+ // deepObject style e.g. /pet?id[name]=cat&id[type]=manx
+ prefix = prefix + "[";
+ suffix = "]";
+ containerSuffix = "]";
+ containerPrefix = "[";
+ }
+
+ StringJoiner joiner = new StringJoiner("&");
+
+ {{#allVars}}
+ // add `{{baseName}}` to the URL query string
+ {{#isArray}}
+ {{#items.isPrimitiveType}}
+ {{#uniqueItems}}
+ if ({{getter}}() != null) {
+ int i = 0;
+ for ({{{items.dataType}}} _item : {{getter}}()) {
+ try {
+ joiner.add(String.format("%s{{baseName}}%s%s=%s", prefix, suffix,
+ "".equals(suffix) ? "" : String.format("%s%d%s", containerPrefix, i, containerSuffix),
+ URLEncoder.encode(String.valueOf(_item), "UTF-8").replaceAll("\\+", "%20")));
+ } catch (UnsupportedEncodingException e) {
+ // Should never happen, UTF-8 is always supported
+ throw new RuntimeException(e);
+ }
+ }
+ i++;
+ }
+ {{/uniqueItems}}
+ {{^uniqueItems}}
+ if ({{getter}}() != null) {
+ for (int i = 0; i < {{getter}}().size(); i++) {
+ try {
+ joiner.add(String.format("%s{{baseName}}%s%s=%s", prefix, suffix,
+ "".equals(suffix) ? "" : String.format("%s%d%s", containerPrefix, i, containerSuffix),
+ URLEncoder.encode(String.valueOf({{getter}}().get(i)), "UTF-8").replaceAll("\\+", "%20")));
+ } catch (UnsupportedEncodingException e) {
+ // Should never happen, UTF-8 is always supported
+ throw new RuntimeException(e);
+ }
+ }
+ }
+ {{/uniqueItems}}
+ {{/items.isPrimitiveType}}
+ {{^items.isPrimitiveType}}
+ {{#items.isModel}}
+ {{#uniqueItems}}
+ if ({{getter}}() != null) {
+ int i = 0;
+ for ({{{items.dataType}}} _item : {{getter}}()) {
+ if (_item != null) {
+ joiner.add(_item.toUrlQueryString(String.format("%s{{baseName}}%s%s", prefix, suffix,
+ "".equals(suffix) ? "" : String.format("%s%d%s", containerPrefix, i, containerSuffix))));
+ }
+ }
+ i++;
+ }
+ {{/uniqueItems}}
+ {{^uniqueItems}}
+ if ({{getter}}() != null) {
+ for (int i = 0; i < {{getter}}().size(); i++) {
+ if ({{getter}}().get(i) != null) {
+ joiner.add({{getter}}().get(i).toUrlQueryString(String.format("%s{{baseName}}%s%s", prefix, suffix,
+ "".equals(suffix) ? "" : String.format("%s%d%s", containerPrefix, i, containerSuffix))));
+ }
+ }
+ }
+ {{/uniqueItems}}
+ {{/items.isModel}}
+ {{^items.isModel}}
+ {{#uniqueItems}}
+ if ({{getter}}() != null) {
+ int i = 0;
+ for ({{{items.dataType}}} _item : {{getter}}()) {
+ if (_item != null) {
+ try {
+ joiner.add(String.format("%s{{baseName}}%s%s=%s", prefix, suffix,
+ "".equals(suffix) ? "" : String.format("%s%d%s", containerPrefix, i, containerSuffix),
+ URLEncoder.encode(String.valueOf(_item), "UTF-8").replaceAll("\\+", "%20")));
+ } catch (UnsupportedEncodingException e) {
+ // Should never happen, UTF-8 is always supported
+ throw new RuntimeException(e);
+ }
+ }
+ i++;
+ }
+ }
+ {{/uniqueItems}}
+ {{^uniqueItems}}
+ if ({{getter}}() != null) {
+ for (int i = 0; i < {{getter}}().size(); i++) {
+ if ({{getter}}().get(i) != null) {
+ try {
+ joiner.add(String.format("%s{{baseName}}%s%s=%s", prefix, suffix,
Review Comment:
Remember ROOT locale for all these formats
##########
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonContentWriter.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.solrj;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.io.IOException;
+import java.io.OutputStream;
+import org.apache.solr.client.solrj.request.RequestWriter;
+
+public class JacksonContentWriter implements RequestWriter.ContentWriter {
+
+ private final Object toWrite;
+ private final String contentType;
+
+ public JacksonContentWriter(String contentType, Object toWrite) {
+ this.contentType = contentType;
+ this.toWrite = toWrite;
+ }
+
+ @Override
+ public void write(OutputStream os) throws IOException {
+ new ObjectMapper().writeValue(os, toWrite);
Review Comment:
share the ObjectMapper?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1670095052
So cool to have this in!! Thanks @gerlowskija
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija merged pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija merged PR #1793:
URL: https://github.com/apache/solr/pull/1793
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1651953719
> Would this come to Solr 9 albeit in an opt-in module users would try (not required by SolrJ), then embrace it in Solr 10?
So the part used by users, being the actual clients that is generated code, will be packaged in SolrJ core (not a separate solrj module), so it can't really be opt-out. SolrJ core also doesn't need to depend on this `solrj-model` or `solrj-api` module, so that doesn't need to be opt-in or opt-out, only `solr-core` (and possibly Solr Modules) would depend on this new SolrJ module.
> Might "Api" be a better name for this module? "Model" seems abstract.
I think I agree here.
> Putting "I" before the interfaces is something I normally kinda hate but if it's only a practice to be used here then I can accept it but not like it. Maybe "Api" prefix or suffix or none at all?
I also really don't like the "I" thing. though, I'm not really sure what would be better. Maybe if we have an automated gradle check that ensures that the class starts with "I". I just want to make sure everything stays consistent.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1652121987
> Might "Api" be a better name for this module? "Model" seems abstract.
+1 - I waffled on the name a few times, but in hindsight I do think 'api' is better.
> Putting "I" before the interfaces is something I normally kinda hate [...] Maybe "Api" prefix or suffix or none at all?
I hate it as well, but I'm not sure how to differentiate between the API interfaces and the classes that implement them. The implementing classes have an -API suffix, so that's already taken. We could move the -API suffix to the interfaces and give the actual implementations an "-Impl" suffix, but I'm not really in love with that either. Anyone have an idea that they like better than -API/-Impl or the "I-" prefix? (Or have a preference between the two?)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1652258491
I'd rather get rid of `Impl` all together. (As I'd like to get rid of it in the other places we use it as well, but that's a whole separate issue). So `CollectionBackupApi` and `CollectionBackup`, (it will be clear it's an `api`, since it lives in the `org.apache.solr.handler.admin.api` package).
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1793: SOLR-16825: Generate v2 API SolrRequest bindings
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1793:
URL: https://github.com/apache/solr/pull/1793#issuecomment-1652534121
Then could we embed "api"/model if it's inextricably a part of SolrJ? (somehow put the same classes in the same JAR)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org