Skip to content

Commit 15a3a50

Browse files
peterniedpeternied
committed
Following up on PR feedback
Signed-off-by: Peter Nied <petern@amazon.com>
1 parent bacffe4 commit 15a3a50

File tree

3 files changed

+68
-109
lines changed

3 files changed

+68
-109
lines changed

‎server/src/internalClusterTest/java/org/opensearch/action/admin/indices/view/ViewIT.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,14 @@
88

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

11-
import org.hamcrest.MatcherAssert;
1211
import org.opensearch.action.search.SearchRequest;
1312
import org.opensearch.action.search.SearchResponse;
1413
import org.opensearch.index.IndexNotFoundException;
1514
import org.opensearch.test.BackgroundIndexer;
1615
import org.opensearch.test.OpenSearchIntegTestCase;
1716
import org.opensearch.test.OpenSearchIntegTestCase.ClusterScope;
1817
import org.opensearch.test.OpenSearchIntegTestCase.Scope;
18+
import org.hamcrest.MatcherAssert;
1919

2020
import java.util.List;
2121

‎server/src/main/java/org/opensearch/rest/action/admin/indices/RestViewAction.java

+43-94
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@
88

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

11-
import org.apache.logging.log4j.LogManager;
12-
import org.apache.logging.log4j.Logger;
1311
import org.opensearch.action.admin.indices.view.CreateViewAction;
1412
import org.opensearch.action.admin.indices.view.DeleteViewAction;
1513
import org.opensearch.action.admin.indices.view.GetViewAction;
@@ -41,12 +39,11 @@
4139
@ExperimentalApi
4240
public class RestViewAction {
4341

44-
private final static Logger LOG = LogManager.getLogger(RestViewAction.class);
45-
46-
public static final String VIEW_ID = "view_id";
47-
public static final String VIEW_ID_PARAMETER = "{" + VIEW_ID + "}";
42+
public static final String VIEW_NAME = "view_name";
43+
public static final String VIEW_NAME_PARAMETER = "{" + VIEW_NAME + "}";
4844

4945
/** Handler for create view */
46+
@ExperimentalApi
5047
public static class CreateViewHandler extends BaseRestHandler {
5148

5249
@Override
@@ -63,18 +60,25 @@ public String getName() {
6360
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
6461
try (final XContentParser parser = request.contentParser()) {
6562
final CreateViewAction.Request createViewAction = CreateViewAction.Request.fromXContent(parser);
63+
64+
final ValidationException validationResult = createViewAction.validate();
65+
if (validationResult != null) {
66+
throw validationResult;
67+
}
68+
6669
return channel -> client.admin().indices().createView(createViewAction, new RestToXContentListener<>(channel));
6770
}
6871
}
6972
}
7073

7174
/** Handler for delete view */
75+
@ExperimentalApi
7276
public static class DeleteViewHandler extends BaseRestHandler {
7377

7478
@Override
7579
public List<Route> routes() {
7680
return List.of(
77-
new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER).method(DELETE).uniqueName(DeleteViewAction.NAME).build()
81+
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER).method(DELETE).uniqueName(DeleteViewAction.NAME).build()
7882
);
7983
}
8084

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

8690
@Override
8791
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
88-
final String viewId = request.param(VIEW_ID);
92+
final String viewId = request.param(VIEW_NAME);
8993

9094
final DeleteViewAction.Request deleteRequest = new DeleteViewAction.Request(viewId);
95+
96+
final ValidationException validationResult = deleteRequest.validate();
97+
if (validationResult != null) {
98+
throw validationResult;
99+
}
100+
91101
return channel -> client.admin().indices().deleteView(deleteRequest, new RestToXContentListener<>(channel));
92102
}
93103
}
94104

95105
/** Handler for update view */
106+
@ExperimentalApi
96107
public static class UpdateViewHandler extends BaseRestHandler {
97108

98109
@Override
99110
public List<Route> routes() {
100111
return List.of(
101-
new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER).method(PUT).uniqueName(UpdateViewAction.NAME).build()
112+
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER).method(PUT).uniqueName(UpdateViewAction.NAME).build()
102113
);
103114
}
104115

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

110121
@Override
111122
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
112-
final String viewId = request.param(VIEW_ID);
123+
final String viewId = request.param(VIEW_NAME);
113124

114125
try (final XContentParser parser = request.contentParser()) {
115126
final CreateViewAction.Request updateRequest = UpdateViewAction.Request.fromXContent(parser, viewId);
127+
128+
final ValidationException validationResult = updateRequest.validate();
129+
if (validationResult != null) {
130+
throw validationResult;
131+
}
132+
116133
return channel -> client.admin().indices().updateView(updateRequest, new RestToXContentListener<>(channel));
117134
}
118135
}
119136
}
120137

