Skip to content

Commit d618df0

Browse files
zane-neorbhavna
zane-neo
and
rbhavna
authored
fix minor bugs and reformat error messsages (opensearch-project#953) (opensearch-project#1258)
* fix minor bugs and reformat error messsages Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com> Co-authored-by: Bhavana Ramaram <rbhavna@amazon.com>
1 parent a47db3a commit d618df0

12 files changed

+70
-60
lines changed

‎plugin/src/main/java/org/opensearch/ml/action/model_group/TransportRegisterModelGroupAction.java

+11-8
Original file line numberDiff line numberDiff line change
@@ -150,41 +150,44 @@ private void validateRequestForAccessControl(MLRegisterModelGroupInput input, Us
150150
Boolean isAddAllBackendRoles = input.getIsAddAllBackendRoles();
151151
if (modelAccessMode == null) {
152152
if (!Boolean.TRUE.equals(isAddAllBackendRoles) && CollectionUtils.isEmpty(input.getBackendRoles())) {
153-
throw new IllegalArgumentException("User must specify at least one backend role or make the model public/private");
153+
throw new IllegalArgumentException(
154+
"You must specify at least one backend role or make the model group public/private for registering it."
155+
);
154156
} else {
155157
input.setModelAccessMode(ModelAccessMode.RESTRICTED);
158+
modelAccessMode = ModelAccessMode.RESTRICTED;
156159
}
157160
}
158161
if ((ModelAccessMode.PUBLIC == modelAccessMode || ModelAccessMode.PRIVATE == modelAccessMode)
159162
&& (!CollectionUtils.isEmpty(input.getBackendRoles()) || Boolean.TRUE.equals(isAddAllBackendRoles))) {
160-
throw new IllegalArgumentException("User cannot specify backend roles to a public/private model group");
163+
throw new IllegalArgumentException("You can specify backend roles only for a model group with the restricted access mode.");
161164
} else if (ModelAccessMode.RESTRICTED == modelAccessMode) {
162165
if (modelAccessControlHelper.isAdmin(user) && Boolean.TRUE.equals(isAddAllBackendRoles)) {
163-
throw new IllegalArgumentException("Admin user cannot specify add all backend roles to a model group");
166+
throw new IllegalArgumentException("Admin users cannot add all backend roles to a model group.");
164167
}
165168
if (CollectionUtils.isEmpty(user.getBackendRoles())) {
166-
throw new IllegalArgumentException("Current user has no backend roles to specify the model group as restricted");
169+
throw new IllegalArgumentException("You must have at least one backend role to register a restricted model group.");
167170
}
168171
if (CollectionUtils.isEmpty(input.getBackendRoles()) && !Boolean.TRUE.equals(isAddAllBackendRoles)) {
169172
throw new IllegalArgumentException(
170-
"User have to specify backend roles or set add all backend roles to true for a restricted model group"
173+
"You must specify one or more backend roles or add all backend roles to register a restricted model group."
171174
);
172175
}
173176
if (!CollectionUtils.isEmpty(input.getBackendRoles()) && Boolean.TRUE.equals(isAddAllBackendRoles)) {
174-
throw new IllegalArgumentException("User cannot specify add all backed roles to true and backend roles not empty");
177+
throw new IllegalArgumentException("You cannot specify backend roles and add all backend roles at the same time.");
175178
}
176179
if (!modelAccessControlHelper.isAdmin(user)
177180
&& !Boolean.TRUE.equals(isAddAllBackendRoles)
178181
&& !new HashSet<>(user.getBackendRoles()).containsAll(input.getBackendRoles())) {
179-
throw new IllegalArgumentException("User cannot specify backend roles that doesn't belong to the current user");
182+
throw new IllegalArgumentException("You don't have the backend roles specified.");
180183
}
181184
}
182185
}
183186

184187
private void validateSecurityDisabledOrModelAccessControlDisabled(MLRegisterModelGroupInput input) {
185188
if (input.getModelAccessMode() != null || input.getIsAddAllBackendRoles() != null || input.getBackendRoles() != null) {
186189
throw new IllegalArgumentException(
187-
"Cluster security plugin not enabled or model access control no enabled, can't pass access control data in request body"
190+
"You cannot specify model access control parameters because the Security plugin or model access control is disabled on your cluster."
188191
);
189192
}
190193
}

‎plugin/src/main/java/org/opensearch/ml/action/model_group/TransportUpdateModelGroupAction.java

+22-15
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,9 @@ private void updateModelGroup(
124124
if (ModelAccessMode.RESTRICTED != updateModelGroupInput.getModelAccessMode()) {
125125
source.put(MLModelGroup.BACKEND_ROLES_FIELD, ImmutableList.of());
126126
}
127+
} else if (updateModelGroupInput.getBackendRoles() != null
128+
|| Boolean.TRUE.equals(updateModelGroupInput.getIsAddAllBackendRoles())) {
129+
source.put(MLModelGroup.ACCESS, ModelAccessMode.RESTRICTED.getValue());
127130
}
128131
if (updateModelGroupInput.getBackendRoles() != null) {
129132
source.put(MLModelGroup.BACKEND_ROLES_FIELD, updateModelGroupInput.getBackendRoles());
@@ -158,40 +161,48 @@ private void updateModelGroup(
158161
private void validateRequestForAccessControl(MLUpdateModelGroupInput input, User user, MLModelGroup mlModelGroup) {
159162
if (hasAccessControlChange(input)) {
160163
if (!modelAccessControlHelper.isOwner(mlModelGroup.getOwner(), user) && !modelAccessControlHelper.isAdmin(user)) {
161-
throw new IllegalArgumentException("Only owner/admin has valid privilege to perform update access control data");
164+
throw new IllegalArgumentException("Only owner or admin can update access control data.");
162165
} else if (modelAccessControlHelper.isOwner(mlModelGroup.getOwner(), user)
166+
&& !modelAccessControlHelper.isAdmin(user)
163167
&& !modelAccessControlHelper.isOwnerStillHasPermission(user, mlModelGroup)) {
164168
throw new IllegalArgumentException(
165-
"Owner doesn't have corresponding backend role to perform update access control data, please check with admin user"
169+
"You don’t have the specified backend role to update access control data. For more information, contact your administrator."
166170
);
167171
}
168172
}
169173
if (!modelAccessControlHelper.isAdmin(user)
170174
&& !modelAccessControlHelper.isOwner(mlModelGroup.getOwner(), user)
171175
&& !modelAccessControlHelper.isUserHasBackendRole(user, mlModelGroup)) {
172-
throw new IllegalArgumentException("User doesn't have corresponding backend role to perform update action");
176+
throw new IllegalArgumentException("You don't have permissions to perform this operation on this model group.");
173177
}
174178
ModelAccessMode modelAccessMode = input.getModelAccessMode();
175179
if ((ModelAccessMode.PUBLIC == modelAccessMode || ModelAccessMode.PRIVATE == modelAccessMode)
176180
&& (!CollectionUtils.isEmpty(input.getBackendRoles()) || Boolean.TRUE.equals(input.getIsAddAllBackendRoles()))) {
177-
throw new IllegalArgumentException("User cannot specify backend roles to a public/private model group");
181+
throw new IllegalArgumentException("You can specify backend roles only for a model group with the restricted access mode.");
178182
} else if (modelAccessMode == null || ModelAccessMode.RESTRICTED == modelAccessMode) {
179183
if (modelAccessControlHelper.isAdmin(user) && Boolean.TRUE.equals(input.getIsAddAllBackendRoles())) {
180-
throw new IllegalArgumentException("Admin user cannot specify add all backend roles to a model group");
184+
throw new IllegalArgumentException("Admin users cannot add all backend roles to a model group.");
181185
}
182186
if (Boolean.TRUE.equals(input.getIsAddAllBackendRoles()) && CollectionUtils.isEmpty(user.getBackendRoles())) {
183-
throw new IllegalArgumentException("Current user doesn't have any backend role");
187+
throw new IllegalArgumentException("You don’t have any backend roles.");
184188
}
185189
if (CollectionUtils.isEmpty(input.getBackendRoles()) && Boolean.FALSE.equals(input.getIsAddAllBackendRoles())) {
186-
throw new IllegalArgumentException("User have to specify backend roles when add all backend roles to false");
190+
throw new IllegalArgumentException("User have to specify backend roles when add all backend roles is set to false.");
187191
}
188192
if (!CollectionUtils.isEmpty(input.getBackendRoles()) && Boolean.TRUE.equals(input.getIsAddAllBackendRoles())) {
189-
throw new IllegalArgumentException("User cannot specify add all backed roles to true and backend roles not empty");
193+
throw new IllegalArgumentException("You cannot specify backend roles and add all backend roles at the same time.");
194+
}
195+
if (ModelAccessMode.RESTRICTED == modelAccessMode
196+
&& CollectionUtils.isEmpty(input.getBackendRoles())
197+
&& !Boolean.TRUE.equals(input.getIsAddAllBackendRoles())) {
198+
throw new IllegalArgumentException(
199+
"You must specify one or more backend roles or add all backend roles to register a restricted model group."
200+
);
190201
}
191202
if (!modelAccessControlHelper.isAdmin(user)
192-
&& inputBackendRolesAndModelBackendRolesBothNotEmpty(input, mlModelGroup)
203+
&& !CollectionUtils.isEmpty(input.getBackendRoles())
193204
&& !new HashSet<>(user.getBackendRoles()).containsAll(input.getBackendRoles())) {
194-
throw new IllegalArgumentException("User cannot specify backend roles that doesn't belong to the current user");
205+
throw new IllegalArgumentException("You don't have the backend roles specified.");
195206
}
196207
}
197208
}
@@ -200,14 +211,10 @@ private boolean hasAccessControlChange(MLUpdateModelGroupInput input) {
200211
return input.getModelAccessMode() != null || input.getIsAddAllBackendRoles() != null || input.getBackendRoles() != null;
201212
}
202213

203-
private boolean inputBackendRolesAndModelBackendRolesBothNotEmpty(MLUpdateModelGroupInput input, MLModelGroup mlModelGroup) {
204-
return !CollectionUtils.isEmpty(input.getBackendRoles()) && !CollectionUtils.isEmpty(mlModelGroup.getBackendRoles());
205-
}
206-
207214
private void validateSecurityDisabledOrModelAccessControlDisabled(MLUpdateModelGroupInput input) {
208215
if (input.getModelAccessMode() != null || input.getIsAddAllBackendRoles() != null || input.getBackendRoles() != null) {
209216
throw new IllegalArgumentException(
210-
"Cluster security plugin not enabled or model access control not enabled, can't pass access control data in request body"
217+
"You cannot specify model access control parameters because the Security plugin or model access control is disabled on your cluster."
211218
);
212219
}
213220
}

‎plugin/src/main/java/org/opensearch/ml/action/register/TransportRegisterModelAction.java

+2-5
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,8 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<MLRegi
121121
modelAccessControlHelper
122122
.validateModelGroupAccess(user, registerModelInput.getModelGroupId(), client, ActionListener.wrap(access -> {
123123
if (!access) {
124-
log.error("User doesn't have valid privilege to perform this operation on this model");
125-
listener
126-
.onFailure(
127-
new IllegalArgumentException("User doesn't have valid privilege to perform this operation on this model")
128-
);
124+
log.error("You don't have permissions to perform this operation on this model.");
125+
listener.onFailure(new IllegalArgumentException("You don't have permissions to perform this operation on this model."));
129126
} else {
130127
if (url != null) {
131128
boolean validUrl = pattern.matcher(url).find();

‎plugin/src/main/java/org/opensearch/ml/action/upload_chunk/MLModelChunkUploader.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,11 @@ public void uploadModelChunk(MLUploadModelChunkInput uploadModelChunkInput, Acti
8181
modelAccessControlHelper
8282
.validateModelGroupAccess(user, existingModel.getModelGroupId(), client, ActionListener.wrap(access -> {
8383
if (!access) {
84-
log.error("User doesn't have valid privilege to perform this operation on this model");
84+
log.error("You don't have permissions to perform this operation on this model.");
8585
listener
8686
.onFailure(
8787
new IllegalArgumentException(
88-
"User doesn't have valid privilege to perform this operation on this model"
88+
"You don't have permissions to perform this operation on this model."
8989
)
9090
);
9191
} else {

‎plugin/src/main/java/org/opensearch/ml/action/upload_chunk/TransportRegisterModelMetaAction.java

+2-3
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,8 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<MLRegi
6161

6262
modelAccessControlHelper.validateModelGroupAccess(user, mlUploadInput.getModelGroupId(), client, ActionListener.wrap(access -> {
6363
if (!access) {
64-
log.error("User doesn't have valid privilege to perform this operation on this model");
65-
listener
66-
.onFailure(new IllegalArgumentException("User doesn't have valid privilege to perform this operation on this model"));
64+
log.error("You don't have permissions to perform this operation on this model.");
65+
listener.onFailure(new IllegalArgumentException("You don't have permissions to perform this operation on this model."));
6766
} else {
6867
mlModelManager.registerModelMeta(mlUploadInput, ActionListener.wrap(modelId -> {
6968
listener.onResponse(new MLRegisterModelMetaResponse(modelId, MLTaskState.CREATED.name()));

‎plugin/src/main/java/org/opensearch/ml/helper/ModelAccessControlHelper.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,8 @@ public boolean isOwnerStillHasPermission(User user, MLModelGroup mlModelGroup) {
188188
if (CollectionUtils.isEmpty(mlModelGroup.getBackendRoles())) {
189189
throw new IllegalStateException("Backend roles should not be null");
190190
}
191-
return user.getBackendRoles() != null && new HashSet<>(mlModelGroup.getBackendRoles()).containsAll(user.getBackendRoles());
191+
return user.getBackendRoles() != null
192+
&& new HashSet<>(mlModelGroup.getBackendRoles()).stream().anyMatch(x -> user.getBackendRoles().contains(x));
192193
}
193194
throw new IllegalStateException("Access shouldn't be null");
194195
}

‎plugin/src/test/java/org/opensearch/ml/action/model_group/TransportRegisterModelGroupActionTests.java

+12-9
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ public void test_ExceptionAllAccessFieldsNull() {
138138
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
139139
verify(actionListener).onFailure(argumentCaptor.capture());
140140
assertEquals(
141-
"User must specify at least one backend role or make the model public/private",
141+
"You must specify at least one backend role or make the model group public/private for registering it.",
142142
argumentCaptor.getValue().getMessage()
143143
);
144144
}
@@ -160,7 +160,7 @@ public void test_BackendRolesProvidedWithPublic() {
160160
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
161161
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
162162
verify(actionListener).onFailure(argumentCaptor.capture());
163-
assertEquals("User cannot specify backend roles to a public/private model group", argumentCaptor.getValue().getMessage());
163+
assertEquals("You can specify backend roles only for a model group with the restricted access mode.", argumentCaptor.getValue().getMessage());
164164
}
165165

166166
public void test_BackendRolesProvidedWithPrivate() {
@@ -170,7 +170,7 @@ public void test_BackendRolesProvidedWithPrivate() {
170170
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
171171
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
172172
verify(actionListener).onFailure(argumentCaptor.capture());
173-
assertEquals("User cannot specify backend roles to a public/private model group", argumentCaptor.getValue().getMessage());
173+
assertEquals("You can specify backend roles only for a model group with the restricted access mode.", argumentCaptor.getValue().getMessage());
174174
}
175175

176176
public void test_AdminSpecifiedAddAllBackendRolesForRestricted() {
@@ -182,7 +182,7 @@ public void test_AdminSpecifiedAddAllBackendRolesForRestricted() {
182182
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
183183
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
184184
verify(actionListener).onFailure(argumentCaptor.capture());
185-
assertEquals("Admin user cannot specify add all backend roles to a model group", argumentCaptor.getValue().getMessage());
185+
assertEquals("Admin users cannot add all backend roles to a model group.", argumentCaptor.getValue().getMessage());
186186
}
187187

188188
public void test_UserWithNoBackendRolesSpecifiedRestricted() {
@@ -193,7 +193,10 @@ public void test_UserWithNoBackendRolesSpecifiedRestricted() {
193193
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
194194
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
195195
verify(actionListener).onFailure(argumentCaptor.capture());
196-
assertEquals("Current user has no backend roles to specify the model group as restricted", argumentCaptor.getValue().getMessage());
196+
assertEquals(
197+
"You must have at least one backend role to register a restricted model group.",
198+
argumentCaptor.getValue().getMessage()
199+
);
197200
}
198201

199202
public void test_UserSpecifiedRestrictedButNoBackendRolesFieldF() {
@@ -205,7 +208,7 @@ public void test_UserSpecifiedRestrictedButNoBackendRolesFieldF() {
205208
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
206209
verify(actionListener).onFailure(argumentCaptor.capture());
207210
assertEquals(
208-
"User have to specify backend roles or set add all backend roles to true for a restricted model group",
211+
"You must specify one or more backend roles or add all backend roles to register a restricted model group.",
209212
argumentCaptor.getValue().getMessage()
210213
);
211214
}
@@ -219,7 +222,7 @@ public void test_RestrictedAndUserSpecifiedBothBackendRolesField() {
219222
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
220223
verify(actionListener).onFailure(argumentCaptor.capture());
221224
assertEquals(
222-
"User cannot specify add all backed roles to true and backend roles not empty",
225+
"You cannot specify backend roles and add all backend roles at the same time.",
223226
argumentCaptor.getValue().getMessage()
224227
);
225228
}
@@ -234,7 +237,7 @@ public void test_RestrictedAndUserSpecifiedIncorrectBackendRoles() {
234237
transportRegisterModelGroupAction.doExecute(task, actionRequest, actionListener);
235238
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
236239
verify(actionListener).onFailure(argumentCaptor.capture());
237-
assertEquals("User cannot specify backend roles that doesn't belong to the current user", argumentCaptor.getValue().getMessage());
240+
assertEquals("You don't have the backend roles specified.", argumentCaptor.getValue().getMessage());
238241
}
239242

240243
public void test_SuccessSecurityDisabledCluster() {
@@ -254,7 +257,7 @@ public void test_ExceptionSecurityDisabledCluster() {
254257
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
255258
verify(actionListener).onFailure(argumentCaptor.capture());
256259
assertEquals(
257-
"Cluster security plugin not enabled or model access control no enabled, can't pass access control data in request body",
260+
"You cannot specify model access control parameters because the Security plugin or model access control is disabled on your cluster.",
258261
argumentCaptor.getValue().getMessage()
259262
);
260263
}

0 commit comments

Comments
 (0)