[ML] Further validate calendar_id and add calendar description (elastic/x-pack-elasticsearch#3624)

relates elastic/x-pack-elasticsearch#3595

Original commit: elastic/x-pack-elasticsearch@fade977361
This commit is contained in:
Dimitris Athanasiou 2018-01-19 10:44:39 +00:00 committed by GitHub
parent 19874e35ee
commit 21f692c02b
14 changed files with 140 additions and 72 deletions

View File

@ -18,11 +18,13 @@ This API enables you to add jobs to a calendar.
`calendar_id` (required)::
(string) Identifier for the calendar.
//==== Request Body
`job_id` (required)::
(string) Identifier for the job.
//==== Request Body
`description`::
(string) A description of the calendar.
==== Authorization

View File

@ -39,6 +39,9 @@ public final class MlMetaIndex {
.startObject(Calendar.JOB_IDS.getPreferredName())
.field(ElasticsearchMappings.TYPE, ElasticsearchMappings.KEYWORD)
.endObject()
.startObject(Calendar.DESCRIPTION.getPreferredName())
.field(ElasticsearchMappings.TYPE, ElasticsearchMappings.KEYWORD)
.endObject()
.startObject(ScheduledEvent.START_TIME.getPreferredName())
.field(ElasticsearchMappings.TYPE, ElasticsearchMappings.DATE)
.endObject()

View File

@ -20,6 +20,7 @@ import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.xpack.ml.calendars.Calendar;
import org.elasticsearch.xpack.ml.job.messages.Messages;
import org.elasticsearch.xpack.ml.utils.ExceptionsHelper;
import org.elasticsearch.xpack.ml.utils.MlStrings;
import java.io.IOException;
import java.util.Objects;
@ -80,6 +81,16 @@ public class PutCalendarAction extends Action<PutCalendarAction.Request, PutCale
addValidationError("Cannot create a Calendar with the reserved name [_all]",
validationException);
}
if (!MlStrings.isValidId(calendar.getId())) {
validationException = addValidationError(Messages.getMessage(
Messages.INVALID_ID, Calendar.ID.getPreferredName(), calendar.getId()),
validationException);
}
if (!MlStrings.hasValidLengthForId(calendar.getId())) {
validationException = addValidationError(Messages.getMessage(
Messages.JOB_CONFIG_ID_TOO_LONG, MlStrings.ID_LENGTH_LIMIT),
validationException);
}
return validationException;
}

View File

