search

takezoe / solr-scala-client

Solr Client for Scala
Apache License 2.0
91 stars 43 forks source link

add and addToCollection throw NPE #53

Closed jeffdyke closed 5 years ago

jeffdyke commented 5 years ago

I have noticed in a newer version of the ApacheSolrClient it seems to want the collection being written to as an argument and also one for commiting, otherwise an NPE, or 404 is thrown depending on the way the client is instantiated. I have a simple schema that conforms to this case class:

case class Test(caption: String, categoryGroup: String, categoryName: String, id: String)

The following are my results with unchanged solr-scala-client (running sbt console):

import com.github.takezoe.solr.scala.{BatchRegister, SolrClient}
case class Test(caption: String, categoryGroup: String, categoryName: String, id: String)
val solrTest = Test("here is a caption", "here is a group", "here is name", "media-6000")
val test = new SolrClient("http://localhost:8983/solr")
test.add(solrTest)
---
java.lang.NullPointerException
  at com.github.takezoe.solr.scala.BatchRegister.$anonfun$add$1(BatchRegister.scala:17)
---

Using addToCollection, same imports/data as above

test.addToCollection("testing", solrTest)
res9: com.github.takezoe.solr.scala.BatchRegister = com.github.takezoe.solr.scala.BatchRegister@4f86b106
res9.commit
---
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 404 Not Found</title>
</head>
<body><h2>HTTP ERROR 404</h2>
<p>Problem accessing /solr/update. Reason:
<pre>    Not Found</pre></p>
</body>
</html>
---

With modified code seen here: https://github.com/takezoe/solr-scala-client/compare/master...jeffdyke:master

add and addToCollection will return properly, when supplied a collection, but you can't send them the collection as part of the URL, that will result in a 404 as it tries to append it.

I'm happy to open an official PR, but would like to get your thoughts on this. If i were to change it, i would make it a breaking change, or you could infer the collection from the URL, but i don't like that idea to much.

takezoe commented 5 years ago

Thanks for your detailed report!

I prefer a natural way rather than a workaround for maintaining the backward compatibility. So your change seems to be better than inferring the collection name from URL.

By the way, unit tests of this library are very limited for now. We might have to add functional tests using the embedded solr or docker to find out this kind of issues earlier.

jeffdyke commented 5 years ago

Thanks for the quick response @takezoe and i agree with you, i don't want to infer a collection from a URI, i'll keep working on my fork and test different approaches and then submit back for PR and see if i have some time to work up more unit tests.

jeffdyke commented 5 years ago

Oh and btw, this is not a breaking change.