121138
/** Handler for get view */
139+
@ExperimentalApi
122140
public static class GetViewHandler extends BaseRestHandler {
123141

124142
@Override
125143
public List<Route> routes() {
126-
return List.of(new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER).method(GET).uniqueName(GetViewAction.NAME).build());
144+
return List.of(
145+
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER).method(GET).uniqueName(GetViewAction.NAME).build()
146+
);
127147
}
128148

129149
@Override
@@ -133,14 +153,21 @@ public String getName() {
133153

134154
@Override
135155
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
136-
final String viewId = request.param(VIEW_ID);
156+
final String viewId = request.param(VIEW_NAME);
137157

138158
final GetViewAction.Request getRequest = new GetViewAction.Request(viewId);
159+
160+
final ValidationException validationResult = getRequest.validate();
161+
if (validationResult != null) {
162+
throw validationResult;
163+
}
164+
139165
return channel -> client.admin().indices().getView(getRequest, new RestToXContentListener<>(channel));
140166
}
141167
}
142168

143169
/** Handler for get view */
170+
@ExperimentalApi
144171
public static class ListViewNamesHandler extends BaseRestHandler {
145172

146173
@Override
@@ -160,15 +187,16 @@ protected RestChannelConsumer prepareRequest(final RestRequest request, final No
160187
}
161188

162189
/** Handler for search view */
190+
@ExperimentalApi
163191
public static class SearchViewHandler extends BaseRestHandler {
164192
@Override
165193
public List<Route> routes() {
166194
return List.of(
167-
new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER + "/_search")
195+
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER + "/_search")
168196
.method(GET)
169197
.uniqueName(SearchViewAction.NAME)
170198
.build(),
171-
new NamedRoute.Builder().path("/views/" + VIEW_ID_PARAMETER + "/_search")
199+
new NamedRoute.Builder().path("/views/" + VIEW_NAME_PARAMETER + "/_search")
172200
.method(POST)
173201
.uniqueName(SearchViewAction.NAME)
174202
.build()
@@ -182,7 +210,7 @@ public String getName() {
182210

183211
@Override
184212
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
185-
final String viewId = request.param(VIEW_ID);
213+
final String viewId = request.param(VIEW_NAME);
186214

187215
final SearchViewAction.Request viewSearchRequest = new SearchViewAction.Request(viewId);
188216
final IntConsumer setSize = size -> viewSearchRequest.source().size(size);
@@ -208,83 +236,4 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
208236
};
209237
}
210238
}
211-
212-
// TODO: Replace and reorganize this layout
213-
214-
// public List<Route> routes() {
215-
216-
// return List.of(
217-
// new NamedRoute.Builder().path("/views").method(GET).uniqueName("cluster:views:list").build(),
218-
// new NamedRoute.Builder().path("/views/" + viewIdParameter).method(GET).uniqueName("cluster:views:get").build(),
219-
// new NamedRoute.Builder().path("/views/" + viewIdParameter).method(DELETE).uniqueName("cluster:views:delete").build(),
220-
// new NamedRoute.Builder().path("/views/" + viewIdParameter).method(PUT).uniqueName("cluster:views:update").build()
221-
// );
222-
// }
223-
224-
// public RestResponse handlePost(final RestRequest r, final RestChannel channel) throws IOException {
225-
// final View inputView;
226-
// try (final XContentParser parser = r.contentParser()) {
227-
// inputView = View.fromXContent(parser);
228-
// }
229-
230-
// final long currentTime = System.currentTimeMillis();
231-
// final View view = new View(inputView.name, inputView.description, currentTime, currentTime, inputView.targets);
232-
233-
// clusterService.submitStateUpdateTask("create_view_task", new ClusterStateUpdateTask() {
234-
// @Override
235-
// public ClusterState execute(final ClusterState currentState) throws Exception {
236-
// return new ClusterState.Builder(clusterService.state()).metadata(Metadata.builder(currentState.metadata()).put(view))
237-
// .build();
238-
// }
239-
240-
// @Override
241-
// public void onFailure(final String source, final Exception e) {
242-
// LOG.error("Unable to create view, due to {}", source, e);
243-
// channel.sendResponse(
244-
// new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, "Unknown error occurred, see the log for details.")
245-
// );
246-
// }
247-
248-
// @Override
249-
// public void clusterStateProcessed(final String source, final ClusterState oldState, final ClusterState newState) {
250-
// try {
251-
// channel.sendResponse(
252-
// new BytesRestResponse(RestStatus.CREATED, channel.newBuilder().startObject().field(view.name, view).endObject())
253-
// );
254-
// } catch (final IOException e) {
255-
// // TODO?
256-
// LOG.error(e);
257-
// }
258-
// }
259-
// });
260-
// // TODO: Handle CREATED vs UPDATED
261-
// return null;
262-
// }
263-
264-
// public RestResponse handleSingleGet(final RestRequest r, final XContentBuilder builder) throws IOException {
265-
// final String viewId = r.param(VIEW_ID);
266-
267-
// if (Strings.isNullOrEmpty(viewId)) {
268-
// return new BytesRestResponse(RestStatus.NOT_FOUND, "");
269-
// }
270-
271-
// final Optional<View> view = Optional.ofNullable(clusterService.state().getMetadata())
272-
// .map(m -> m.views())
273-
// .map(views -> views.get(viewId));
274-
275-
// if (view.isEmpty()) {
276-
// return new BytesRestResponse(RestStatus.NOT_FOUND, "");
277-
// }
278-
279-
// return new BytesRestResponse(RestStatus.OK, builder.startObject().value(view).endObject());
280-
// }
281-
282-
// public RestResponse handleSinglePut(final RestRequest r) {
283-
// return new BytesRestResponse(RestStatus.NOT_IMPLEMENTED, "");
284-
// }
285-
286-
// public RestResponse handleSingleDelete(final RestRequest r) {
287-
// return new BytesRestResponse(RestStatus.NOT_IMPLEMENTED, "");
288-
// }
289-
290239
}

