Re: [PR] SOLR-17617: Add smoke test for V2 API and fix up problems [solr]
colvinco commented on code in PR #3023:
URL: https://github.com/apache/solr/pull/3023#discussion_r2008794161
##
solr/core/src/java/org/apache/solr/api/V2HttpCall.java:
##
@@ -174,7 +174,7 @@ public void call(SolrQueryRequest req, SolrQueryResponse
rsp) {
}
}
}
- } else if ("cores".equals(prefix)) {
+ } else if ("cores".equals(prefix) && pathSegments.size() > 1) {
origCorename = pathSegments.get(1);
Review Comment:
This is getting an index OOB if you request `/cores`, could combine the size
check with the existing size check on the earlier if statement...
--
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-17617: Add smoke test for V2 API and fix up problems [solr]
colvinco commented on code in PR #3023:
URL: https://github.com/apache/solr/pull/3023#discussion_r2029046624
##
solr/core/src/test/org/apache/solr/handler/admin/api/V2APISmokeTest.java:
##
@@ -0,0 +1,457 @@
+/*
+ * 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.handler.admin.api;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.http.HttpResponse;
+import org.apache.http.client.methods.HttpDelete;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.client.methods.HttpPut;
+import org.apache.http.entity.ContentType;
+import org.apache.http.entity.FileEntity;
+import org.apache.http.entity.StringEntity;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.*;
+import java.net.URI;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.time.Instant;
+import java.util.*;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+
+public class V2APISmokeTest extends SolrCloudTestCase {
+
+// TODO: How is this normally done in these tests?
+private final ObjectMapper objectMapper = new ObjectMapper()
+.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES,
false);
+private URL baseUrl;
+private String baseUrlV2;
+
+@BeforeClass
+public static void setupCluster() throws Exception {
+System.setProperty("enable.packages", "true"); // for file upload
+System.setProperty("solr.allowPaths", "*"); // for backups
+configureCluster(2).addConfig("conf",
configset("cloud-minimal")).configure();
+}
+
+@Before
+@Override
+public void setUp() throws Exception {
+super.setUp();
+baseUrl = cluster.getJettySolrRunner(0).getBaseUrl();
+baseUrlV2 = cluster.getJettySolrRunner(0).getBaseURLV2().toString();
+}
+
+@AfterClass
+public static void teardownClass() {
+System.clearProperty("enable.packages");
+System.clearProperty("solr.allowPaths");
+}
+
+@Test
+public void testCollectionsApi() throws Exception {
+canGet("/collections");
+canPost("/collections", """
Review Comment:
The one downside is that this can't be backported to the v9 branch as it
supports Java 11, but I think that's okay given where we are at with 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-17617: Add smoke test for V2 API and fix up problems [solr]
colvinco commented on PR #3023: URL: https://github.com/apache/solr/pull/3023#issuecomment-2779015426 Thanks for the feedback @gerlowskija. Okay yep lets get something in and iterate on the rest of it. I will do a bit of cleanup, raise some issues under [SOLR-15734](https://issues.apache.org/jira/browse/SOLR-15734) for the bugs I've found so far and then I'm happy for you to polish the set of tests that you think are best to start with, thank you. > One tradeoff I guess is whether we want the test to be explicit about the HTTP details (the path, method, request body format, etc.), or whether it should use the SolrJ classes that we're auto-generating for the v2 APIs. > > The former approach asserts that the API looks exactly the way we want it to. The latter approach loses that explicitness by abstracting it behind client objects, but it tests the client and server code both which is nice. This is always a bit of a dilemma isn't it; how far do you trust that the implementation and spec align and is it worth the effort of doing things exhaustively? The "raw" approach is good for asserting the API shape, as you say. But obviously testing the client code is also a good thing. Perhaps the "best" solution is a hybrid approach where the requests are made using the client, but with some interception to assert that the request details match the expected "raw" details... but perhaps that's overkill here and better suited to individual endpoint tests (or unit tests for the client) rather than the smoke test? -- 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-17617: Add smoke test for V2 API and fix up problems [solr]
gerlowskija commented on PR #3023:
URL: https://github.com/apache/solr/pull/3023#issuecomment-2776308451
Hey @colvinco - my apologies - I missed some of the updates you put on this
PR. Trying to catch up now!
Will answer some specific questions you mentioned above, and then I'll wade
in for a broader review...
> Does it make sense to test the /core APIs with a cloud deployment, or do I
need to run a standalone?
You wouldn't want to hit those APIs in SolrCloud generally - the `/core`
APIs are built to work in standalone mode so they won't update the ZooKeeper
state to reflect created cores, etc. But for testing purposes if it's more
convenient to exercise those APIs in a SolrCloud cluster, I think that's
reasonable.
> I guess the /shards APIs mostly make sense when using implicit routing
rather than composite?
Yep - exactly!
On the overall approach you shared above:
```
public void testCollectionsApi() throws Exception {
canGet("/collections");
canPost("/collections", """
{
"name": "testCollection",
"numShards": 1
}
""");
```
This is nice and concise; I like it!
One tradeoff I guess is whether we want the test to be explicit about the
HTTP details (the path, method, request body format, etc.), or whether it
should use the SolrJ classes that we're auto-generating for the v2 APIs.
The former approach asserts that the API looks exactly the way we want it
to. The latter approach loses that explicitness by abstracting it behind
client objects, but it tests the client and server code both which is nice.
--
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-17617: Add smoke test for V2 API and fix up problems [solr]
gerlowskija commented on code in PR #3023:
URL: https://github.com/apache/solr/pull/3023#discussion_r2027395629
##
solr/core/src/test/org/apache/solr/handler/admin/api/V2APISmokeTest.java:
##
@@ -0,0 +1,457 @@
+/*
+ * 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.handler.admin.api;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.http.HttpResponse;
+import org.apache.http.client.methods.HttpDelete;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.client.methods.HttpPut;
+import org.apache.http.entity.ContentType;
+import org.apache.http.entity.FileEntity;
+import org.apache.http.entity.StringEntity;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.*;
+import java.net.URI;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.time.Instant;
+import java.util.*;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+
+public class V2APISmokeTest extends SolrCloudTestCase {
+
+// TODO: How is this normally done in these tests?
Review Comment:
We're still pretty "early days" in our adoption of Jackson for deserializing
response types, so I wouldn't say we have an established pattern around this.
We generate SolrRequest/SolrResponse client objects, which use Jackson to
deserialize responses internally. But if you're looking to invoke the APIs
directly, then using an ObjectMapper as you're doing here is probably the best
bet.
One tweak though - you could reuse an existing ObjectMapper rather than
creating your own. `JacksonContentWriter.DEFAULT_MAPPER` should be good -
that's the one that we use in SolrJ.
##
solr/core/src/test/org/apache/solr/handler/admin/api/V2APISmokeTest.java:
##
@@ -0,0 +1,457 @@
+/*
+ * 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.handler.admin.api;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.http.HttpResponse;
+import org.apache.http.client.methods.HttpDelete;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.client.methods.HttpPut;
+import org.apache.http.entity.ContentType;
+import org.apache.http.entity.FileEntity;
+import org.apache.http.entity.StringEntity;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.*;
+import java.net.URI;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.time.Instant;
+import java.util.*;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+
+public class V2APISmokeTest extends SolrCloudTestCase {
+
+// TODO: How is this normally done in these tests?
+private final ObjectMapper objectMapper = new ObjectMapper()
+.
Re: [PR] SOLR-17617: Add smoke test for V2 API and fix up problems [solr]
colvinco commented on PR #3023: URL: https://github.com/apache/solr/pull/3023#issuecomment-2745318524 This is still a fair way from finished, but I have found quite a few problems while doing it - some of which I've put fixes in for, others are just noted in TODOs in the test and still need attention. I could probably do with help on the right way to set things up to test the APIs that I've not finished the tests for. - Does it make sense to test the /core APIs with a cloud deployment, or do I need to run a standalone? - I guess the /shards APIs mostly make sense when using implicit routing rather than composite? -- 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]
