[DCJ-503] Gracefully handle DRS resolution failure case for self-hosted snapshots

[okotsopoulos]

Jira ticket: https://broadworkbench.atlassian.net/browse/DCJ-503

Addresses

In production, we are seeing some DRS resolution attempts fail with an NPE:

java.lang.NullPointerException: Cannot invoke \"com.google.cloud.storage.Bucket.getLocation()\" because \"bucket\" is null"

Reading the corresponding stack trace shows that this is happening for self-hosted GCP snapshots (logs).

Summary of changes

• Gracefully handle the case where TDR cannot obtain the GCS bucket for a file object within a self-hosted GCP snapshot.
• Provide diagnostic information on the source snapshot, root dataset, and file object in the error details. (Note that if we've gotten this far in our attempted DRS resolution, we've already verified that the user is authorized to read the snapshot, so they're allowed to get this diagnostics information back.)

An example exception thrown with this change (obtained from unit tests, so may not be fully populated as we've seen in the wild):

DrsObjectNotFoundException
GCS bucket from gs://path/to/file.txt not found (diagnostics returned in error details)
[class SnapshotCacheResult {
  snapshot: {
    id: e1c97007-d80a-487d-90da-f019ba6a9ebd
    name: null
    isSelfHosted: true
    globalFileIds: false
    billingProfileId: bd9b1387-2fb3-4aad-959f-c6015cdaab8a
    cloudPlatform: gcp
    googleProjectId: snapshot-google-project
  }
  rootDataset: {
    id: null
    name: null
    datasetProjectId: dataset-google-project
  }
}, bio.terra.service.filedata.FSFile@3cb01485[datasetId=<null>,cloudPath=gs://path/to/file.txt,mimeType=<null>,bucketResourceId=1fc4ea5c-fdc7-4262-ba38-7a8dcfbeb89b,loadTag=<null>]]

Testing Strategy

Expanded unit tests.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[rushtong]

Thank you 👍🏽

[pshapiro4broad]

Looks OK, but using a record may be a better way to ensure that all fields in a class are logged.

[pshapiro4broad]

It's longer but I think a text block makes it clearer what the output will look like:

    @Override
    public String toString() {
      return """
          class SnapshotCacheResult {
            snapshot: {
              id: %s
              name: %s
              isSelfHosted: %s
              globalFileIds: %s
              billingProfileId: %s
              cloudPlatform: %s
              googleProjectId: %s
            }
            rootDataset: {
              id: %s
              name: %s
              datasetProjectId: %s
            }
          }
          """
          .formatted(
              id,
              name,
              isSelfHosted,
              globalFileIds,
              snapshotBillingProfileId,
              cloudPlatform,
              googleProjectId,
              datasetId,
              datasetName,
              datasetProjectId);
    }

[pshapiro4broad]

Looking at the constructor for SnapshotCacheResult, it looks possible to have a null googleProjectId even if isSelfHosted is true. If that happens, I think BucketGetOption.userProject() will throw an NPE.

If the code is correct -- when isSelfHosted is true, googleProjectId is not null -- you can make it explicit in the code by using requireNonNull()

Suggested change

	BucketGetOption userProject = BucketGetOption.userProject(cachedSnapshot.googleProjectId);
	BucketGetOption userProject = BucketGetOption.userProject(Objects.requireNonNull(cachedSnapshot.googleProjectId));

[pshapiro4broad]

Why are some data provided in the initial error message and some are appended at the end? Could all the data be provided in the error message? My understanding was causes is used in the case where we want to collect the chained exception causes and roll them up to the top level message.

[pshapiro4broad]

Instead of a custom toString(), how about converting this class to a record? It looks like all the fields are read-only already.

  record SnapshotCacheResult(
      UUID id,
      String name,
      boolean isSelfHosted,
      boolean globalFileIds,
      BillingProfileModel datasetBillingProfileModel,
      UUID snapshotBillingProfileId,
      CloudPlatform cloudPlatform,
      String googleProjectId,
      String datasetProjectId,
      UUID datasetId,
      String datasetName) {

    public SnapshotCacheResult(Snapshot snapshot) {
      this(
          snapshot.getId(),
          snapshot.getName(),
          snapshot.isSelfHosted(),
          snapshot.hasGlobalFileIds(),
          snapshot.getSourceDataset().getDatasetSummary().getDefaultBillingProfile(),
          snapshot.getProfileId(),
          snapshot.getCloudPlatform(),
          Optional.ofNullable(snapshot.getProjectResource())
              .map(GoogleProjectResource::getGoogleProjectId)
              .orElse(null),
          Optional.ofNullable(snapshot.getSourceDataset().getProjectResource())
              .map(GoogleProjectResource::getGoogleProjectId)
              .orElse(null),
          snapshot.getSourceDataset().getId(),
          snapshot.getSourceDataset().getName());
    }

[okotsopoulos]

Great idea! Working on that now.

[fboulnois]

I agree with Phil's comment on string formatting. Otherwise looks good 👍