Resilience of the `buildTarget/*` requests

[adpi2]

The requests of the form buildTarget/* take a sequence of build target identifiers as paramater and expect a sequence of result items as response, each item corresponding to one of the build target identifier.

For example, the buildTarget/source params are

export interface SourcesParams {
  targets: BuildTargetIdentifier[];
}

and result is:

export interface SourcesResult {
  items: SourcesItem[];
}

But what if the request fails on a single build target:

1. should the server fails the entire request and returns an error response?
2. should it returns an incomplete result and ignore the failed build target?

sbt does 1 which is probably not a good idea because we don't want the entire build import to fail if one build target fails. Bloop does 2 but that's not fully satisfying either because the build client doesn't really know why some build targets are missing.

So I propose to augment every buildTarget/* response with a new failedTargets field which must contain the build target failures.

export interface FailedBuildTarget {
  target: BuildTargetIdentifier;
  code: integer;
  message: string;
  data?: string | number | boolean | array | object | null;
}

export interface SourcesResult {
  items: SourcesItem[];
  failedTargets: FailedBuildTarget[];
}

Do you think this is the correct approach for this problem?

[jastice]

Your suggestion sounds reasonable. What should code be in FailedBuildTarget?

[adpi2]

What should code be in FailedBuildTarget?

Probably we don't need it.

[ckipp01]

What should code be in FailedBuildTarget?

Probably we don't need it.

Either that or do you think it'd be useful to also have space for an error or message? Something like:

export interface FailedBuildTarget {
  id: BuildTargetIdentifier;
  message?: String;
}

😆 I someone totally missed that you had an even more robust version up above. Just ignore me, I need to learn how to read fully.

[julienrf]

What would IDEs do with the failures? Would they show a message saying that some build targets could not be resolved?

[ckipp01]

What would IDEs do with the failures? Would they show a message saying that some build targets could not be resolved?

I believe on the Metals side we more or less swallow it. In the buildTarget/sources example it seems that in this scenario with Bloop if we successfully import the build but not all build targets were able to retrieve their their sources for example, then we'd still have a successful import, but one build target wouldn't have any recognized sources. As you can imagine that can cause confusion. If I'm not mistaken we actually had an issue with this before where the build target was recognized, but nothing would work with it, and it was hard to track down the why behind it. I'll have to dig a bit deeper though to see in these situations exactly what is happening. My thought behind this was that on the build server side if they know more information behind why something is failing, it may be helpful to forward that information back to the client in order for the client to at least log it, which may greatly help with debugging the issue. Again, just thinking out loud about this.

[adpi2]

Either that or do you think it'd be useful to also have space for an error or message?

Yes of course. I am just saying that code is probably useless but message should be mandatory and we can also have an optional data.

export interface FailedBuildTarget {
  target: BuildTargetIdentifier;
  message: string;
  data?: string | number | boolean | array | object | null;
}

What would IDEs do with the failures? Would they show a message saying that some build targets could not be resolved?

It's up to them.

Also I agree with @ckipp01 that it's not a good idea to swallow a failure. In general I think if one of the buildTarget/scalacOptions, buildTarget/dependencySources, buildTarget/sources fails the client should clearly states that the import of some build targets failed.

In Metals, the build target error messages could be displayed in the doctor section.

Build target	scalacOptions	sources	dependencySources	message
foo	✓	✓	✓
bar	x	✓	✓	Error downloading some-library_2.13:1.2.3
fizz	✓	x	✓	Failed source generation

[ckipp01]

Yes of course. I am just saying that code is probably useless but message should be mandatory and we can also have an optional data.

Ach, I totally misread the first post and missed that you already had it all defined 😆. Sorry!

[lefou]

This approach makes it necessary to extends each response type with failure information. Wouldn't it be easier to just wrap the specific result type in some generic Either-like result type? The only reason to not do this is, that your proposed solution could be delivered in a backward compatible ways. Whereas my suggestion to use a result wrapper isn't compatible at all.