@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.ml.calendars;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
@ -31,19 +32,20 @@ public class Calendar implements ToXContentObject, Writeable {
public static final ParseField TYPE = new ParseField("type");
public static final ParseField JOB_IDS = new ParseField("job_ids");
public static final ParseField ID = new ParseField("calendar_id");
public static final ParseField DESCRIPTION = new ParseField("description");
private static final String DOCUMENT_ID_PREFIX = "calendar_";
// For QueryPage
public static final ParseField RESULTS_FIELD = new ParseField("calendars");
public static final ObjectParser<Builder, Void> PARSER =
new ObjectParser<>(ID.getPreferredName(), Calendar.Builder::new);
public static final ObjectParser<Builder, Void> PARSER = new ObjectParser<>(ID.getPreferredName(), Builder::new);
static {
PARSER.declareString(Calendar.Builder::setId, ID);
PARSER.declareStringArray(Calendar.Builder::setJobIds, JOB_IDS);
PARSER.declareString(Builder::setId, ID);
PARSER.declareStringArray(Builder::setJobIds, JOB_IDS);
PARSER.declareString((builder, s) -> {}, TYPE);
PARSER.declareStringOrNull(Builder::setDescription, DESCRIPTION);
}
public static String documentId(String calendarId) {
@ -52,20 +54,24 @@ public class Calendar implements ToXContentObject, Writeable {
private final String id;
private final List<String> jobIds;
private final String description;
/**
* {@code jobIds} can be a mix of job groups and job Ids
* @param id The calendar Id
* @param jobIds List of job Ids or job groups.
* @param jobIds List of job Ids or job groups
* @param description An optional description
*/
public Calendar(String id, List<String> jobIds) {
public Calendar(String id, List<String> jobIds, @Nullable String description) {
this.id = Objects.requireNonNull(id, ID.getPreferredName() + " must not be null");
this.jobIds = Objects.requireNonNull(jobIds, JOB_IDS.getPreferredName() + " must not be null");
this.description = description;
}
public Calendar(StreamInput in) throws IOException {
id = in.readString();
jobIds = Arrays.asList(in.readStringArray());
description = in.readOptionalString();
}
public String getId() {
@ -80,10 +86,16 @@ public class Calendar implements ToXContentObject, Writeable {
return Collections.unmodifiableList(jobIds);
}
@Nullable
public String getDescription() {
return description;
}
@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeString(id);
out.writeStringArray(jobIds.toArray(new String[jobIds.size()]));
out.writeOptionalString(description);
}
@Override
@ -91,6 +103,9 @@ public class Calendar implements ToXContentObject, Writeable {
builder.startObject();
builder.field(ID.getPreferredName(), id);
builder.field(JOB_IDS.getPreferredName(), jobIds);
if (description != null) {
builder.field(DESCRIPTION.getPreferredName(), description);
}
if (params.paramAsBoolean(MlMetaIndex.INCLUDE_TYPE_KEY, false)) {
builder.field(TYPE.getPreferredName(), CALENDAR_TYPE);
}
@ -109,12 +124,12 @@ public class Calendar implements ToXContentObject, Writeable {
}
Calendar other = (Calendar) obj;
return id.equals(other.id) && jobIds.equals(other.jobIds);
return id.equals(other.id) && jobIds.equals(other.jobIds) && Objects.equals(description, other.description);
}
@Override
public int hashCode() {
return Objects.hash(id, jobIds);
return Objects.hash(id, jobIds, description);
}
public static Builder builder() {
@ -125,9 +140,10 @@ public class Calendar implements ToXContentObject, Writeable {
private String calendarId;
private List<String> jobIds = Collections.emptyList();
private String description;
public String getId() {
return this.calendarId;
return calendarId;
}
public void setId(String calendarId) {
@ -139,8 +155,13 @@ public class Calendar implements ToXContentObject, Writeable {
return this;
}
public Builder setDescription(String description) {
this.description = description;
return this;
}
public Calendar build() {
return new Calendar(calendarId, jobIds);
return new Calendar(calendarId, jobIds, description);
}
}
}

View File

@ -90,7 +90,6 @@ public class Job extends AbstractDiffable<Job> implements Writeable, ToXContentO
public static final ObjectParser<Builder, Void> CONFIG_PARSER = new ObjectParser<>("job_details", false, Builder::new);
public static final Map<MlParserType, ObjectParser<Builder, Void>> PARSERS = new EnumMap<>(MlParserType.class);
public static final int MAX_JOB_ID_LENGTH = 64;
public static final TimeValue MIN_BACKGROUND_PERSIST_INTERVAL = TimeValue.timeValueHours(1);
public static final ByteSizeValue PROCESS_MEMORY_OVERHEAD = new ByteSizeValue(100, ByteSizeUnit.MB);
@ -1050,8 +1049,8 @@ public class Job extends AbstractDiffable<Job> implements Writeable, ToXContentO
if (!MlStrings.isValidId(id)) {
throw new IllegalArgumentException(Messages.getMessage(Messages.INVALID_ID, ID.getPreferredName(), id));
}
if (id.length() > MAX_JOB_ID_LENGTH) {
throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_ID_TOO_LONG, MAX_JOB_ID_LENGTH));
if (!MlStrings.hasValidLengthForId(id)) {
throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_ID_TOO_LONG, MlStrings.ID_LENGTH_LIMIT));
}
validateGroups();

View File

@ -1089,7 +1089,7 @@ public class JobProvider {
Set<String> currentJobs = new HashSet<>(calendar.getJobIds());
currentJobs.addAll(jobIdsToAdd);
currentJobs.removeAll(jobIdsToRemove);
Calendar updatedCalendar = new Calendar(calendar.getId(), new ArrayList<>(currentJobs));
Calendar updatedCalendar = new Calendar(calendar.getId(), new ArrayList<>(currentJobs), calendar.getDescription());
UpdateRequest updateRequest = new UpdateRequest(MlMetaIndex.INDEX_NAME, MlMetaIndex.TYPE, updatedCalendar.documentId());
updateRequest.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE);
@ -1156,7 +1156,7 @@ public class JobProvider {
.map(c -> {
Set<String> ids = new HashSet<>(c.getJobIds());
ids.remove(jobId);
return new Calendar(c.getId(), new ArrayList<>(ids));
return new Calendar(c.getId(), new ArrayList<>(ids), c.getDescription());
}).forEach(c -> {
UpdateRequest updateRequest = new UpdateRequest(MlMetaIndex.INDEX_NAME, MlMetaIndex.TYPE,
c.documentId());

View File

@ -25,6 +25,8 @@ public final class MlStrings {
*/
private static final Pattern VALID_ID_CHAR_PATTERN = Pattern.compile("[a-z0-9](?:[a-z0-9_\\-\\.]*[a-z0-9])?");
public static final int ID_LENGTH_LIMIT = 64;
private MlStrings() {
}
@ -61,6 +63,19 @@ public final class MlStrings {
return id != null && VALID_ID_CHAR_PATTERN.matcher(id).matches() && !MetaData.ALL.equals(id);
}
/**
* Checks if the given {@code id} has a valid length.
* We keep IDs in a length < {@link #ID_LENGTH_LIMIT}
* in order to avoid unfriendly errors when storing docs with
* more than 512 bytes.
*
* @param id the id
* @return {@code true} if the id has a valid length
*/
public static boolean hasValidLengthForId(String id) {
return id.length() <= ID_LENGTH_LIMIT;
}
/**
* Returns the path to the parent field if {@code fieldPath} is nested
* or {@code fieldPath} itself.

View File

@ -42,7 +42,7 @@ public class RestPutCalendarAction extends BaseRestHandler {
XContentParser parser = restRequest.contentOrSourceParamParser();
putCalendarRequest = PutCalendarAction.Request.parseRequest(calendarId, parser);
} else {
putCalendarRequest = new PutCalendarAction.Request(new Calendar(calendarId, Collections.emptyList()));
putCalendarRequest = new PutCalendarAction.Request(new Calendar(calendarId, Collections.emptyList(), null));
}
return channel -> client.execute(PutCalendarAction.INSTANCE, putCalendarRequest, new RestToXContentListener<>(channel));

View File

@ -7,24 +7,16 @@ package org.elasticsearch.xpack.ml.action;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.test.AbstractStreamableXContentTestCase;
import org.elasticsearch.xpack.ml.calendars.Calendar;
import java.util.ArrayList;
import java.util.List;
import org.elasticsearch.xpack.ml.calendars.CalendarTests;
import org.elasticsearch.xpack.ml.job.config.JobTests;
public class PutCalendarActionRequestTests extends AbstractStreamableXContentTestCase<PutCalendarAction.Request> {
private final String calendarId = randomAlphaOfLengthBetween(1, 20);
private final String calendarId = JobTests.randomValidJobId();
@Override
protected PutCalendarAction.Request createTestInstance() {
int size = randomInt(10);
List<String> jobIds = new ArrayList<>(size);
for (int i = 0; i < size; i++) {
jobIds.add(randomAlphaOfLengthBetween(1, 20));
}
Calendar calendar = new Calendar(calendarId, jobIds);
return new PutCalendarAction.Request(calendar);
return new PutCalendarAction.Request(CalendarTests.testInstance(calendarId));
}
@Override

View File

@ -8,6 +8,7 @@ package org.elasticsearch.xpack.ml.calendars;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.test.AbstractSerializingTestCase;
import org.elasticsearch.xpack.ml.job.config.JobTests;
import java.io.IOException;
import java.util.ArrayList;
@ -19,12 +20,20 @@ import static org.hamcrest.Matchers.equalTo;
public class CalendarTests extends AbstractSerializingTestCase<Calendar> {
public static Calendar testInstance() {
return testInstance(JobTests.randomValidJobId());
}
public static Calendar testInstance(String calendarId) {
int size = randomInt(10);
List<String> items = new ArrayList<>(size);
for (int i = 0; i < size; i++) {
items.add(randomAlphaOfLengthBetween(1, 20));
}
return new Calendar(randomAlphaOfLengthBetween(1, 20), items);
String description = null;
if (randomBoolean()) {
description = randomAlphaOfLength(20);
}
return new Calendar(calendarId, items, description);
}
@Override
@ -43,7 +52,7 @@ public class CalendarTests extends AbstractSerializingTestCase<Calendar> {
}
public void testNullId() {
NullPointerException ex = expectThrows(NullPointerException.class, () -> new Calendar(null, Collections.emptyList()));
NullPointerException ex = expectThrows(NullPointerException.class, () -> new Calendar(null, Collections.emptyList(), null));
assertEquals(Calendar.ID.getPreferredName() + " must not be null", ex.getMessage());
}

View File

@ -113,11 +113,11 @@ public class JobProviderIT extends XPackSingleNodeTestCase {
public void testGetCalandarByJobId() throws Exception {
List<Calendar> calendars = new ArrayList<>();
calendars.add(new Calendar("empty calendar", Collections.emptyList()));
calendars.add(new Calendar("foo calendar", Collections.singletonList("foo")));
calendars.add(new Calendar("foo bar calendar", Arrays.asList("foo", "bar")));
calendars.add(new Calendar("cat calendar", Collections.singletonList("cat")));
calendars.add(new Calendar("cat foo calendar", Arrays.asList("cat", "foo")));
calendars.add(new Calendar("empty calendar", Collections.emptyList(), null));
calendars.add(new Calendar("foo calendar", Collections.singletonList("foo"), null));
calendars.add(new Calendar("foo bar calendar", Arrays.asList("foo", "bar"), null));
calendars.add(new Calendar("cat calendar", Collections.singletonList("cat"), null));
calendars.add(new Calendar("cat foo calendar", Arrays.asList("cat", "foo"), null));
indexCalendars(calendars);
List<Calendar> queryResult = getCalendars("ted");
@ -137,7 +137,7 @@ public class JobProviderIT extends XPackSingleNodeTestCase {
public void testUpdateCalendar() throws Exception {
String calendarId = "empty calendar";
Calendar emptyCal = new Calendar(calendarId, Collections.emptyList());
Calendar emptyCal = new Calendar(calendarId, Collections.emptyList(), null);
indexCalendars(Collections.singletonList(emptyCal));
Set<String> addedIds = new HashSet<>();
@ -161,11 +161,11 @@ public class JobProviderIT extends XPackSingleNodeTestCase {
public void testRemoveJobFromCalendar() throws Exception {
List<Calendar> calendars = new ArrayList<>();
calendars.add(new Calendar("empty calendar", Collections.emptyList()));
calendars.add(new Calendar("foo calendar", Collections.singletonList("foo")));
calendars.add(new Calendar("foo bar calendar", Arrays.asList("foo", "bar")));
calendars.add(new Calendar("cat calendar", Collections.singletonList("cat")));
calendars.add(new Calendar("cat foo calendar", Arrays.asList("cat", "foo")));
calendars.add(new Calendar("empty calendar", Collections.emptyList(), null));
calendars.add(new Calendar("foo calendar", Collections.singletonList("foo"), null));
calendars.add(new Calendar("foo bar calendar", Arrays.asList("foo", "bar"), null));
calendars.add(new Calendar("cat calendar", Collections.singletonList("cat"), null));
calendars.add(new Calendar("cat foo calendar", Arrays.asList("cat", "foo"), null));
indexCalendars(calendars);
CountDownLatch latch = new CountDownLatch(1);
@ -289,7 +289,7 @@ public class JobProviderIT extends XPackSingleNodeTestCase {
String calendarAId = "maintenance_a";
List<Calendar> calendars = new ArrayList<>();
calendars.add(new Calendar(calendarAId, Collections.singletonList("job_a")));
calendars.add(new Calendar(calendarAId, Collections.singletonList("job_a"), null));
ZonedDateTime now = ZonedDateTime.now();
List<ScheduledEvent> events = new ArrayList<>();
@ -298,7 +298,7 @@ public class JobProviderIT extends XPackSingleNodeTestCase {
events.add(buildScheduledEvent("downtime_AAA", now.plusDays(15), now.plusDays(16), calendarAId));
String calendarABId = "maintenance_a_and_b";
calendars.add(new Calendar(calendarABId, Arrays.asList("job_a", "job_b")));
calendars.add(new Calendar(calendarABId, Arrays.asList("job_a", "job_b"), null));
events.add(buildScheduledEvent("downtime_AB", now.plusDays(12), now.plusDays(13), calendarABId));
@ -339,14 +339,14 @@ public class JobProviderIT extends XPackSingleNodeTestCase {
String calendarAId = "calendar_a";
List<Calendar> calendars = new ArrayList<>();
calendars.add(new Calendar(calendarAId, Collections.singletonList(groupA)));
calendars.add(new Calendar(calendarAId, Collections.singletonList(groupA), null));
ZonedDateTime now = ZonedDateTime.now();
List<ScheduledEvent> events = new ArrayList<>();
events.add(buildScheduledEvent("downtime_A", now.plusDays(1), now.plusDays(2), calendarAId));
String calendarBId = "calendar_b";
calendars.add(new Calendar(calendarBId, Arrays.asList(groupB)));
calendars.add(new Calendar(calendarBId, Arrays.asList(groupB), null));
events.add(buildScheduledEvent("downtime_B", now.plusDays(12), now.plusDays(13), calendarBId));
indexCalendars(calendars);
@ -372,7 +372,7 @@ public class JobProviderIT extends XPackSingleNodeTestCase {
Job.Builder job = createJob(jobId, Arrays.asList("fruit", "tea"));
String calendarId = "downtime";
Calendar calendar = new Calendar(calendarId, Collections.singletonList(jobId));
Calendar calendar = new Calendar(calendarId, Collections.singletonList(jobId), null);
indexCalendars(Collections.singletonList(calendar));
// index the param docs

View File

@ -40,4 +40,9 @@ public class MlStringsTests extends ESTestCase {
assertThat(MlStrings.getParentField("foo.bar"), equalTo("foo"));
assertThat(MlStrings.getParentField("x.y.z"), equalTo("x.y"));
}
public void testHasValidLengthForId() {
assertThat(MlStrings.hasValidLengthForId(randomAlphaOfLength(64)), is(true));
assertThat(MlStrings.hasValidLengthForId(randomAlphaOfLength(65)), is(false));
}
}

View File

@ -33,11 +33,13 @@
calendar_id: "advent"
body: >
{
"job_ids": ["cal-job", "cal-job2"]
"job_ids": ["cal-job", "cal-job2"],
"description": "This is a calendar about..."
}
- match: { calendar_id: advent }
- match: { job_ids.0: cal-job }
- match: { job_ids.1: cal-job2 }
- match: { description: "This is a calendar about..."}
- do:
xpack.ml.get_calendars:
@ -47,11 +49,12 @@
calendars.0:
calendar_id: "advent"
job_ids: ["cal-job", "cal-job2"]
description: "This is a calendar about..."
- is_false: type
- do:
xpack.ml.put_calendar:
calendar_id: "Dogs of the Year"
calendar_id: "dogs_of_the_year"
body: >
{
"job_ids": ["cal-job"]
@ -59,7 +62,7 @@
- do:
xpack.ml.put_calendar:
calendar_id: "Cats of the Year"
calendar_id: "cats_of_the_year"
- do:
xpack.ml.get_calendars: {}
@ -67,7 +70,7 @@
- do:
xpack.ml.delete_calendar:
calendar_id: "Dogs of the Year"
calendar_id: "dogs_of_the_year"
- do:
xpack.ml.get_calendars: {}
@ -81,12 +84,12 @@
- do:
catch: missing
xpack.ml.get_calendars:
calendar_id: "Dogs of the Year"
calendar_id: "dogs_of_the_year"
- do:
catch: missing
xpack.ml.put_calendar:
calendar_id: "new cal with unknown job"
calendar_id: "new_cal_with_unknown_job"
body: >
{
"job_ids": ["cal-job", "unknown-job"]
@ -99,24 +102,31 @@
xpack.ml.get_calendars:
calendar_id: "unknown"
---
"Test put calendar given id contains invalid chars":
- do:
catch: bad_request
xpack.ml.put_calendar:
calendar_id: "Mayas"
---
"Test PageParams":
- do:
xpack.ml.put_calendar:
calendar_id: "Calendar1"
calendar_id: "calendar1"
- do:
xpack.ml.put_calendar:
calendar_id: "Calendar2"
calendar_id: "calendar2"
- do:
xpack.ml.put_calendar:
calendar_id: "Calendar3"
calendar_id: "calendar3"
- do:
xpack.ml.get_calendars:
from: 2
- match: { count: 3 }
- length: { calendars: 1}
- match: { calendars.0.calendar_id: Calendar3 }
- match: { calendars.0.calendar_id: calendar3 }
- do:
xpack.ml.get_calendars:
@ -124,14 +134,14 @@
size: 1
- match: { count: 3 }
- length: { calendars: 1}
- match: { calendars.0.calendar_id: Calendar2 }
- match: { calendars.0.calendar_id: calendar2 }
---
"Test PageParams with ID is invalid":
- do:
catch: bad_request
xpack.ml.get_calendars:
calendar_id: Tides
calendar_id: tides
size: 10
---
@ -139,12 +149,12 @@
- do:
xpack.ml.put_calendar:
calendar_id: "Mayan"
calendar_id: "mayan"
- do:
catch: /version_conflict_engine_exception/
xpack.ml.put_calendar:
calendar_id: "Mayan"
calendar_id: "mayan"
---
"Test cannot create calendar with name _all":
@ -193,7 +203,7 @@
- do:
xpack.ml.put_calendar:
calendar_id: "Wildlife"
calendar_id: "wildlife"
- do:
xpack.ml.put_job:
@ -210,41 +220,41 @@
- do:
xpack.ml.put_calendar_job:
calendar_id: "Wildlife"
calendar_id: "wildlife"
job_id: "tiger"
- match: { calendar_id: "Wildlife" }
- match: { calendar_id: "wildlife" }
- match: { job_ids.0: "tiger" }
- do:
xpack.ml.get_calendars:
calendar_id: "Wildlife"
calendar_id: "wildlife"
- match: { count: 1 }
- match: { calendars.0.calendar_id: "Wildlife" }
- match: { calendars.0.calendar_id: "wildlife" }
- length: { calendars.0.job_ids: 1 }
- match: { calendars.0.job_ids.0: "tiger" }
- do:
xpack.ml.delete_calendar_job:
calendar_id: "Wildlife"
calendar_id: "wildlife"
job_id: "tiger"
- do:
xpack.ml.get_calendars:
calendar_id: "Wildlife"
calendar_id: "wildlife"
- match: { count: 1 }
- match: { calendars.0.calendar_id: "Wildlife" }
- match: { calendars.0.calendar_id: "wildlife" }
- length: { calendars.0.job_ids: 0 }
- do:
catch: missing
xpack.ml.put_calendar_job:
calendar_id: "Wildlife"
calendar_id: "wildlife"
job_id: "missing job"
- do:
catch: missing
xpack.ml.delete_calendar_job:
calendar_id: "Wildlife"
calendar_id: "wildlife"
job_id: "missing job"
---

View File

@ -24,6 +24,7 @@ integTestRunner {
'ml/calendar_crud/Test cannot create calendar with name _all',
'ml/calendar_crud/Test PageParams with ID is invalid',
'ml/calendar_crud/Test post calendar events given empty events',
'ml/calendar_crud/Test put calendar given id contains invalid chars',
'ml/custom_all_field/Test querying custom all field',
'ml/datafeeds_crud/Test delete datafeed with missing id',
'ml/datafeeds_crud/Test put datafeed referring to missing job_id',