Add External Event Visibility to Procedural Constraints by pranav-super · Pull Request #1789 · NASA-AMMOS/plandev

pranav-super · 2026-02-25T01:38:07Z

Tickets addressed: --
Review: By commit
Merge strategy: Merge (no squash)

Description

This pull request introduces visibility of external events in procedural constraints. Conceptually, the approach is extremely similar to that taken when introducing external events to procedural scheduling, and the implementation largely piggybacks off of that implementation.

That being said, this PR introduces external events to PlanRepository, PlanService, and other related entities like EvaluationEnvironment that previously lacked visibility to external events. These are the analogues in procedural constraints to MerlinDatabaseService (and other smaller items like SchedulerToProcedurePlanAdapter and SynchronousSchedulerAgent). But common entities are still shared, like the Kotlin timeline representations of External Events and Sources.

Verification

A few new end-to-end tests were introduced in ExternalEventConstraintTests under e2e-tests, to verify that several constraints leveraging External Events worked as expected.

Documentation

No documentation was invalidated, though examples can/should now be added to the "Examples" page under the Procedural Constraints section of the documentation. As a matter of fact, that page already claims/suggests that external events can be leveraged by the constraints engine!

Future work

None anticipated.

(Small note: significant issues with the build process arose when trying to organize the procedures in src/main/java under e2e-tests into separate constraints and scheduling folders. This organization would be useful but has worked inconsistently when building.)

Called by TypeUtilsPlanAdapter, which is used in procedural evaluations

TypeUtilsEditablePlanAdapter rightfully has an implementation for the events() method, while TypeUtilsPlanAdapter does not.

Done to reflect that we also test procedural constraints.

…Setup Done to reflect that we also test procedural constraints.

NOTE TO REVIEWER - was having difficulties when trying to use a build/libs/scheduling/....jar path owing to a weird gradle issue. This is why these tests have been updated to just use build/libs/....jar, but this isn't a complete fix. Ideally, the constraints can go in a constraint subfolder and goals in a scheduling folder (i.e. build/libs/scheduling and a build/libs/constraints directory).

…and TestApplyWhen

Mythicaeda

Marking this as "Request Changes" because of the GetExternalEventAction code.

With regards to the tests, I noticed that you only tested that the eventType query param of the plan.events method works, not derivationGroup or source. Tests that prove that these other two filters work, and that they work in combination, would be valuable.

Additionally, I'm requesting a test added where a constraint that runs plan.events is checked against a plan that has no external events.

Mythicaeda · 2026-04-20T13:42:17Z

+        ee.source_key,
+        ee.derivation_group_name,
+        ee.start_time,
+        extract(epoch from ee.duration) as duration,


Proposal:

Suggested change

extract(epoch from ee.duration) as duration,

extract(epoch from ee.duration)*1e6 as duration_micros,

This would allow you to just do Duration.of(resultSet.getLong("duration_micros"), Duration.MICROSECOND); on L68, rather than casting Double to a Long

Mythicaeda · 2026-04-20T13:58:21Z

+      // get event attributes
+      final var eventAttributes = new SerializedValueJsonParser().parse(Json
+          .createReader(
+              new StringReader(Objects.requireNonNull(resultSet.getObject("attributes", PGobject.class).getValue()))
+          )
+          .readObject())
+          .getSuccessOrThrow(reason -> new InvalidEntityException(List.of(reason)))
+          .asMap()
+          .get();
+
+      // get source attributes
+      // NOTE: in the query, we get the source attributes with each event. This is pretty repetitive, which I don't
+      //        quite like, especially as there could be a _lot_ of events. A fix to this could entail a second query,
+      //        but I am unsure if the design would appreciate multiple queries, or perhaps an Action referencing
+      //        another Action (i.e. we have a GetExternalSourceAttributesAction invoked by this action).
+      final var sourceAttributes = new SerializedValueJsonParser().parse(Json
+          .createReader(
+              new StringReader(Objects.requireNonNull(resultSet.getObject("source_attributes", PGobject.class).getValue()))
+          )
+          .readObject())
+         .getSuccessOrThrow(reason -> new InvalidEntityException(List.of(reason)))
+         .asMap()
+         .get();


Heads up: this is not standard practice for how we get JsonObject fields from the DB in Aerie. Instead we use the helper method PostgresParsers.getJsonColumn.

To fix this code, you need to perform 2 steps:

Step 1: Create a Parser for "Event Attributes"

Add the following line to gov.nasa.jpl.aerie.merlin.server.remotes.postgres.PostgresParsers (I specifically recommend adding it around L120):

