Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley merged PR #3282: URL: https://github.com/apache/solr/pull/3282 -- 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]
Re: [PR] SOLR-17705 for V2 API [solr]
gerlowskija commented on PR #3282: URL: https://github.com/apache/solr/pull/3282#issuecomment-2812533955 Sorry, was out on some PTO. This all LGTM; merge away! -- 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]
Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley commented on PR #3282: URL: https://github.com/apache/solr/pull/3282#issuecomment-2803635565 Well I'll merge this in another day if I don't hear back. -- 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]
Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley commented on PR #3282: URL: https://github.com/apache/solr/pull/3282#issuecomment-2789929157 Good. After merging the parent issue tonight, I'll brush up this PR, adding lucene.experimental but mostly unchanged, update CHANGES.txt. Should be in mergeable shape tomorrow. -- 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]
Re: [PR] SOLR-17705 for V2 API [solr]
gerlowskija commented on code in PR #3282:
URL: https://github.com/apache/solr/pull/3282#discussion_r2035167694
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind.
*/
+public class JacksonDataBindResponseParser extends ResponseParser {
+
+ private final Class typeParam;
+
+ public JacksonDataBindResponseParser(Class typeParam) {
+this.typeParam = typeParam;
+ }
+
+ @Override
+ public String getWriterType() {
+return "json";
+ }
+
+ @Override
+ public Collection getContentTypes() {
+return List.of("application/json");
+ }
+
+ // TODO it'd be nice if the ResponseParser could receive the mime type so it
can parse
+ // accordingly, maybe json, cbor, smile
+
+ @Override
+ public NamedList processResponse(InputStream stream, String
encoding) throws IOException {
+// TODO generalize to CBOR, Smile, ...
+var mapper = JacksonContentWriter.DEFAULT_MAPPER;
+
+final T parsedVal;
+if (encoding == null) {
+ parsedVal = mapper.readValue(stream, typeParam);
+} else {
+ parsedVal = mapper.readValue(new InputStreamReader(stream, encoding),
typeParam);
+}
+
+return SimpleOrderedMap.of("response", parsedVal);
Review Comment:
> I would personally rather prefer that become a new evolutionary issue/PR
rather than increasing the scope of this one
Sounds good - we've already got this tracked in SOLR-17549, so we're good on
that front.
--
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]
Re: [PR] SOLR-17705 for V2 API [solr]
gerlowskija commented on code in PR #3282:
URL: https://github.com/apache/solr/pull/3282#discussion_r2035173216
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind.
*/
+public class JacksonDataBindResponseParser extends ResponseParser {
+
+ private final Class typeParam;
+
+ public JacksonDataBindResponseParser(Class typeParam) {
+this.typeParam = typeParam;
+ }
+
+ @Override
+ public String getWriterType() {
+return "json";
+ }
+
+ @Override
+ public Collection getContentTypes() {
+return List.of("application/json");
+ }
+
+ // TODO it'd be nice if the ResponseParser could receive the mime type so it
can parse
+ // accordingly, maybe json, cbor, smile
+
+ @Override
+ public NamedList processResponse(InputStream stream, String
encoding) throws IOException {
Review Comment:
I like that overall plan - it makes sense conceptually to have
`ResponseParser` do error detection, since that's part of the response. (And
it'd be a really nice simplification for our SolrClient implementations
themselves.)
And as mentioned elsewhere - it's fine with me to not put that in this PR.
It's definitely a bigger change.
--
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]
Re: [PR] SOLR-17705 for V2 API [solr]
gerlowskija commented on code in PR #3282:
URL: https://github.com/apache/solr/pull/3282#discussion_r2035164196
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind.
*/
+public class JacksonDataBindResponseParser extends ResponseParser {
Review Comment:
> Jackson is the parsing, DataBind is the, well, data binding -- mapping to
POJOs via annotations etc.
Oh, OK, fair enough.
I've only ever used Jackson with those annotations - I know in theory that
the dependencies come separately, but it's hard for me to imagine how you'd
even use Jackson if you're not converting to+from some POJO. Is it just all
`Map` and `List` instances without databind? (That's rhetorical I guess - I
can google it.)
--
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]
Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley commented on PR #3282: URL: https://github.com/apache/solr/pull/3282#issuecomment-2776630387 > What's the relationship between this PR and the one that underlies it? Are you looking for consensus on both before you merge the underlying one? Yes; these are important changes that require peer review. I don't want to be a cowboy here, doing whatever to important APIs. -- 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]
Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley commented on code in PR #3282:
URL: https://github.com/apache/solr/pull/3282#discussion_r2027550274
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind.
*/
+public class JacksonDataBindResponseParser extends ResponseParser {
+
+ private final Class typeParam;
+
+ public JacksonDataBindResponseParser(Class typeParam) {
+this.typeParam = typeParam;
+ }
+
+ @Override
+ public String getWriterType() {
+return "json";
+ }
+
+ @Override
+ public Collection getContentTypes() {
+return List.of("application/json");
+ }
+
+ // TODO it'd be nice if the ResponseParser could receive the mime type so it
can parse
+ // accordingly, maybe json, cbor, smile
+
+ @Override
+ public NamedList processResponse(InputStream stream, String
encoding) throws IOException {
Review Comment:
I would honestly hate to bring back a root type merely for error handling.
Java has exceptions for this; let's use that instead. We could make
processResponse return whatever and for successful responses and throw
something for the error case. That's a bigger change than the scope of this
PR, so WDYT about merging this without additional error handling?
I propose a new JIRA issue (or your existing SOLR-17549 maybe) enhance the
`ResponseParser` abstraction to take on the responsibility of error detection
and throwing, and thus reducing some of the scope in the HTTP SolrClient side.
I could imagine the processResponse method having more parameters including the
HTTP status code, and using that to determine are we going to use data-bind or
are we going to look for the error. We'd need to do this for XML & JavaBin;
finding a way to reuse some logic where it's similar. Don't need to spec this
out this second.
--
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]
Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley commented on code in PR #3282:
URL: https://github.com/apache/solr/pull/3282#discussion_r2027539523
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind.
*/
+public class JacksonDataBindResponseParser extends ResponseParser {
+
+ private final Class typeParam;
+
+ public JacksonDataBindResponseParser(Class typeParam) {
+this.typeParam = typeParam;
+ }
+
+ @Override
+ public String getWriterType() {
+return "json";
+ }
+
+ @Override
+ public Collection getContentTypes() {
+return List.of("application/json");
+ }
+
+ // TODO it'd be nice if the ResponseParser could receive the mime type so it
can parse
+ // accordingly, maybe json, cbor, smile
+
+ @Override
+ public NamedList processResponse(InputStream stream, String
encoding) throws IOException {
+// TODO generalize to CBOR, Smile, ...
+var mapper = JacksonContentWriter.DEFAULT_MAPPER;
+
+final T parsedVal;
+if (encoding == null) {
+ parsedVal = mapper.readValue(stream, typeParam);
+} else {
+ parsedVal = mapper.readValue(new InputStreamReader(stream, encoding),
typeParam);
+}
+
+return SimpleOrderedMap.of("response", parsedVal);
Review Comment:
Ah; good catch! Hmm. It'd be good to induce an error in a test using this
API so we can verify that whatever we want to happen, actually happens. I
would personally rather prefer that become a new evolutionary issue/PR rather
than increasing the scope of this one. I could see us making bigger changes to
accommodate that concern. I'll expand on that in the other comment...
--
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]
Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley commented on code in PR #3282:
URL: https://github.com/apache/solr/pull/3282#discussion_r2027510686
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind.
*/
+public class JacksonDataBindResponseParser extends ResponseParser {
+
+ private final Class typeParam;
+
+ public JacksonDataBindResponseParser(Class typeParam) {
+this.typeParam = typeParam;
+ }
+
+ @Override
+ public String getWriterType() {
+return "json";
Review Comment:
That's a separate issue for sure, and was on my mind a bit as I worked on
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: [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]
Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley commented on code in PR #3282:
URL: https://github.com/apache/solr/pull/3282#discussion_r2027509100
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind.
*/
+public class JacksonDataBindResponseParser extends ResponseParser {
Review Comment:
Jackson does not imply Jackson-DataBind -- the latter depends on the former,
of course. Jackson is the parsing, DataBind is the, well, data binding --
mapping to POJOs via annotations etc.
Happy to add lucene.experimental wherever.
--
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]
Re: [PR] SOLR-17705 for V2 API [solr]
gerlowskija commented on code in PR #3282:
URL: https://github.com/apache/solr/pull/3282#discussion_r2026858878
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind.
*/
+public class JacksonDataBindResponseParser extends ResponseParser {
Review Comment:
[0] Maybe remove "DataBind" from the name, as it's implied/redundant once
Jackson is mentioned?
[-0] Wdyt about marking this class "lucene.experimental" so we can rename or
make changes more freely later if needed. I guess we're still held to the
ResponseParser interface, so things couldn't change too drastically, but it
still might be prudent since this is something that we're still trying to get
right in SolrJ.
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collection;
+import java.util.List;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind.
*/
+public class JacksonDataBindResponseParser extends ResponseParser {
+
+ private final Class typeParam;
+
+ public JacksonDataBindResponseParser(Class typeParam) {
+this.typeParam = typeParam;
+ }
+
+ @Override
+ public String getWriterType() {
+return "json";
Review Comment:
[0] I think this would need to be changed at the ResponseParser level, but
it's a bummer that this method can't return a list, or indicate handling for
multiple write-types. There's no reason our Jackson support should be specific
to JSON, as you indicate in some comments below.
Much larger issue than this PR though...
##
solr/solrj/src/test/org/apache/solr/client/solrj/ApiMustacheTemplateTests.java:
##
Review Comment:
[0] It's obsolete in its current form, but in theory it still makes sense to
have *some* sort of test validating that the v2 deserialization plumbing
actually works as intended?
We use the generated v2 code in a few places (e.g. CLIUtils,
TestDistribFileStore), so I guess there's some implicit testing of this
already? Maybe that's enough? idk - just thinking aloud.
##
solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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/l
Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley commented on PR #3282: URL: https://github.com/apache/solr/pull/3282#issuecomment-2760232448 WDYT @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: [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]
Re: [PR] SOLR-17705 for V2 API [solr]
dsmiley commented on code in PR #3282:
URL: https://github.com/apache/solr/pull/3282#discussion_r2008841563
##
solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java:
##
@@ -96,7 +97,7 @@ public static void postFile(SolrClient client, ByteBuffer
buffer, String name, S
uploadReq.setSig(List.of(sig));
}
- final var uploadRsp = uploadReq.process(client).getParsed();
+ final UploadToFileStoreResponse uploadRsp = uploadReq.process(client);
Review Comment:
IMO we've been using `var` too much.
##
solr/core/src/java/org/apache/solr/cli/CLIUtils.java:
##
@@ -322,7 +322,7 @@ public static boolean safeCheckCoreExists(String solrUrl,
String coreName, Strin
Thread.sleep(clamPeriodForStatusPollMs);
}
final var coreStatusReq = new CoresApi.GetCoreStatus(coreName);
-final var coreStatusRsp =
coreStatusReq.process(solrClient).getParsed();
Review Comment:
I admit the result of this work as seen in consuming/calling code right here
is _merely_ removing `getParsed()` since there is now no intermediate class
holding the parsed value. But anyone browsing the generated code / class
hierarchy will now see something simpler.
##
solr/solrj/src/resources/java-template/api.mustache:
##
@@ -91,17 +90,13 @@ public class {{classname}} {
{{#operation}}
{{^vendorExtensions.x-omitFromCodegen}}
{{#vendorExtensions.x-rawOutput}}
-public static class {{operationIdCamelCase}}Response extends
InputStreamResponse {}
+public static class {{operationIdCamelCase}} extends SolrRequest
+ {
{{/vendorExtensions.x-rawOutput}}
{{^vendorExtensions.x-rawOutput}}
-public static class {{operationIdCamelCase}}Response extends
JacksonParsingResponse<{{modelPackage}}.{{returnType}}> {
Review Comment:
gone!
##
solr/solrj/src/test/org/apache/solr/client/solrj/ApiMustacheTemplateTests.java:
##
Review Comment:
seems obsolete now
##
solr/solrj/src/resources/java-template/api.mustache:
##
@@ -91,17 +90,13 @@ public class {{classname}} {
{{#operation}}
{{^vendorExtensions.x-omitFromCodegen}}
{{#vendorExtensions.x-rawOutput}}
-public static class {{operationIdCamelCase}}Response extends
InputStreamResponse {}
Review Comment:
gone!
--
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]
