[AERIE-1935] Expose plan start time to mission model

[pcrosemurgy]

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

Description

Mission modelers currently don't have access to a plan's start time.
With this PR a plan start time Instant parameter can optionally be declared in the top-level mission model ("Mission.java") constructor.

If a top-level mission model defines a constructor with Instant as the 2nd argument then the sim. start time will be passed to the mission model upon instantiation. If the Instant parameter (or Registrar parameter and optionally a "Configuration" parameter) is declared out-of-order then an actionable error message is now presented at compile-time. See the section below for examples.

More concretely, the following top-level mission model constructor parameters are allowed (lifted from MissionModelParser.java):

    // - (Registrar, Instant, Configuration)
    // - (Registrar, Configuration)
    // - (Registrar, Instant)
    // - (Registrar)

As seen above, both sim. start time and mission model configuration parameters are optional. However, if both are defined "Configuration" must be declared last. This ensures backwards compatibility with existing mission models while allowing future mission models to make use of this Instant.

See the future work section for a discussion on alternative approaches for this PR.

Verification

See foo-missionmodel's MissionConfigurationTest.

Error Handling

When the following Mission constructor parameters are declared in an allowable order no compile-time error is thrown:

public Mission(final Registrar registrar, final Instant planStart, final Configuration config) {
  ...
}

However, when Instant (or any of the above parameters) are declared out of expected order a custom error message will appear:

.../foomissionmodel/package-info.java:21: error: The top-level model's constructor is expected to accept [gov.nasa.jpl.aerie.merlin.framework.Registrar, java.time.Instant, gov.nasa.jpl.aerie.foomissionmodel.Configuration], found: [gov.nasa.jpl.aerie.merlin.framework.Registrar, gov.nasa.jpl.aerie.foomissionmodel.Configuration, java.time.Instant]

In this example, Mission's constructor was declared with a plan start Instant in the wrong expected order:

public Mission(final Registrar registrar, final Configuration config, final Instant planStart) {
  ...
}

Documentation

• See deployment/Environment.md and deployment/README.md for information on UNTRUE_PLAN_START.
• See https://github.com/NASA-AMMOS/aerie/wiki/Developing-a-Mission-Model#mission-model-class
• See https://github.com/NASA-AMMOS/aerie/wiki/Configuring-a-Mission-Model#mission-model

Future work

Initially a @PlanStart annotation was considered but in prototyping this implementation it was found that this change would require plan information to be passed in at our GQL layer (to allow any configured context to have plan information).
This would break existing GQL queries and more tightly couple our plan and model "domains".

See Slack thread for more context and discussion on this.

[mattdailis]

From reading the PR description, it sounds like the annotation approach would be preferable, but we're concerned that having different annotations for records vs regular classes would cause confusion. Am I reading that correctly?

Would it help to have two similarly named annotations in different names paces, or would that make it even more confusing?

[Twisol]

• I don't believe it's possible to define a single annotation for use as both a class member variable and constructor parameter but may be wrong.

I'd expect this to be possible. Annotations don't do anything on their own -- it's up to our processor how to interpret them.

If you're concerned about how to set java.lang.annotation.Target to specify where the annotation is allowed, @Target can accept a list of locations:

@Target({ElementType.RECORD_COMPONENT, ElementType.FIELD})
@interface SimulationStart {
}

(Also, can you remind me what the difference is between the simulation start time and the plan start time?)

[Twisol]

• I'm not 100% sure the above record annotation example is possible (see https://stackoverflow.com/questions/67168624/constructor-annotation-on-java-records).

That question is asking about propagating annotations on records to their synthesized constructors. I don't think this matters to us -- if we know it's a record, we can look at the record's components directly, which IIRC is how the other record-based analyses in our processor work.

[pcrosemurgy]

Also, can you remind me what the difference is between the simulation start time and the plan start time?

Sim. start time is passed through from a "create sim. message". Currently I believe they should always be the same but we're carefully making sure to use sim. start time here since I believe in the future we may have sim. and plan start time differ (for example when re-running part of a sim. I believe).

[pcrosemurgy]

@Twisol

If you're concerned about how to set java.lang.annotation.Target to specify where the annotation is allowed, @ Target can accept a list of locations:

Thanks for the help! I'll rework this PR to use a single annotation with multiple targets. Having this annotation apply to only config. types will complicate our mapper code-gen. but an annotation does seem like the correct approach. :)

[pcrosemurgy]

Everything is now stable. Done making changes :)

[goetzrrGit]

Just to understand it better the mission model will use the UNTRUE_PLAN_START by default unless specified by the user in their queries?

[pcrosemurgy]

Just to understand it better the mission model will use the UNTRUE_PLAN_START by default unless specified by the user in their queries?

The mission model will actually use UNTRUE_PLAN_START for every mission model instantiation besides the instantiation done for a simulation run. So any of these:

[pcrosemurgy]

When running a simulation the actual plan start time will be used :)

[goetzrrGit]

Thanks for the info!! I also just read through the slack thread I missed between you, Matt, and Jonathan, which gave me more context.

[pcrosemurgy]

Thanks Ryan, appreciate you spending time on this w/ us!

[goetzrrGit]

Not much I would improve upon. Just make sure you create another ticket for the mission model redesign.

[mattdailis]

Looks good to me. If you haven't already, let's make a ticket to come up with the longer term solution.

EDIT: Whoops, just saw that Ryan said the same thing