public static final JsonParser<Map<String, SerializedValue>> eventAttributesP = mapP(serializedValueP);

^I'm assuming the event attributes are a Map of SerializedValues based on the use of SerializedValueJsonParser.

Step 2: Use PostgresParsers.getJsonColumn

These massive statements then simplify into

// get event attributes final var eventAttributes = getJsonColumn(resultSet, "attributes", eventAttributesP) .getSuccessOrThrow(reason -> new InvalidEntityException(List.of(reason))); // get source attributes // (long comment skipped for brevity) final var sourceAttributes = getJsonColumn(resultSet, "source_attributes", eventAttributesP) .getSuccessOrThrow(reason -> new InvalidEntityException(List.of(reason)));

Mythicaeda · 2026-04-20T14:11:37Z

+      // NOTE: in the query, we get the source attributes with each event. This is pretty repetitive, which I don't
+      //        quite like, especially as there could be a _lot_ of events. A fix to this could entail a second query,
+      //        but I am unsure if the design would appreciate multiple queries, or perhaps an Action referencing
+      //        another Action (i.e. we have a GetExternalSourceAttributesAction invoked by this action).


WRT this comment, something that bothers me is that you're creating a new External Source object for every single External Event, even if those events share a Source. (It's definitely a new instance, as ExternalSource.kt does not use the @JvmRecord annotation that would make it a record)

To avoid this, what you should do is have this method accept a Map<Key, ExternalSource> sources as a parameter and then use the source_key column from the resultSet to look up the source.

You would then add a GetExternalSourcesMap Action (and corresponding get method in the ProfileRepository class). Finally, you should update ProfileRepository.getExternalEvents (the caller of this method) to call GetExternalSourcesMap and feed the resulting map into this method.

Mythicaeda · 2026-04-20T14:16:03Z

    }
  }

+  static Map<String, List<ExternalEvent>> getExternalEvents(


Can you please explain why this method is in ProfileRepository rather than PostgresPlanRepository?

Mythicaeda · 2026-04-20T14:21:40Z

+    } catch (final SQLException ex) {
+      throw new DatabaseException(
+          "Failed to get external events for plan with id `%s`".formatted(planId), ex);
+    } catch (final InvalidEntityException in) {
+      throw new RuntimeException(
+          ("Failed to get external events for plan with id `%s; "
+           + "failed to parse jsonb for external event/source attributes`").formatted(planId), in);
+    }


Something I've been looking into improving is reducing the number of "Silent Runtime Exceptions" thrown by our code. If this method were to properly throw its SQLException and InvalidEntityException rather than cast them to RuntimeException, would that effect user code or just the Constraint Action?

