Skip to content

Commit e626ec0

Browse files
DonalEvansDonalEvans
authored
[ML] Fix and expand test coverage for model alias validation (elastic#136839)
The existing test for the validate() method was not using lowercase for the model alias, leading to the test not testing the behaviour it was intending to. - Expand coverage for PutTrainedModelAliasAction.validate() - Reword invalid model alias message slightly to be more accurate - Implement missing mutateInstance() method
1 parent 6a9f710 commit e626ec0

File tree

3 files changed

+116
-28
lines changed

3 files changed

+116
-28
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/PutTrainedModelAliasAction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ public class PutTrainedModelAliasAction extends ActionType<AcknowledgedResponse>
2929

3030
// NOTE this is similar to our valid ID check. The difference here is that model_aliases cannot end in numbers
3131
// This is to protect our automatic model naming conventions from hitting weird model_alias conflicts
32-
private static final Pattern VALID_MODEL_ALIAS_CHAR_PATTERN = Pattern.compile("[a-z0-9](?:[a-z0-9_\\-\\.]*[a-z])?");
32+
private static final Pattern VALID_MODEL_ALIAS_CHAR_PATTERN = Pattern.compile("[a-z0-9](?:[a-z0-9_\\-.]*[a-z])?");
3333

3434
public static final PutTrainedModelAliasAction INSTANCE = new PutTrainedModelAliasAction();
3535
public static final String NAME = "cluster:admin/xpack/ml/inference/model_aliases/put";

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ public final class Messages {
140140
public static final String INFERENCE_DEPLOYMENT_UPDATED_NUMBER_OF_ALLOCATIONS = "Updated number_of_allocations to [{0}]";
141141

142142
public static final String INVALID_MODEL_ALIAS = "Invalid model_alias; ''{0}'' can contain lowercase alphanumeric (a-z and 0-9), "
143-
+ "hyphens or underscores; must start with alphanumeric and cannot end with numbers";
143+
+ "hyphens or underscores; must start with alphanumeric and cannot end with numbers, hyphens or underscores";
144144
public static final String TRAINED_MODEL_INPUTS_DIFFER_SIGNIFICANTLY =
145145
"The input fields for new model [{0}] and for old model [{1}] differ significantly, model results may change drastically.";
146146

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/PutTrainedModelAliasActionRequestTests.java‎

Lines changed: 114 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,60 +10,148 @@
1010
import org.elasticsearch.common.io.stream.Writeable;
1111
import org.elasticsearch.test.AbstractWireSerializingTestCase;
1212
import org.elasticsearch.xpack.core.ml.action.PutTrainedModelAliasAction.Request;
13-
import org.junit.Before;
13+
14+
import java.util.HashSet;
15+
import java.util.List;
16+
import java.util.Locale;
17+
import java.util.Set;
1418

1519
import static org.hamcrest.Matchers.containsString;
20+
import static org.hamcrest.Matchers.hasSize;
1621
import static org.hamcrest.Matchers.not;
1722
import static org.hamcrest.Matchers.nullValue;
1823

1924
public class PutTrainedModelAliasActionRequestTests extends AbstractWireSerializingTestCase<Request> {
2025

21-
private String modelAlias;
22-
23-
@Before
24-
public void setupModelAlias() {
25-
modelAlias = randomAlphaOfLength(10);
26-
}
26+
private static final Set<String> INVALID_CHARACTERS = Set.of(
27+
"!",
28+
"@",
29+
"#",
30+
"$",
31+
"%",
32+
"^",
33+
"&",
34+
"*",
35+
"(",
36+
")",
37+
"+",
38+
"=",
39+
"[",
40+
"]",
41+
"{",
42+
"}",
43+
"|",
44+
";",
45+
":",
46+
"'",
47+
"\"",
48+
",",
49+
"<",
50+
">",
51+
"/",
52+
"?"
53+
);
2754

2855
@Override
2956
protected Request createTestInstance() {
30-
return new Request(modelAlias, randomAlphaOfLength(10), randomBoolean());
57+
return new Request(randomAlphaOfLength(10), randomAlphaOfLength(10), randomBoolean());
3158
}
3259

3360
@Override
3461
protected Request mutateInstance(Request instance) {
35-
return null;// TODO implement https://github.com/elastic/elasticsearch/issues/25929
62+
String modelAlias = instance.getModelAlias();
63+
String modelId = instance.getModelId();
64+
boolean reassign = instance.isReassign();
65+
int value = randomInt(2);
66+
return switch (value) {
67+
case 0 -> new Request(randomValueOtherThan(modelAlias, () -> randomAlphaOfLength(10)), modelId, reassign);
68+
case 1 -> new Request(modelAlias, randomValueOtherThan(modelId, () -> randomAlphaOfLength(10)), reassign);
69+
case 2 -> new Request(modelAlias, modelId, reassign == false);
70+
default -> throw new IllegalStateException("Unexpected value " + value);
71+
};
3672
}
3773

3874
@Override
3975
protected Writeable.Reader<Request> instanceReader() {
4076
return Request::new;
4177
}
4278

43-
public void testCtor() {
79+
public void testConstructor() {
4480
expectThrows(Exception.class, () -> new Request(null, randomAlphaOfLength(10), randomBoolean()));
4581
expectThrows(Exception.class, () -> new Request(randomAlphaOfLength(10), null, randomBoolean()));
4682
}
4783

4884
public void testValidate() {
49-
50-
{ // model_alias equal to model Id
51-
ActionRequestValidationException ex = new Request("foo", "foo", randomBoolean()).validate();
52-
assertThat(ex, not(nullValue()));
53-
assertThat(ex.getMessage(), containsString("model_alias [foo] cannot equal model_id [foo]"));
54-
}
55-
{ // model_alias cannot end in numbers
56-
modelAlias = randomAlphaOfLength(10) + randomIntBetween(0, Integer.MAX_VALUE);
85+
List<String> validAliases = List.of("a", "1", "2b", "c-3d", "e_4f", "g.5h");
86+
for (String modelAlias : validAliases) {
5787
ActionRequestValidationException ex = new Request(modelAlias, "foo", randomBoolean()).validate();
58-
assertThat(ex, not(nullValue()));
59-
assertThat(
60-
ex.getMessage(),
61-
containsString(
62-
"can contain lowercase alphanumeric (a-z and 0-9), hyphens or underscores; "
63-
+ "must start with alphanumeric and cannot end with numbers"
64-
)
65-
);
88+
assertThat("For alias [" + modelAlias + "]", ex, nullValue());
6689
}
6790
}
6891

92+
public void testValidate_modelAliasEqualToModelId() {
93+
ActionRequestValidationException ex = new Request("foo", "foo", randomBoolean()).validate();
94+
assertThat(ex, not(nullValue()));
95+
assertThat(ex.getMessage(), containsString("model_alias [foo] cannot equal model_id [foo]"));
96+
}
97+
98+
public void testValidate_modelAliasEqualToModelIdWithInvalidCharacter() {
99+
String modelAlias = "foo" + randomFrom(INVALID_CHARACTERS);
100+
ActionRequestValidationException ex = new Request(modelAlias, modelAlias, randomBoolean()).validate();
101+
assertThat("For alias [" + modelAlias + "]", ex, not(nullValue()));
102+
assertThat(ex.validationErrors(), hasSize(2));
103+
assertThat(ex.getMessage(), containsString("model_alias [" + modelAlias + "] cannot equal model_id [" + modelAlias + "]"));
104+
assertThat(
105+
ex.getMessage(),
106+
containsString(
107+
"can contain lowercase alphanumeric (a-z and 0-9), hyphens or underscores; "
108+
+ "must start with alphanumeric and cannot end with numbers"
109+
)
110+
);
111+
}
112+
113+
public void testValidate_modelAliasContainsUppercase() {
114+
List<String> invalidAliases = List.of("Start", "midDle", "enD");
115+
for (String invalidAlias : invalidAliases) {
116+
assertInvalidAlias(invalidAlias);
117+
}
118+
}
119+
120+
public void testValidate_modelAliasEndsWithNumber() {
121+
String modelAlias = (randomAlphaOfLength(10) + randomIntBetween(0, Integer.MAX_VALUE)).toLowerCase(Locale.ROOT);
122+
assertInvalidAlias(modelAlias);
123+
}
124+
125+
public void testValidate_modelAliasStartsWithInvalidCharacter() {
126+
Set<String> invalidFirstCharacters = new HashSet<>(INVALID_CHARACTERS);
127+
// '-', '_' and '.' are not valid as the first character
128+
invalidFirstCharacters.addAll(Set.of("-", "_", "."));
129+
String modelAlias = (randomFrom(invalidFirstCharacters) + randomAlphaOfLength(10)).toLowerCase(Locale.ROOT);
130+
assertInvalidAlias(modelAlias);
131+
}
132+
133+
public void testValidate_modelAliasContainsInvalidCharacter() {
134+
String modelAlias = (randomAlphaOfLength(5) + randomFrom(INVALID_CHARACTERS) + randomAlphaOfLength(5)).toLowerCase(Locale.ROOT);
135+
assertInvalidAlias(modelAlias);
136+
}
137+
138+
public void testValidate_modelAliasEndsWithInvalidCharacter() {
139+
Set<String> invalidLastCharacters = new HashSet<>(INVALID_CHARACTERS);
140+
// '-', '_' and '.' are not valid as the last character
141+
invalidLastCharacters.addAll(Set.of("-", "_", "."));
142+
String modelAlias = (randomAlphaOfLength(10) + randomFrom(invalidLastCharacters)).toLowerCase(Locale.ROOT);
143+
assertInvalidAlias(modelAlias);
144+
}
145+
146+
private static void assertInvalidAlias(String modelAlias) {
147+
ActionRequestValidationException ex = new Request(modelAlias, "foo", randomBoolean()).validate();
148+
assertThat("For alias [" + modelAlias + "]", ex, not(nullValue()));
149+
assertThat(
150+
ex.getMessage(),
151+
containsString(
152+
"can contain lowercase alphanumeric (a-z and 0-9), hyphens or underscores; "
153+
+ "must start with alphanumeric and cannot end with numbers"
154+
)
155+
);
156+
}
69157
}

0 commit comments

Comments
 (0)