Merge branch 'main' into main

Signed-off-by: Srikanth Padakanti <srikanth29.9@gmail.com>
opensearch-project · Apr 19, 2024 · 6de3241 · 6de3241
2 parents c208a44 + b4692c8
commit 6de3241
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 155 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -45,6 +45,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Improve built-in secure transports support ([#12907](https://github.com/opensearch-project/OpenSearch/pull/12907))
 - Update links to documentation in rest-api-spec ([#13043](https://github.com/opensearch-project/OpenSearch/pull/13043))
 - Refactoring globMatch using simpleMatchWithNormalizedStrings from Regex ([#13104](https://github.com/opensearch-project/OpenSearch/pull/13104))
+- [BWC and API enforcement] Reconsider the breaking changes check policy to detect breaking changes against released versions ([#13292](https://github.com/opensearch-project/OpenSearch/pull/13292))
 
 ### Deprecated
 
@@ -60,6 +61,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix implement mark() and markSupported() in class FilterStreamInput ([#13098](https://github.com/opensearch-project/OpenSearch/pull/13098))
 - Fix snapshot _status API to return correct status for partial snapshots ([#12812](https://github.com/opensearch-project/OpenSearch/pull/12812))
 - Improve the error messages for _stats with closed indices ([#13012](https://github.com/opensearch-project/OpenSearch/pull/13012))
+- Ignore BaseRestHandler unconsumed content check as it's always consumed. ([#13290](https://github.com/opensearch-project/OpenSearch/pull/13290))
 
 ### Security
 

diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md
@@ -57,6 +57,7 @@
       - [Developer API](#developer-api)
       - [User API](#user-api)
       - [Experimental Development](#experimental-development)
+      - [API Compatibility Checks](#api-compatibility-checks)
     - [Backports](#backports)
     - [LineLint](#linelint)
     - [Lucene Snapshots](#lucene-snapshots)
@@ -607,6 +608,20 @@ a LTS feature but with additional guard rails and communication mechanisms to si
 release, or be removed altogether. Any Developer or User APIs implemented along with the experimental feature should be marked with `@ExperimentalApi` (or documented as
 `@opensearch.experimental`) annotation to signal the implementation is not subject to LTS and does not follow backwards compatibility guidelines.
 
+#### API Compatibility Checks
+
+The compatibility checks for public APIs are performed using [japicmp](https://siom79.github.io/japicmp/) and are available as separate Gradle tasks (those are run on demand at the moment):
+
+```
+./gradlew japicmp
+```
+
+By default, the API compatibility checks are run against the latest released version of the OpenSearch, however the target version to compare to could be provided using system property during the build, fe.:
+
+```
+./gradlew japicmp  -Djapicmp.compare.version=2.14.0-SNAPSHOT
+```
+
 ### Backports
 
 The Github workflow in [`backport.yml`](.github/workflows/backport.yml) creates backport PRs automatically when the original PR with an appropriate label `backport <backport-branch-name>` is merged to main with the backport workflow run successfully on the PR. For example, if a PR on main needs to be backported to `1.x` branch, add a label `backport 1.x` to the PR and make sure the backport workflow runs on the PR along with other checks. Once this PR is merged to main, the workflow will create a backport PR to the `1.x` branch.

diff --git a/server/build.gradle b/server/build.gradle
@@ -173,6 +173,22 @@ tasks.named("testingConventions").configure {
     }
 }
 
+// Set to current version by default
+def japicmpCompareTarget = System.getProperty("japicmp.compare.version")
+if (japicmpCompareTarget == null) { /* use latest released version */
+    // Read the list from maven central.
+    // Fetch the metadata and parse the xml into Version instances, pick the latest one
+    japicmpCompareTarget = new URL('https://repo1.maven.org/maven2/org/opensearch/opensearch/maven-metadata.xml').openStream().withStream { s ->
+        new XmlParser().parse(s)
+           .versioning.versions.version
+           .collect { it.text() }.findAll { it ==~ /\d+\.\d+\.\d+/ }
+           .collect { org.opensearch.gradle.Version.fromString(it) }
+           .toSorted()
+           .last()
+           .toString()
+        }
+}
+
 def generateModulesList = tasks.register("generateModulesList") {
     List<String> modules = project(':modules').subprojects.collect { it.name }
     File modulesFile = new File(buildDir, 'generated-resources/modules.txt')
@@ -380,60 +396,59 @@ tasks.named("sourcesJar").configure {
   }
 }
 
-/** Compares the current build against a snapshot build */
+/** Compares the current build against a laltest released version or the version supplied through 'japicmp.compare.version' system property */
 tasks.register("japicmp", me.champeau.gradle.japicmp.JapicmpTask) {
-    oldClasspath.from(files("${buildDir}/snapshot/opensearch-${version}.jar"))
+    logger.info("Comparing public APIs from ${version} to ${japicmpCompareTarget}")
+    oldClasspath.from(files("${buildDir}/japicmp-target/opensearch-${japicmpCompareTarget}.jar"))
     newClasspath.from(tasks.named('jar'))
     onlyModified = true
     failOnModification = true
     ignoreMissingClasses = true
     annotationIncludes = ['@org.opensearch.common.annotation.PublicApi', '@org.opensearch.common.annotation.DeprecatedApi']
     txtOutputFile = layout.buildDirectory.file("reports/java-compatibility/report.txt")
     htmlOutputFile = layout.buildDirectory.file("reports/java-compatibility/report.html")
-    dependsOn downloadSnapshot
+    dependsOn downloadJapicmpCompareTarget
 }
 
 /** If the Java API Comparison task failed, print a hint if the change should be merged from its target branch */
 gradle.taskGraph.afterTask { Task task, TaskState state ->
     if (task.name == 'japicmp' && state.failure != null) {
-        def sha = getGitShaFromJar("${buildDir}/snapshot/opensearch-${version}.jar")
-        logger.info("Incompatiable java api from snapshot jar built off of commit ${sha}")
-
-        if (!inHistory(sha)) {
-          logger.warn('\u001B[33mPlease merge from the target branch and run this task again.\u001B[0m')
-        }
+        logger.info("Public APIs changes incompatiable with ${japicmpCompareTarget} target have been detected")
     }
 }
 
-/** Downloads latest snapshot from maven repository */
-tasks.register("downloadSnapshot", Copy) {
+/** Downloads latest released version from maven repository */
+tasks.register("downloadJapicmpCompareTarget", Copy) {
     def mavenSnapshotRepoUrl = "https://aws.oss.sonatype.org/content/repositories/snapshots/"
     def groupId = "org.opensearch"
     def artifactId = "opensearch"
 
-    def repos = project.getRepositories();
-    MavenArtifactRepository opensearchRepo = repos.maven(repo -> {
-        repo.setName("opensearch-snapshots");
-        repo.setUrl(mavenSnapshotRepoUrl);
-    });
-
-    repos.exclusiveContent(exclusiveRepo -> {
-        exclusiveRepo.filter(descriptor -> descriptor.includeGroup(groupId));
-        exclusiveRepo.forRepositories(opensearchRepo);
-    });
+    // Add repository for snapshot artifacts if japicmp compare target version is snapshot
+    if (japicmpCompareTarget.endsWith("-SNAPSHOT")) {
+        def repos = project.getRepositories();
+        MavenArtifactRepository opensearchRepo = repos.maven(repo -> {
+            repo.setName("opensearch-snapshots");
+            repo.setUrl(mavenSnapshotRepoUrl);
+        });
+
+        repos.exclusiveContent(exclusiveRepo -> {
+            exclusiveRepo.filter(descriptor -> descriptor.includeGroup(groupId));
+            exclusiveRepo.forRepositories(opensearchRepo);
+        });
+    }
 
     configurations {
-        snapshotArtifact {
+        japicmpCompareTargetArtifact {
             exclude group: 'org.apache.lucene'
         }
     }
 
     dependencies {
-        snapshotArtifact("${groupId}:${artifactId}:${version}:")
+        japicmpCompareTargetArtifact("${groupId}:${artifactId}:${japicmpCompareTarget}:")
     }
 
-    from configurations.snapshotArtifact
-    into "$buildDir/snapshot"
+    from configurations.japicmpCompareTargetArtifact
+    into "$buildDir/japicmp-target"
 }
 
 /** Check if the sha is in the current history */

diff --git a/server/src/main/java/org/opensearch/rest/BaseRestHandler.java b/server/src/main/java/org/opensearch/rest/BaseRestHandler.java
@@ -122,10 +122,6 @@ public final void handleRequest(RestRequest request, RestChannel channel, NodeCl
             throw new IllegalArgumentException(unrecognized(request, unconsumedParams, candidateParams, "parameter"));
         }
 
-        if (request.hasContent() && request.isContentConsumed() == false) {
-            throw new IllegalArgumentException("request [" + request.method() + " " + request.path() + "] does not support having a body");
-        }
-
         usageCount.increment();
         // execute the action
         action.accept(channel);

diff --git a/...c/test/java/org/opensearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java b/...c/test/java/org/opensearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java
@@ -32,45 +32,23 @@
 
 package org.opensearch.action.admin.indices.forcemerge;
 
-import org.opensearch.client.node.NodeClient;
-import org.opensearch.common.xcontent.json.JsonXContent;
-import org.opensearch.core.common.bytes.BytesArray;
-import org.opensearch.core.xcontent.MediaTypeRegistry;
 import org.opensearch.core.xcontent.NamedXContentRegistry;
 import org.opensearch.rest.RestRequest;
 import org.opensearch.rest.action.admin.indices.RestForceMergeAction;
-import org.opensearch.test.rest.FakeRestChannel;
 import org.opensearch.test.rest.FakeRestRequest;
 import org.opensearch.test.rest.RestActionTestCase;
 import org.junit.Before;
 
 import java.util.HashMap;
 import java.util.Map;
 
-import static org.hamcrest.Matchers.equalTo;
-import static org.mockito.Mockito.mock;
-
 public class RestForceMergeActionTests extends RestActionTestCase {
 
     @Before
     public void setUpAction() {
         controller().registerHandler(new RestForceMergeAction());
     }
 
-    public void testBodyRejection() throws Exception {
-        final RestForceMergeAction handler = new RestForceMergeAction();
-        String json = JsonXContent.contentBuilder().startObject().field("max_num_segments", 1).endObject().toString();
-        final FakeRestRequest request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(
-            new BytesArray(json),
-            MediaTypeRegistry.JSON
-        ).withPath("/_forcemerge").build();
-        IllegalArgumentException e = expectThrows(
-            IllegalArgumentException.class,
-            () -> handler.handleRequest(request, new FakeRestChannel(request, randomBoolean(), 1), mock(NodeClient.class))
-        );
-        assertThat(e.getMessage(), equalTo("request [GET /_forcemerge] does not support having a body"));
-    }
-
     public void testDeprecationMessage() {
         final Map<String, String> params = new HashMap<>();
         params.put("only_expunge_deletes", Boolean.TRUE.toString());

diff --git a/server/src/test/java/org/opensearch/rest/BaseRestHandlerTests.java b/server/src/test/java/org/opensearch/rest/BaseRestHandlerTests.java
@@ -35,10 +35,6 @@
 import org.opensearch.client.node.NodeClient;
 import org.opensearch.common.Table;
 import org.opensearch.common.settings.Settings;
-import org.opensearch.common.xcontent.json.JsonXContent;
-import org.opensearch.core.common.bytes.BytesArray;
-import org.opensearch.core.xcontent.MediaTypeRegistry;
-import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.rest.RestHandler.ReplacedRoute;
 import org.opensearch.rest.RestHandler.Route;
 import org.opensearch.rest.RestRequest.Method;
@@ -281,81 +277,6 @@ public String getName() {
         assertTrue(executed.get());
     }
 
-    public void testConsumedBody() throws Exception {
-        final AtomicBoolean executed = new AtomicBoolean();
-        final BaseRestHandler handler = new BaseRestHandler() {
-            @Override
-            protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
-                request.content();
-                return channel -> executed.set(true);
-            }
-
-            @Override
-            public String getName() {
-                return "test_consumed_body";
-            }
-        };
-
-        try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) {
-            final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent(
-                new BytesArray(builder.toString()),
-                MediaTypeRegistry.JSON
-            ).build();
-            final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
-            handler.handleRequest(request, channel, mockClient);
-            assertTrue(executed.get());
-        }
-    }
-
-    public void testUnconsumedNoBody() throws Exception {
-        final AtomicBoolean executed = new AtomicBoolean();
-        final BaseRestHandler handler = new BaseRestHandler() {
-            @Override
-            protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
-                return channel -> executed.set(true);
-            }
-
-            @Override
-            public String getName() {
-                return "test_unconsumed_body";
-            }
-        };
-
-        final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).build();
-        final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
-        handler.handleRequest(request, channel, mockClient);
-        assertTrue(executed.get());
-    }
-
-    public void testUnconsumedBody() throws IOException {
-        final AtomicBoolean executed = new AtomicBoolean();
-        final BaseRestHandler handler = new BaseRestHandler() {
-            @Override
-            protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
-                return channel -> executed.set(true);
-            }
-
-            @Override
-            public String getName() {
-                return "test_unconsumed_body";
-            }
-        };
-
-        try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) {
-            final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent(
-                new BytesArray(builder.toString()),
-                MediaTypeRegistry.JSON
-            ).build();
-            final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
-            final IllegalArgumentException e = expectThrows(
-                IllegalArgumentException.class,
-                () -> handler.handleRequest(request, channel, mockClient)
-            );
-            assertThat(e, hasToString(containsString("request [GET /] does not support having a body")));
-            assertFalse(executed.get());
-        }
-    }
-
     public void testReplaceRoutesMethod() throws Exception {
         List<Route> routes = Arrays.asList(new Route(Method.GET, "/path/test"), new Route(Method.PUT, "/path2/test"));
         List<ReplacedRoute> replacedRoutes = RestHandler.replaceRoutes(routes, "/prefix", "/deprecatedPrefix");

diff --git a/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java b/server/src/test/java/org/opensearch/search/pit/RestDeletePitActionTests.java
@@ -82,31 +82,6 @@ public void deletePits(DeletePitRequest request, ActionListener<DeletePitRespons
         }
     }
 
-    public void testDeleteAllPitWithBody() {
-        SetOnce<Boolean> pitCalled = new SetOnce<>();
-        try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) {
-            @Override
-            public void deletePits(DeletePitRequest request, ActionListener<DeletePitResponse> listener) {
-                pitCalled.set(true);
-                assertThat(request.getPitIds(), hasSize(1));
-                assertThat(request.getPitIds().get(0), equalTo("_all"));
-            }
-        }) {
-            RestDeletePitAction action = new RestDeletePitAction();
-            RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withContent(
-                new BytesArray("{\"pit_id\": [\"BODY\"]}"),
-                MediaTypeRegistry.JSON
-            ).withPath("/_all").build();
-            FakeRestChannel channel = new FakeRestChannel(request, false, 0);
-
-            IllegalArgumentException ex = expectThrows(
-                IllegalArgumentException.class,
-                () -> action.handleRequest(request, channel, nodeClient)
-            );
-            assertTrue(ex.getMessage().contains("request [GET /_all] does not support having a body"));
-        }
-    }
-
     public void testDeletePitQueryStringParamsShouldThrowException() {
         SetOnce<Boolean> pitCalled = new SetOnce<>();
         try (NodeClient nodeClient = new NoOpNodeClient(this.getTestName()) {