Skip to content

Commit

Permalink
Following up on PR feedback
Browse files Browse the repository at this point in the history 
Signed-off-by: Peter Nied <[email protected]>
  • Loading branch information
@peternied
peternied committed Feb 6, 2024
1 parent 327d158 commit b95056a
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 109 deletions.
2 changes: 1 addition & 1 deletion server/src/internalClusterTest/java/org/opensearch/action/admin/indices/view/ViewIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@

package org.opensearch.action.admin.indices.view;

import org.hamcrest.MatcherAssert;
import org.opensearch.action.search.SearchRequest;
import org.opensearch.action.search.SearchResponse;
import org.opensearch.index.IndexNotFoundException;
import org.opensearch.test.BackgroundIndexer;
import org.opensearch.test.OpenSearchIntegTestCase;
import org.opensearch.test.OpenSearchIntegTestCase.ClusterScope;
import org.opensearch.test.OpenSearchIntegTestCase.Scope;
import org.hamcrest.MatcherAssert;

import java.util.List;

Expand Down
137 changes: 43 additions & 94 deletions server/src/main/java/org/opensearch/rest/action/admin/indices/RestViewAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@

package org.opensearch.rest.action.admin.indices;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.opensearch.action.admin.indices.view.CreateViewAction;
import org.opensearch.action.admin.indices.view.DeleteViewAction;
import org.opensearch.action.admin.indices.view.GetViewAction;
Expand Down Expand Up @@ -41,12 +39,11 @@
@ExperimentalApi
public class RestViewAction {

private final static Logger LOG = LogManager.getLogger(RestViewAction.class);

public static final String VIEW_ID = "view_id";
public static final String VIEW_ID_PARAMETER = "{" + VIEW_ID + "}";
public static final String VIEW_NAME = "view_name";
public static final String VIEW_NAME_PARAMETER = "{" + VIEW_NAME + "}";

/** Handler for create view */
@ExperimentalApi
public static class CreateViewHandler extends BaseRestHandler {

@Override
Expand All @@ -63,18 +60,25 @@ public String getName() {
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
try (final XContentParser parser = request.contentParser()) {
final CreateViewAction.Request createViewAction = CreateViewAction.Request.fromXContent(parser);

final ValidationException validationResult = createViewAction.validate();
if (validationResult != null) {
throw validationResult;
}

return channel -> client.admin().indices().createView(createViewAction, new RestToXContentListener<>(channel));
}
}
}

/** Handler for delete view */
@ExperimentalApi
public static class DeleteViewHandler extends BaseRestHandler {

@Override
public List<Route> routes() {
return List.of(
new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER).method(DELETE).uniqueName(DeleteViewAction.NAME).build()
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER).method(DELETE).uniqueName(DeleteViewAction.NAME).build()
);
}

Expand All @@ -85,20 +89,27 @@ public String getName() {

@Override
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
final String viewId = request.param(VIEW_ID);
final String viewId = request.param(VIEW_NAME);

final DeleteViewAction.Request deleteRequest = new DeleteViewAction.Request(viewId);

final ValidationException validationResult = deleteRequest.validate();
if (validationResult != null) {
throw validationResult;
}

return channel -> client.admin().indices().deleteView(deleteRequest, new RestToXContentListener<>(channel));
}
}

/** Handler for update view */
@ExperimentalApi
public static class UpdateViewHandler extends BaseRestHandler {

@Override
public List<Route> routes() {
return List.of(
new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER).method(PUT).uniqueName(UpdateViewAction.NAME).build()
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER).method(PUT).uniqueName(UpdateViewAction.NAME).build()
);
}

Expand All @@ -109,21 +120,30 @@ public String getName() {

@Override
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
final String viewId = request.param(VIEW_ID);
final String viewId = request.param(VIEW_NAME);

try (final XContentParser parser = request.contentParser()) {
final CreateViewAction.Request updateRequest = UpdateViewAction.Request.fromXContent(parser, viewId);

final ValidationException validationResult = updateRequest.validate();
if (validationResult != null) {
throw validationResult;
}

return channel -> client.admin().indices().updateView(updateRequest, new RestToXContentListener<>(channel));
}
}
}

