[Story] GraphQL Fetchers should only populate response as-needed

[andrewazores]

For example: https://github.com/cryostatio/cryostat/blob/2b052d65717f71c2f00c43b3539c5edbc2133ed7/src/main/java/io/cryostat/net/web/http/api/v2/graph/RecordingsFetcher.java#L110

When performing a query for only archived recordings, for example, this implementation unconditionally populates both the active and archived fields of the recordings object. This means that the GraphQL resolver ends up opening a JMX connection and querying for active recordings, potentially across very many different targets across the network, when all that is actually needed to be done is to list the contents of the archived directories in local filesystem.

This is a common pattern in GraphQL so there are well-known techniques to solve it: https://stackoverflow.com/questions/65935113/getting-requested-fields-on-resolver-level-from-graphql

In this particular example, the RecordingsFetcher should check the requested fields from the DataFetchingEnvironment and only populate them as needed. If the API client has only requested information about archived recordings, the active side of the Recordings structure should be left unpopulated (null or just empty list? In types.graphqls this is defined as non-null) and no target JMX connections opened.

This is a [Story] because the nullability of various GraphQL response types should be re-evaluated, and there are a lot of GraphQL implementation points that should be reviewed as well to ensure that response computations only occur if it's part of the client's requested fields.

[hareetd]

In this particular example, the RecordingsFetcher should check the requested fields from the DataFetchingEnvironment and only populate them as needed.

In #924 I separated the retrieving of active and archived recordings using this method so that the Archives view could populate its archived recordings count data without needing to open a target JMX connection:

https://github.com/cryostatio/cryostat/blob/22380ca0bd76689b3de162fb94487f1124d3775c/src/main/java/io/cryostat/net/web/http/api/v2/graph/RecordingsFetcher.java#L110

This is a [Story] because the nullability of various GraphQL response types should be re-evaluated

I left the nullability of the involved data types as is, and retrieving just the active recordings or just the archived recordings works without any issues, despite both the active and archived fields being defined as non-null. GraphQL only complains if neither of the two data types is requested:

query {
    targetNodes(filter: { name: "service:jmx:rmi:///jndi/rmi://cryostat:9093/jmxrmi" }) {
        recordings
    }
}

So this should be a simpler change than expected.

[hareetd]

So it seems the RecordingsFetcher is unique because it returns a static Recordings object, with active and archived fields, which lends itself well to the conditional computation of these fields depending on what the user has actually requested. In #924 I changed the ArchivedRecordingsFetcher to also return a static object, specifically an instance of the Archived class. However, it feels a bit contrived to only populate the requested fields (data and aggregate) as needed for this object since the computation of the aggregate information is dependent on knowing what the actual data is.

The remaining Fetchers are different because they return the base data types defined in types,graphqls (eg. EnvironmentNode, TargetNode, ActiveRecording, ArchivedRecording etc.) which means their fields are populated by virtue of being mapped to the same fields in their Java object representations. For fields that don't fit this criterion (eg. descendantTargets in the EnvironmentNode type), they're mapped to their own separate Fetcher meaning the computation is only done if the user requests that field.

In light of this, I don't think we need to populate responses as needed for any other Fetchers besides the RecordingsFetcher, which was already taken care of in #294. Thoughts?

[andrewazores]

I agree, I think the only currently existing case for it is the RecordingsFetcher. Just refactoring to clean up that one implementation solves the immediate concern here. Solving it in some extensible or abstracted way so that we can easily reuse the same logic for any future cases would also be great.

[hareetd]

In that case, I believe the RecordingsFetcher is good to go already from #924 onwards (see my first comment above). The AllTargetsArchivedRecordingsTable on -web makes use of the conditional population of the archived field in the RecordingsFetcher when it queries for only a target's aggregate count, bypassing the opening of a JMX connection. Feel free to test it out yourself as well.

[andrewazores]

Yep, looks good.

Closed by #924