‎server/src/test/java/org/opensearch/action/admin/indices/view/ViewServiceTest.java

+24-14
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,6 @@
88

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

11-
import org.junit.After;
12-
import org.junit.Before;
13-
import org.junit.Test;
1411
import org.opensearch.ResourceNotFoundException;
1512
import org.opensearch.action.search.SearchAction;
1613
import org.opensearch.client.node.NodeClient;
@@ -20,25 +17,38 @@
2017
import org.opensearch.cluster.metadata.View;
2118
import org.opensearch.cluster.service.ClusterService;
2219
import org.opensearch.core.action.ActionListener;
20+
import org.hamcrest.MatcherAssert;
21+
import org.junit.After;
22+
import org.junit.Before;
23+
import org.junit.Test;
2324

2425
import java.util.List;
2526
import java.util.Map;
2627
import java.util.concurrent.atomic.AtomicLong;
2728
import java.util.function.LongSupplier;
2829

2930
import static org.hamcrest.Matchers.equalTo;
30-
import static org.junit.Assert.*;
31-
import static org.mockito.Mockito.*;
32-
33-
31+
import static org.junit.Assert.assertThrows;
32+
import static org.mockito.Mockito.any;
33+
import static org.mockito.Mockito.anyString;
34+
import static org.mockito.Mockito.doAnswer;
35+
import static org.mockito.Mockito.doThrow;
36+
import static org.mockito.Mockito.eq;
37+
import static org.mockito.Mockito.mock;
38+
import static org.mockito.Mockito.spy;
39+
import static org.mockito.Mockito.verify;
40+
import static org.mockito.Mockito.verifyNoMoreInteractions;
41+
import static org.mockito.Mockito.when;
42+
43+
@SuppressWarnings("unchecked")
3444
public class ViewServiceTest {
3545

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

3949
private ClusterService clusterService;
4050
private NodeClient nodeClient;
41-
private AtomicLong currentTime = new AtomicLong(0);
51+
private final AtomicLong currentTime = new AtomicLong(0);
4252
private LongSupplier timeProvider = currentTime::longValue;
4353
private ViewService viewService;
4454

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

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

93103
@Test
@@ -109,7 +119,7 @@ public void deleteView_doesNotExist() {
109119

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

112-
assertThat(ex.getMessage(), equalTo("abc"));
122+
MatcherAssert.assertThat(ex.getMessage(), equalTo("abc"));
113123
}
114124

115125
@Test
@@ -131,14 +141,14 @@ public void getView_doesNotExist() {
131141

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

134-
assertThat(ex.getMessage(), equalTo("abc"));
144+
MatcherAssert.assertThat(ex.getMessage(), equalTo("abc"));
135145
}
136146

137147
@Test
138148
public void listViewNames() {
139-
final var clusterState = new ClusterState.Builder(new ClusterName("MyCluster"))
140-
.metadata(new Metadata.Builder().views(Map.of(typicalView.getName(), typicalView)).build())
141-
.build();
149+
final var clusterState = new ClusterState.Builder(new ClusterName("MyCluster")).metadata(
150+
new Metadata.Builder().views(Map.of(typicalView.getName(), typicalView)).build()
151+
).build();
142152
final var listener = mock(ActionListener.class);
143153
when(clusterService.state()).thenReturn(clusterState);
144154

0 commit comments

Comments
 (0)