Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/430#discussion_r207741118
--- Diff:
solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java ---
@@ -114,17 +121,34 @@ private static void writeVal(Writer writer, String
name, Object v, String update
if (update == null) {
if (v != null) {
- XML.writeXML(writer, "field", v.toString(), "name", name );
+ if(v instanceof SolrInputDocument) {
+ OutputStream os = SolrInputDocumentXmlOutput((SolrInputDocument)
v);
--- End diff --
1st; it's not generally valid to call OutputString.toString() (or
Writer.toString() for that matter) unless the declared implementation expressly
defines what it does. I am aware the impl/subclass you've chosen does but the
declared type here, OutputStream, does not. It would be better if that
"solrInputDocumentXmlOutput" method returns a String since that's all we're
going to do here any way.
But moreover, I'm concerned that the approach here buffers child documents
instead of writing directly to a stream. The buffering happens for each depth
again, and gets larger. It seems this deficiency is mostly due to the API
contract of XML.writeXML which is not recursive -- it wants a string value and
thus forces you to materialize that up front. We could choose to just accept
this as _perhaps_ not a big deal. Or, what if XML had an inner interface:
(lambda friendly)
```
interface Writable {
void write(Writer w) throws IOException ;
}
```
And the writeXML method was overloaded with a variant that took that
instead of the Value. To avoid duplicating similar methods, the existing ones
could be convenience methods to this one with a simple lambda that wrote the
string, possibly escaping it first if needed. WDYT?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
