Skip to content

Commit d660b89

Browse files
zane-neozane-neo
authored
Fix error when non-exsiting doc is found replacing global tenant id (#258)
* Fix error when un-exsiting doc is foundd during replacing global tenant id Signed-off-by: zane-neo <[email protected]> * Fix deserialization exception when using parser Signed-off-by: zane-neo <[email protected]> * different approach to handle parse toknen is null issue Signed-off-by: zane-neo <[email protected]> * Add UT to increase code coverage Signed-off-by: zane-neo <[email protected]> * address commnets Signed-off-by: zane-neo <[email protected]> --------- Signed-off-by: zane-neo <[email protected]>
1 parent 9cae4c1 commit d660b89

File tree

7 files changed

+74
-23
lines changed

7 files changed

+74
-23
lines changed

‎core/src/main/java/org/opensearch/remote/metadata/client/AbstractSdkClient.java‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,10 @@ protected CompletionStage<GetDataObjectResponse> addToGlobalResourceCache(
139139
CompletionStage<GetDataObjectResponse> dataFetchedWithGlobalTenantId
140140
) {
141141
return dataFetchedWithGlobalTenantId.thenCompose(res -> {
142+
GetResponse getResponse = res.getResponse();
143+
if (getResponse == null || !getResponse.isExists()) {
144+
return CompletableFuture.completedFuture(res);
145+
}
142146
if (globalResourceCacheTTL.getMillis() == 0) {
143147
return CompletableFuture.completedFuture(replaceGlobalTenantId(request, res));
144148
}
@@ -168,14 +172,9 @@ protected GetDataObjectResponse getGlobalResourceDataFromCache(GetDataObjectRequ
168172
}
169173

170174
private GetDataObjectResponse replaceGlobalTenantId(GetDataObjectRequest request, GetDataObjectResponse response) {
171-
response.source().put(TENANT_ID_FIELD_KEY, request.tenantId());
172175
GetResponse getResponse = response.getResponse();
173-
if (getResponse == null) {
174-
throw new OpenSearchStatusException(
175-
"Cached response is null, please check configuration with system admin!",
176-
RestStatus.INTERNAL_SERVER_ERROR
177-
);
178-
}
176+
assert getResponse != null : "GetResponse shouldn't be null";
177+
response.source().put(TENANT_ID_FIELD_KEY, request.tenantId());
179178
try {
180179
JsonNode jsonNode = OBJECT_MAPPER.readTree(getResponse.toString());
181180
((ObjectNode) jsonNode.get("_source")).put(TENANT_ID_FIELD_KEY, Optional.ofNullable(request.tenantId()).orElse(DEFAULT_TENANT));

‎core/src/main/java/org/opensearch/remote/metadata/client/GetDataObjectResponse.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ public Map<String, Object> source() {
7777
*/
7878
public @Nullable GetResponse getResponse() {
7979
if (this.getResponse == null) {
80-
try {
81-
this.getResponse = GetResponse.fromXContent(parser());
80+
try (XContentParser parser = parser()) {
81+
this.getResponse = GetResponse.fromXContent(parser);
8282
return this.getResponse;
8383
} catch (IOException | NullPointerException e) {
8484
return null;
@@ -89,7 +89,7 @@ public Map<String, Object> source() {
8989

9090
@Override
9191
public XContentParser parser() {
92-
if (super.parser() == null) {
92+
if (super.parser() == null || super.parser().isClosed()) {
9393
try {
9494
return SdkClientUtils.createParser(this.getResponse);
9595
} catch (IOException | NullPointerException e) {

‎core/src/main/java/org/opensearch/remote/metadata/client/SdkClient.java‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public SdkClient(SdkClientDelegate delegate, Executor defaultExecutor, Boolean m
6464
* @return A completion stage encapsulating the response or exception
6565
*/
6666
public CompletionStage<PutDataObjectResponse> putDataObjectAsync(PutDataObjectRequest request, Executor executor) {
67-
validateTenantId(request.tenantId());
67+
validateTenantId(request.tenantId(), false);
6868
return delegate.putDataObjectAsync(request, executor, isMultiTenancyEnabled);
6969
}
7070

@@ -108,7 +108,7 @@ public CompletionStage<GetDataObjectResponse> getDataObjectAsync(GetDataObjectRe
108108
* @return A completion stage encapsulating the response or exception
109109
*/
110110
public CompletionStage<GetDataObjectResponse> getDataObjectAsync(GetDataObjectRequest request) {
111-
validateTenantId(request.tenantId());
111+
validateTenantId(request.tenantId(), true);
112112
return getDataObjectAsync(request, defaultExecutor);
113113
}
114114

@@ -133,7 +133,7 @@ public GetDataObjectResponse getDataObject(GetDataObjectRequest request) {
133133
* @return A completion stage encapsulating the response or exception
134134
*/
135135
public CompletionStage<UpdateDataObjectResponse> updateDataObjectAsync(UpdateDataObjectRequest request, Executor executor) {
136-
validateTenantId(request.tenantId());
136+
validateTenantId(request.tenantId(), false);
137137
return delegate.updateDataObjectAsync(request, executor, isMultiTenancyEnabled);
138138
}
139139

@@ -168,7 +168,7 @@ public UpdateDataObjectResponse updateDataObject(UpdateDataObjectRequest request
168168
* @return A completion stage encapsulating the response or exception
169169
*/
170170
public CompletionStage<DeleteDataObjectResponse> deleteDataObjectAsync(DeleteDataObjectRequest request, Executor executor) {
171-
validateTenantId(request.tenantId());
171+
validateTenantId(request.tenantId(), false);
172172
return delegate.deleteDataObjectAsync(request, executor, isMultiTenancyEnabled);
173173
}
174174

@@ -239,7 +239,7 @@ public BulkDataObjectResponse bulkDataObject(BulkDataObjectRequest request) {
239239
* @return A completion stage encapsulating the response or exception
240240
*/
241241
public CompletionStage<SearchDataObjectResponse> searchDataObjectAsync(SearchDataObjectRequest request, Executor executor) {
242-
validateTenantId(request.tenantId());
242+
validateTenantId(request.tenantId(), false);
243243
return delegate.searchDataObjectAsync(request, executor, isMultiTenancyEnabled);
244244
}
245245

@@ -275,22 +275,24 @@ public SdkClientDelegate getDelegate() {
275275
}
276276

277277
/**
278-
* Throw exception if tenantId is null and multitenancy is enabled
279-
* @param tenantId The tenantId from the request
278+
* Throw exception if tenantId is null and multitenancy is enabled, or the present tenant id equals to global tenant id.
279+
*
280+
* @param tenantId The tenantId from the request
281+
* @param allowGlobalTenantId Whether to allow global tenant id or not
280282
*/
281-
private void validateTenantId(String tenantId) {
283+
private void validateTenantId(String tenantId, boolean allowGlobalTenantId) {
282284
if (Boolean.TRUE.equals(isMultiTenancyEnabled)) {
283285
if (Strings.isNullOrEmpty(tenantId)) {
284286
throw new IllegalArgumentException("A tenant ID is required when multitenancy is enabled.");
285-
} else if (tenantId.equals(globalTenantId)) {
287+
} else if (!allowGlobalTenantId && tenantId.equals(globalTenantId)) {
286288
throw new OpenSearchStatusException(NO_PERMISSION_TO_OPERATE_GLOBAL_RESOURCE, RestStatus.FORBIDDEN);
287289
}
288290
}
289291
}
290292

291293
/**
292294
* Throw exception if tenantId is null for any bulk request and multitenancy is enabled
293-
* @param tenantId The tenantId from the request
295+
* @param requests The request contains tenantIds
294296
*/
295297
private void validateTenantIds(List<DataObjectRequest> requests) {
296298
if (Boolean.TRUE.equals(isMultiTenancyEnabled)

‎core/src/test/java/org/opensearch/remote/metadata/client/GetDataObjectResponseTests.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import org.opensearch.action.get.GetResponse;
1212
import org.opensearch.core.rest.RestStatus;
1313
import org.opensearch.core.xcontent.XContentParser;
14+
import org.junit.jupiter.api.Assertions;
1415
import org.junit.jupiter.api.BeforeEach;
1516
import org.junit.jupiter.api.Test;
1617

@@ -91,4 +92,24 @@ public void testGetDataObjectResponseWithNullSource() {
9192

9293
assertEquals(Collections.emptyMap(), response.source());
9394
}
95+
96+
@Test
97+
public void test_parser_nullOrClosed() {
98+
GetDataObjectResponse response1 = new GetDataObjectResponse(testIndex, testId, null, testFailed, testCause, testStatus, testSource);
99+
XContentParser parser1 = response1.parser();
100+
Assertions.assertNull(parser1);
101+
102+
when(testParser.isClosed()).thenReturn(true);
103+
GetDataObjectResponse response2 = new GetDataObjectResponse(
104+
testIndex,
105+
testId,
106+
testParser,
107+
testFailed,
108+
testCause,
109+
testStatus,
110+
testSource
111+
);
112+
XContentParser parser2 = response2.parser();
113+
Assertions.assertNull(parser2);
114+
}
94115
}

‎core/src/test/java/org/opensearch/remote/metadata/client/SdkClientTests.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import static org.junit.Assert.assertSame;
3333
import static org.junit.jupiter.api.Assertions.assertEquals;
3434
import static org.junit.jupiter.api.Assertions.assertFalse;
35+
import static org.junit.jupiter.api.Assertions.assertNotNull;
3536
import static org.junit.jupiter.api.Assertions.assertThrows;
3637
import static org.junit.jupiter.api.Assertions.assertTrue;
3738
import static org.mockito.ArgumentMatchers.any;
@@ -221,8 +222,8 @@ public void testGetDataObjectNullTenantId() {
221222
@Test
222223
public void testGetDataObjectGlobalTenantId() {
223224
when(getRequest.tenantId()).thenReturn(GLOBAL_TENANT_ID);
224-
OpenSearchStatusException exception = assertThrows(OpenSearchStatusException.class, () -> sdkClient.getDataObject(getRequest));
225-
assertEquals(NO_PERMISSION_TO_OPERATE_GLOBAL_RESOURCE, exception.getMessage());
225+
GetDataObjectResponse response = sdkClient.getDataObject(getRequest);
226+
assertNotNull(response);
226227
}
227228

228229
@Test

‎ddb-client/src/main/java/org/opensearch/remote/metadata/client/impl/DDBOpenSearchClient.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,7 @@ public CompletionStage<GetDataObjectResponse> getDataObjectAsync(
294294
// fetch resource with user tenant id.
295295
CompletionStage<GetDataObjectResponse> getDataFromDynamoDB = innerGetDataObjectAsync(request, executor, isMultiTenancyEnabled);
296296
return getDataFromDynamoDB.thenCompose(response -> {
297-
// return the document if it's not exist.
297+
// return the document if it's exist under user tenant id.
298298
if (Optional.ofNullable(response).map(GetDataObjectResponse::getResponse).map(GetResponse::isExists).orElse(false)) {
299299
return CompletableFuture.completedFuture(response);
300300
}

‎ddb-client/src/test/java/org/opensearch/remote/metadata/client/impl/DDBOpenSearchClientTests.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1315,6 +1315,34 @@ public void testGetDataObject_globalTenantIdEnabled_userTenantIdResourceFound()
13151315
assertEquals(TEST_TENANT_ID, getDataObjectResponse.getResponse().getSourceAsMap().get(TENANT_ID_FIELD_KEY));
13161316
}
13171317

1318+
@Test
1319+
public void testGetDataObject_globalTenantIdEnabled_resourceNotFound() {
1320+
GetDataObjectRequest getRequest = GetDataObjectRequest.builder().index(TEST_INDEX).id("unExistId").tenantId(TEST_TENANT_ID).build();
1321+
Map<String, AttributeValue> itemResponse = new HashMap<>();
1322+
GetItemResponse getItemResponse = GetItemResponse.builder().item(itemResponse).build();
1323+
when(dynamoDbAsyncClient.getItem(any(GetItemRequest.class))).thenReturn(CompletableFuture.completedFuture(getItemResponse));
1324+
SdkClient sdkClient = SdkClientFactory.wrapSdkClientDelegate(
1325+
new DDBOpenSearchClient(
1326+
dynamoDbAsyncClient,
1327+
aosOpenSearchClient,
1328+
Map.of(
1329+
TENANT_ID_FIELD_KEY,
1330+
TEST_TENANT_ID,
1331+
REMOTE_METADATA_GLOBAL_TENANT_ID_KEY,
1332+
GLOBAL_TENANT_ID,
1333+
REMOTE_METADATA_GLOBAL_RESOURCE_CACHE_TTL_KEY,
1334+
TEST_GLOBAL_RESOURCE_CACHE_TTL
1335+
)
1336+
),
1337+
true,
1338+
GLOBAL_TENANT_ID
1339+
);
1340+
GetDataObjectResponse getDataObjectResponse = sdkClient.getDataObjectAsync(getRequest, testThreadPool.executor(TEST_THREAD_POOL))
1341+
.toCompletableFuture()
1342+
.join(); // Read from cache.
1343+
assertFalse(getDataObjectResponse.getResponse().isExists());
1344+
}
1345+
13181346
@Test
13191347
public void testIsGlobalResource_globalTenantIdEnabled_foundGlobalResourceFromDDB() {
13201348
Map<String, AttributeValue> itemResponse = new HashMap<>();

0 commit comments

Comments
 (0)