(Please note that I'm not asking for a change here, just trying to gauge the impact for some future work.)

Mythicaeda · 2026-04-20T15:03:24Z

+import org.jetbrains.annotations.NotNull;
+
+@ConstraintProcedure
+public record ExternalEventPresenceConstraint() implements Constraint {


This Constraint should be written as inverted. That is, it returns a Violation if it detects an external event. This way, it is obvious when reading the test verifyPresenceOfExternalEvent that the Constraint saw an External Event of type TestType.

You could additionally make this a paramaterized constraint and also run it with an eventType argument that isnt present (ie fakeType) to show that plan.events is successfully filtering out undesired types

Mythicaeda · 2026-04-20T15:12:35Z

+    // check that there are violations in two places (where the existing events are)
+    assertEquals(1, resp.constraintsRun().size());
+    assertEquals(2, resp.constraintsRun().getFirst().result().get().violations().size());


Asserting that there are two violations is necessary but not sufficient to prove that those violations are "(where the existing events are)". You need additional assertions confirming that these are window violations and not activityDirective violations, and that the windows correspond to the expected intervals.

Mythicaeda · 2026-04-20T15:14:33Z

+import org.jetbrains.annotations.NotNull;
+
+@ConstraintProcedure
+public record ExternalEventAttributeQuantityConstraint(int quantity) implements Constraint {


Suggestion: A more meaningful parameter for the purpose of testing would be attribute rather than quantity, with the constraint returning a violation for each external event with that attribute. After all, this constraint is meant to be testing that constraints have access to the attributes of external events.

Mythicaeda · 2026-04-20T15:20:21Z

+    // get length of violation (expect 30 minutes)
+    var violation = violations.get(0).windows().get(0);
+    assertEquals(30, (violation.end() - violation.start())/(60*1000000));


You need additional assertions to prove that the violation window actually aligns with where the activity and external event overlap

Mythicaeda · 2026-04-20T15:22:39Z

+  // ensure external event (with given attributes) overlaps with activity for total of M seconds
+  @Test
+  void testExternalEventActivityOverlapForSeconds() throws IOException {
+    // upload the constraint


What is this testing with regards to the visibility of External Events in Constraints that was not covered by testExternalEventActivityOverlap?

pranav-super requested a review from a team as a code owner February 25, 2026 01:38

pranav-super requested review from JoelCourtney and cartermak February 25, 2026 01:38

pranav-super had a problem deploying to e2e-test February 25, 2026 01:38 — with GitHub Actions Failure

pranav-super temporarily deployed to e2e-test March 17, 2026 14:59 — with GitHub Actions Inactive

pranav-super force-pushed the exploration/procedural-constraints-external-events branch from e29f084 to 4f8d292 Compare March 17, 2026 15:00

pranav-super temporarily deployed to e2e-test March 17, 2026 15:00 — with GitHub Actions Inactive

pranav-super temporarily deployed to e2e-test March 17, 2026 15:33 — with GitHub Actions Inactive

pranav-super temporarily deployed to e2e-test March 17, 2026 15:51 — with GitHub Actions Inactive

dandelany force-pushed the exploration/procedural-constraints-external-events branch from b180fa9 to 87edfeb Compare March 23, 2026 18:28

dandelany temporarily deployed to e2e-test March 23, 2026 18:28 — with GitHub Actions Inactive

psubram3 added 18 commits April 6, 2026 10:19

add GetPlanExternalEventsAction

8a63cd8

add getExternalEvents to ProfileRepository

9ff41e8

add getExternalEvents to PlanRepository

b39befc

add getExternalEvents to PlanService

93c1076

add external events to EvaluationEnvironment

dc80458

update goals with respect to changes in EvaluationEnvironment

1546168

update ReadOnlyPlan's external event access

2a711d2

Called by TypeUtilsPlanAdapter, which is used in procedural evaluations

update ConstraintAction to add external events to environment

ab99b92

correct TODOs in TypeUtilsPlanAdapter and TypeUtilsEditablePlanAdapter

f1905b0

TypeUtilsEditablePlanAdapter rightfully has an implementation for the events() method, while TypeUtilsPlanAdapter does not.

update error in PostgresPlanRepository

2c17ac2

add new test constraints

21fe51e

rename ProceduralSchedulingSetup.java to ProceduralTestingSetup

50022ef

Done to reflect that we also test procedural constraints.

reflect update of ProceduralSchedulingSetup.java to ProceduralTesting…

d02a709

…Setup Done to reflect that we also test procedural constraints.

extract common external event test functionality

3eb67a2

add ExternalEventConstraintTests

1cee278

update EvaluationEnvironment constructor in ASTTests.java

54a4051

update EvaluationEnvironment constructor in InstantiateArgumentsTest …

272163d

…and TestApplyWhen

dandelany force-pushed the exploration/procedural-constraints-external-events branch from 87edfeb to 272163d Compare April 6, 2026 17:19

dandelany temporarily deployed to e2e-test April 6, 2026 17:19 — with GitHub Actions Inactive

Mythicaeda requested changes Apr 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add External Event Visibility to Procedural Constraints#1789

Add External Event Visibility to Procedural Constraints#1789
pranav-super wants to merge 18 commits intodevelopfrom
exploration/procedural-constraints-external-events

pranav-super commented Feb 25, 2026

Uh oh!

Mythicaeda left a comment

Uh oh!

Mythicaeda Apr 20, 2026

Uh oh!

Mythicaeda Apr 20, 2026 •

edited

Loading

Uh oh!

Mythicaeda Apr 20, 2026

Uh oh!

Mythicaeda Apr 20, 2026

Uh oh!

Mythicaeda Apr 20, 2026

Uh oh!

Mythicaeda Apr 20, 2026

Uh oh!

Mythicaeda Apr 20, 2026

Uh oh!

Mythicaeda Apr 20, 2026

Uh oh!

Mythicaeda Apr 20, 2026

Uh oh!

Mythicaeda Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	extract(epoch from ee.duration) as duration,
	extract(epoch from ee.duration)*1e6 as duration_micros,

Conversation

pranav-super commented Feb 25, 2026

Description

Verification

Documentation

Future work

Uh oh!

Mythicaeda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mythicaeda Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Step 1: Create a Parser for "Event Attributes"

Step 2: Use PostgresParsers.getJsonColumn

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mythicaeda Apr 20, 2026 •

edited

Loading

Step 2: Use `PostgresParsers.getJsonColumn`