[AERIE-1858] Facilitate recommended pattern for computed attributes

[mattdailis]

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

Description

The goal of this PR is to demonstrate a recommended pattern for declaring computed attributes. We think that the most idiomatic way to declare computed attributes is with a record, like so:

aerie/examples/banananation/src/main/java/gov/nasa/jpl/aerie/banananation/activities/BiteBananaActivity.java

Lines 29 to 39 in 6a5a5b4

	@EffectModel
	public ComputedAttributes run(final Mission mission) {
	final var bigBiteSize = biteSize > 1.0;
	final var newFlag = bigBiteSize ? Flag.B : Flag.A;
	mission.flag.set(newFlag);
	mission.fruit.subtract(biteSize);
	return new ComputedAttributes(bigBiteSize, newFlag);
	}
	@AutoValueMapper
	public record ComputedAttributes(boolean biteSizeWasBig, Flag newFlag) {}

Records are a low-boilerplate mechanism for expressing sets of key-value pairs, where the keys are fixed (as opposed to Maps, which describe sets key-value pairs where the keys are dynamic).

Prior to this PR, it is still definitely possible to use records for computed attributes. However, every new record type would need a corresponding ValueMapper to be added to the value mapper factory.

In the interest of making our "recommended practice" as easy as possible for users, this PR adds an @AutoValueMapper annotation. This annotation can only be applied to records, and it will generate an AutoValueMappers.java file that contains a method that looks like this:

@Generated("gov.nasa.jpl.aerie.merlin.processor.MissionModelProcessor")
public final class AutoValueMappers {
  public static ValueMapper<BiteBananaActivity.ComputedAttributes> gov_nasa_jpl_aerie_banananation_activities_BiteBananaActivity_ComputedAttributes(
      final ValueMapper<Boolean> biteSizeWasBigValueMapper,
      final ValueMapper<Flag> newFlagValueMapper) {
    return new RecordValueMapper<>(
      gov.nasa.jpl.aerie.banananation.activities.BiteBananaActivity.ComputedAttributes.class,
      List.of(
        new RecordValueMapper.Component<>(
          "biteSizeWasBig",
          gov.nasa.jpl.aerie.banananation.activities.BiteBananaActivity.ComputedAttributes::biteSizeWasBig,
        biteSizeWasBigValueMapper),
        new RecordValueMapper.Component<>(
          "newFlag",
          gov.nasa.jpl.aerie.banananation.activities.BiteBananaActivity.ComputedAttributes::newFlag,
        newFlagValueMapper)
    ));
  }
}

It will simultaneously create a TypeRule that would have been the result of parsing that factory method, had that method been defined prior to annotation processing.

Verification

I have added unit tests for RecordValueMapper. I've also done some visual spot checking. What I would still like to do prior to merging this PR is some end-to-end tests, just as a sanity check that the serialization/deserialization doesn't hit any snags

• manual e2e test where AutoValueMapper is used for computed attributes

Value Schema

{
    "type": "struct",
    "items": {
        "newFlag": {
            "type": "variant",
            "variants": [
                {
                    "key": "A",
                    "label": "A"
                },
                {
                    "key": "B",
                    "label": "B"
                }
            ]
        },
        "biteSizeWasBig": {
            "type": "boolean"
        }
    }
}

- [ ] manual e2e test where AutoValueMapper is used as a parameter

Documentation

• The existing documentation already recommends using a Map. We should just update it to recommend a record instead.

• Perhaps an extra section on @AutoValueMapper should be added to our ValueMapper docs.

Future work

• Test in the UI
• Some of this AutoValueMapper stuff may be reworked if/when we refactor the annotation processing code

[Twisol]

@mattdailis, I haven't had a chance to look too deeply at this yet, but I have a quick question. If a user opts not to use @AutoValueMapper, they can still provide their own custom mapper, correct? I think they would just add the value mapper to a family in one of the package's @MissionModel.WithMappers annotations, right?

[pcrosemurgy]

@mattdailis Sorry if I'm just out of the loop but what's the state of this PR? Is it in kind of a do-not-merge place right now?

[mattdailis]

@mattdailis Sorry if I'm just out of the loop but what's the state of this PR? Is it in kind of a do-not-merge place right now?

It's available for review! I probably need to rebase/resolve merge conflicts, I can do that tomorrow morning 👍

[pcrosemurgy]

@mattdailis let me know if you want/need to pair at all to get this PR through 👌 The conflicts with develop are likely from changes I've pushed through since this PR was opened.

[pcrosemurgy]

Great work Matt!

Last question/comment I have is about the @AutoValueMapper. I'm wondering if this should be called RecordValueMapper or something that indicates it's only supported for computed attribute return types.

Maybe the thinking is that this annotation will eventually support auto value mappers for other parts of mission model? If not then I could see it causing some confusion if developers think it's truly an auto value mapper (without context).

[Twisol]

Maybe the thinking is that this annotation will eventually support auto value mappers for other parts of mission model? If not then I could see it causing some confusion if developers think it's truly an auto value mapper (without context).

I can definitely see this being extended to activity types in the future -- I'm imagining this coming in when we extract resource registration out of model initialization. At that point we should probably move activity type registration into the same place that resource registration ends up, which would mean @ActivityType's behavior will reduce almost entirely to @AutoValueMapper's behavior.

[adrienmaillard]

It's all great. Thank you for the tour, it definitely helped. I only have two minor non-blocking comments.