/** Handler for get view */
@ExperimentalApi
public static class GetViewHandler extends BaseRestHandler {

@Override
public List<Route> routes() {
return List.of(new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER).method(GET).uniqueName(GetViewAction.NAME).build());
return List.of(
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER).method(GET).uniqueName(GetViewAction.NAME).build()
);
}

@Override
Expand All @@ -133,14 +153,21 @@ public String getName() {

@Override
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
final String viewId = request.param(VIEW_ID);
final String viewId = request.param(VIEW_NAME);

final GetViewAction.Request getRequest = new GetViewAction.Request(viewId);

final ValidationException validationResult = getRequest.validate();
if (validationResult != null) {
throw validationResult;
}

return channel -> client.admin().indices().getView(getRequest, new RestToXContentListener<>(channel));
}
}

/** Handler for get view */
@ExperimentalApi
public static class ListViewNamesHandler extends BaseRestHandler {

@Override
Expand All @@ -160,15 +187,16 @@ protected RestChannelConsumer prepareRequest(final RestRequest request, final No
}

/** Handler for search view */
@ExperimentalApi
public static class SearchViewHandler extends BaseRestHandler {
@Override
public List<Route> routes() {
return List.of(
new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER + "/_search")
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER + "/_search")
.method(GET)
.uniqueName(SearchViewAction.NAME)
.build(),
new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER + "/_search")
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER + "/_search")
.method(POST)
.uniqueName(SearchViewAction.NAME)
.build()
Expand All @@ -182,7 +210,7 @@ public String getName() {

@Override
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
final String viewId = request.param(VIEW_ID);
final String viewId = request.param(VIEW_NAME);

final SearchViewAction.Request viewSearchRequest = new SearchViewAction.Request(viewId);
final IntConsumer setSize = size -> viewSearchRequest.source().size(size);
Expand All @@ -208,83 +236,4 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
};
}
}

// TODO: Replace and reorganize this layout

// public List<Route> routes() {

// return List.of(
// new NamedRoute.Builder().path("/views").method(GET).uniqueName("cluster:views:list").build(),
// new NamedRoute.Builder().path("/views/" + viewIdParameter).method(GET).uniqueName("cluster:views:get").build(),
// new NamedRoute.Builder().path("/views/" + viewIdParameter).method(DELETE).uniqueName("cluster:views:delete").build(),
// new NamedRoute.Builder().path("/views/" + viewIdParameter).method(PUT).uniqueName("cluster:views:update").build()
// );
// }

// public RestResponse handlePost(final RestRequest r, final RestChannel channel) throws IOException {
// final View inputView;
// try (final XContentParser parser = r.contentParser()) {
// inputView = View.fromXContent(parser);
// }

// final long currentTime = System.currentTimeMillis();
// final View view = new View(inputView.name, inputView.description, currentTime, currentTime, inputView.targets);

// clusterService.submitStateUpdateTask("create_view_task", new ClusterStateUpdateTask() {
// @Override
// public ClusterState execute(final ClusterState currentState) throws Exception {
// return new ClusterState.Builder(clusterService.state()).metadata(Metadata.builder(currentState.metadata()).put(view))
// .build();
// }

// @Override
// public void onFailure(final String source, final Exception e) {
// LOG.error("Unable to create view, due to {}", source, e);
// channel.sendResponse(
// new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, "Unknown error occurred, see the log for details.")
// );
// }

// @Override
// public void clusterStateProcessed(final String source, final ClusterState oldState, final ClusterState newState) {
// try {
// channel.sendResponse(
// new BytesRestResponse(RestStatus.CREATED, channel.newBuilder().startObject().field(view.name, view).endObject())
// );
// } catch (final IOException e) {
// // TODO?
// LOG.error(e);
// }
// }
// });
// // TODO: Handle CREATED vs UPDATED
// return null;
// }

