Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix response codes returned by JSON formatting them #2156

Merged
merged 14 commits into from
Oct 3, 2023
Merged

Fix response codes returned by JSON formatting them #2156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
75e9f04
Glue datasource support (#2055)
vamsi-amazon Sep 9, 2023
cda01e9
Initial commit of new job APIs (#2050)
vamsi-amazon Sep 9, 2023
71f4155
Create Job API (#2070)
vamsi-amazon Sep 21, 2023
be82714
Cancel Job API (#2126)
vamsi-amazon Sep 21, 2023
6783ed7
Fix response codes returned
derek-ho Sep 26, 2023
96f3ef0
Remove @opensearch datasource, update with new type
derek-ho Sep 26, 2023
fb2281e
Spotless Apply
derek-ho Sep 26, 2023
9c5ffb8
Fix tests
derek-ho Sep 26, 2023
03aba02
Revert change back to json string
derek-ho Sep 27, 2023
767a9cb
Update tests to use JSON string literal instead of formatting
derek-ho Sep 27, 2023
e772fde
Update IT
derek-ho Sep 27, 2023
0e37dca
Fix show datsources IT
derek-ho Sep 27, 2023
4363d4d
Merge remote-tracking branch 'remote/main' into fix-response-code
derek-ho Sep 28, 2023
97b8985
Remove files from merge
derek-ho Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/java/org/opensearch/sql/planner/physical/datasource/DataSourceTableScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public String explain() {
@Override
public void open() {
List<ExprValue> exprValues = new ArrayList<>();
Set<DataSourceMetadata> dataSourceMetadataSet = dataSourceService.getDataSourceMetadata(true);
Set<DataSourceMetadata> dataSourceMetadataSet = dataSourceService.getDataSourceMetadata(false);
penghuo marked this conversation as resolved.
Show resolved Hide resolved
for (DataSourceMetadata dataSourceMetadata : dataSourceMetadataSet) {
exprValues.add(
new ExprTupleValue(
Expand Down
2 changes: 1 addition & 1 deletion ...src/test/java/org/opensearch/sql/planner/physical/datasource/DataSourceTableScanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ void testIterator() {
Collections.emptyList(),
ImmutableMap.of()))
.collect(Collectors.toSet());
when(dataSourceService.getDataSourceMetadata(true)).thenReturn(dataSourceMetadata);
when(dataSourceService.getDataSourceMetadata(false)).thenReturn(dataSourceMetadata);

assertFalse(dataSourceTableScan.hasNext());
dataSourceTableScan.open();
Expand Down
14 changes: 11 additions & 3 deletions ...c/main/java/org/opensearch/sql/datasources/transport/TransportCreateDataSourceAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

package org.opensearch.sql.datasources.transport;

import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY;

import org.opensearch.action.ActionType;
import org.opensearch.action.support.ActionFilters;
import org.opensearch.action.support.HandledTransportAction;
Expand All @@ -17,6 +19,7 @@
import org.opensearch.sql.datasources.model.transport.CreateDataSourceActionRequest;
import org.opensearch.sql.datasources.model.transport.CreateDataSourceActionResponse;
import org.opensearch.sql.datasources.service.DataSourceServiceImpl;
import org.opensearch.sql.protocol.response.format.JsonResponseFormatter;
import org.opensearch.tasks.Task;
import org.opensearch.transport.TransportService;

Expand Down Expand Up @@ -56,9 +59,14 @@ protected void doExecute(
try {
DataSourceMetadata dataSourceMetadata = request.getDataSourceMetadata();
dataSourceService.createDataSource(dataSourceMetadata);
actionListener.onResponse(
new CreateDataSourceActionResponse(
"Created DataSource with name " + dataSourceMetadata.getName()));
String responseContent =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return string in json format, not schema required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning behind this change was to have a response class and later on, if we have time we can do something like return the DataSourceMetadata that was Updated/Created. Don't think we will have time for that/I don't know what the standard is for SQL repo - but I reverted it back to just a JSON string for now - it should be fine as long as the UI can parse it.

new JsonResponseFormatter<String>(PRETTY) {
@Override
protected Object buildJsonObject(String response) {
return response;
}
}.format("Created DataSource with name " + dataSourceMetadata.getName());
actionListener.onResponse(new CreateDataSourceActionResponse(responseContent));
} catch (Exception e) {
actionListener.onFailure(e);
}
Expand Down
14 changes: 11 additions & 3 deletions ...c/main/java/org/opensearch/sql/datasources/transport/TransportUpdateDataSourceAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

package org.opensearch.sql.datasources.transport;

import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY;

import org.opensearch.action.ActionType;
import org.opensearch.action.support.ActionFilters;
import org.opensearch.action.support.HandledTransportAction;
Expand All @@ -16,6 +18,7 @@
import org.opensearch.sql.datasources.model.transport.UpdateDataSourceActionRequest;
import org.opensearch.sql.datasources.model.transport.UpdateDataSourceActionResponse;
import org.opensearch.sql.datasources.service.DataSourceServiceImpl;
import org.opensearch.sql.protocol.response.format.JsonResponseFormatter;
import org.opensearch.tasks.Task;
import org.opensearch.transport.TransportService;

Expand Down Expand Up @@ -55,9 +58,14 @@ protected void doExecute(
ActionListener<UpdateDataSourceActionResponse> actionListener) {
try {
dataSourceService.updateDataSource(request.getDataSourceMetadata());
actionListener.onResponse(
new UpdateDataSourceActionResponse(
"Updated DataSource with name " + request.getDataSourceMetadata().getName()));
String responseContent =
new JsonResponseFormatter<String>(PRETTY) {
@Override
protected Object buildJsonObject(String response) {
return response;
}
}.format("Updated DataSource with name " + request.getDataSourceMetadata().getName());
actionListener.onResponse(new UpdateDataSourceActionResponse(responseContent));
} catch (Exception e) {
actionListener.onFailure(e);
}
Expand Down
4 changes: 2 additions & 2 deletions ...st/java/org/opensearch/sql/datasources/transport/TransportCreateDataSourceActionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ public void testDoExecute() {
.onResponse(createDataSourceActionResponseArgumentCaptor.capture());
CreateDataSourceActionResponse createDataSourceActionResponse =
createDataSourceActionResponseArgumentCaptor.getValue();
Assertions.assertEquals(
"Created DataSource with name test_datasource", createDataSourceActionResponse.getResult());
String responseAsJson = "\"Created DataSource with name test_datasource\"";
Assertions.assertEquals(responseAsJson, createDataSourceActionResponse.getResult());
}

@Test
Expand Down
5 changes: 3 additions & 2 deletions ...st/java/org/opensearch/sql/datasources/transport/TransportUpdateDataSourceActionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,9 @@ public void testDoExecute() {
.onResponse(updateDataSourceActionResponseArgumentCaptor.capture());
UpdateDataSourceActionResponse updateDataSourceActionResponse =
updateDataSourceActionResponseArgumentCaptor.getValue();
Assertions.assertEquals(
"Updated DataSource with name test_datasource", updateDataSourceActionResponse.getResult());
String responseAsJson = "\"Updated DataSource with name test_datasource\"";

Assertions.assertEquals(responseAsJson, updateDataSourceActionResponse.getResult());
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions integ-test/src/test/java/org/opensearch/sql/datasource/DataSourceAPIsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public void createDataSourceAPITest() {
Response response = client().performRequest(createRequest);
Assert.assertEquals(201, response.getStatusLine().getStatusCode());
String createResponseString = getResponseBody(response);
Assert.assertEquals("Created DataSource with name create_prometheus", createResponseString);
Assert.assertEquals("\"Created DataSource with name create_prometheus\"", createResponseString);
// Datasource is not immediately created. so introducing a sleep of 2s.
Thread.sleep(2000);

Expand Down Expand Up @@ -109,7 +109,7 @@ public void updateDataSourceAPITest() {
Response updateResponse = client().performRequest(updateRequest);
Assert.assertEquals(200, updateResponse.getStatusLine().getStatusCode());
String updateResponseString = getResponseBody(updateResponse);
Assert.assertEquals("Updated DataSource with name update_prometheus", updateResponseString);
Assert.assertEquals("\"Updated DataSource with name update_prometheus\"", updateResponseString);

// Datasource is not immediately updated. so introducing a sleep of 2s.
Thread.sleep(2000);
Expand Down
2 changes: 1 addition & 1 deletion integ-test/src/test/java/org/opensearch/sql/ppl/ShowDataSourcesCommandIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ protected void deleteDataSourceMetadata() throws IOException {
@Test
public void testShowDataSourcesCommands() throws IOException {
JSONObject result = executeQuery("show datasources");
verifyDataRows(result, rows("my_prometheus", "PROMETHEUS"), rows("@opensearch", "OPENSEARCH"));
verifyDataRows(result, rows("my_prometheus", "PROMETHEUS"));
verifyColumn(result, columnName("DATASOURCE_NAME"), columnName("CONNECTOR_TYPE"));
}

Expand Down
Loading