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<T> 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<T> extends ResponseParser { + + private final Class<T> typeParam; + + public JacksonDataBindResponseParser(Class<T> 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/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<T> extends ResponseParser { + + private final Class<T> typeParam; + + public JacksonDataBindResponseParser(Class<T> typeParam) { + this.typeParam = typeParam; + } + + @Override + public String getWriterType() { + return "json"; + } + + @Override + public Collection<String> 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<Object> 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: [0] `processResponse`'s callers (`HttpSolrClientBase`, `HttpSolrClient`) do some error-checking on this method's retval, looking for an "error" key in the NamedList as an indicator of error. This implementation will never return a NL with that key, so it loses the benefits of that error-checking. IMO we needn't handle that in this PR. It pre-exists your change in InputStreamResponseParser, NoOpResponseParser, etc. And there's [a JIRA ticket](https://issues.apache.org/jira/browse/SOLR-17549) for thinking through how we want error-handling to work in v2, so it won't get lost or fall through the cracks. But I'm curious if you had any thoughts. I had a plan in the back of my mind of uniting JacksonParsingResponse and InputStreamResponse, which would allow us to hook into some of the validation the ISR currently has. But it was never a great plan, and it doesn't make any sense if JPR goes away entirely. It'd be hacky, but we could have callers look for `instanceof SolrJerseyResponse` and inspect that POJO for errors? Or maybe the client only looks at the HTTP status code for v2 APIs? ########## solr/test-framework/src/java/org/apache/solr/cloud/ConfigRequest.java: ########## @@ -25,13 +24,13 @@ import org.apache.solr.common.params.CommonParams; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.params.SolrParams; +import org.apache.solr.common.util.NamedList; /** * A class for making a request to the config handler. Tests can use this e.g. to add custom * components, handlers, parsers, etc. to an otherwise generic configset. */ -@SuppressWarnings({"rawtypes"}) -public class ConfigRequest extends CollectionRequiringSolrRequest { +public class ConfigRequest extends CollectionRequiringSolrRequest<SolrResponse> { Review Comment: [0] Unrelated to your PR, but I wonder why this lives in `test-framework`... ########## 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<T> extends ResponseParser { + + private final Class<T> typeParam; + + public JacksonDataBindResponseParser(Class<T> typeParam) { + this.typeParam = typeParam; + } + + @Override + public String getWriterType() { + return "json"; + } + + @Override + public Collection<String> 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<Object> processResponse(InputStream stream, String encoding) throws IOException { Review Comment: [0] I love that `process` can now return arbitrary types; it's a shame that can't extend to the retval of `processResponse`, and that we have to repackage our arbitrary type as a NL for all of the various SolrClient-internal NL-inspection code. I think I brought this up in the related PR, but it strikes me again whether we shouldn't be slightly stricter on the allowed return types. Allow `process` to return types other than NL, but require retvals to implement some interface that exposes the information that most of our SolrClient impls look at internally: `wasSuccessful()`, `getErrorMessage()`, etc. That'd give our clients a uniform way to inspect responses and throw SolrServerExceptions, etc, regardless of whether we parse the response into a NamedList, or a SolrJerseyResponse, or an InputStreamResponse, or... (This would break the error-handling logjam I've been wrestling with for awhile now too - see comment on L62 for more details.) ########## 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: Haha, you turned me on to it a year or two back and I've loved it since. I get that the type is less visible, but it makes the code scan better in places where the type name is a bit of a mouthful and "tidy" would otherwise split it up into multiple lines But, to each their own. -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