// public RestResponse handleSingleGet(final RestRequest r, final XContentBuilder builder) throws IOException {
// final String viewId = r.param(VIEW_ID);

// if (Strings.isNullOrEmpty(viewId)) {
// return new BytesRestResponse(RestStatus.NOT_FOUND, "");
// }

// final Optional<View> view = Optional.ofNullable(clusterService.state().getMetadata())
// .map(m -> m.views())
// .map(views -> views.get(viewId));

// if (view.isEmpty()) {
// return new BytesRestResponse(RestStatus.NOT_FOUND, "");
// }

// return new BytesRestResponse(RestStatus.OK, builder.startObject().value(view).endObject());
// }

// public RestResponse handleSinglePut(final RestRequest r) {
// return new BytesRestResponse(RestStatus.NOT_IMPLEMENTED, "");
// }

// public RestResponse handleSingleDelete(final RestRequest r) {
// return new BytesRestResponse(RestStatus.NOT_IMPLEMENTED, "");
// }

}
38 changes: 24 additions & 14 deletions server/src/test/java/org/opensearch/action/admin/indices/view/ViewServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@

package org.opensearch.action.admin.indices.view;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.opensearch.ResourceNotFoundException;
import org.opensearch.action.search.SearchAction;
import org.opensearch.client.node.NodeClient;
Expand All @@ -20,25 +17,38 @@
import org.opensearch.cluster.metadata.View;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.core.action.ActionListener;
import org.hamcrest.MatcherAssert;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.LongSupplier;

import static org.hamcrest.Matchers.equalTo;
import static org.junit.Assert.*;
import static org.mockito.Mockito.*;


import static org.junit.Assert.assertThrows;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyString;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

@SuppressWarnings("unchecked")
public class ViewServiceTest {

private final View.Target typicalTarget = new View.Target("my-index-*");
private final View typicalView = new View("actualView", "actual description", -1L, -1L, List.of(typicalTarget));

private ClusterService clusterService;
private NodeClient nodeClient;
private AtomicLong currentTime = new AtomicLong(0);
private final AtomicLong currentTime = new AtomicLong(0);
private LongSupplier timeProvider = currentTime::longValue;
private ViewService viewService;

Expand Down Expand Up @@ -87,7 +97,7 @@ public void updateView_doesNotExist() {
doThrow(new ResourceNotFoundException("abc")).when(viewService).getViewOrThrowException(anyString());

final Exception ex = assertThrows(ResourceNotFoundException.class, () -> viewService.updateView(request, listener));
assertThat(ex.getMessage(), equalTo("abc"));
MatcherAssert.assertThat(ex.getMessage(), equalTo("abc"));
}

@Test
Expand All @@ -109,7 +119,7 @@ public void deleteView_doesNotExist() {

final ResourceNotFoundException ex = assertThrows(ResourceNotFoundException.class, () -> viewService.deleteView(request, listener));

assertThat(ex.getMessage(), equalTo("abc"));
MatcherAssert.assertThat(ex.getMessage(), equalTo("abc"));
}

@Test
Expand All @@ -131,14 +141,14 @@ public void getView_doesNotExist() {

final ResourceNotFoundException ex = assertThrows(ResourceNotFoundException.class, () -> viewService.getView(request, listener));

assertThat(ex.getMessage(), equalTo("abc"));
MatcherAssert.assertThat(ex.getMessage(), equalTo("abc"));
}

@Test
public void listViewNames() {
final var clusterState = new ClusterState.Builder(new ClusterName("MyCluster"))
.metadata(new Metadata.Builder().views(Map.of(typicalView.getName(), typicalView)).build())
.build();
final var clusterState = new ClusterState.Builder(new ClusterName("MyCluster")).metadata(
new Metadata.Builder().views(Map.of(typicalView.getName(), typicalView)).build()
).build();
final var listener = mock(ActionListener.class);
when(clusterService.state()).thenReturn(clusterState);

Expand Down

0 comments on commit b95056a

Please sign in to comment.