Use strong types for additional data in problem reports #28964

donat · 2024-04-25T10:15:09Z

One feature we want to support for the Problems API is to be attach domain-specific additional data to problem reports. There's a(n internal) solution that uses a string map, which is quite rudimentary and we don't want to make it part of the actual API.

The ultimate goal is to use template to declare additional data for problems.

interface Problem<S> {
    S getAdditionalData();
}

This PR implements the first step to get there: it replaces the string map in the private API with on object reference. The main change is new signature for the InternalProblemSpec.additionalData() method:

<U extends AdditionalDataSpec> InternalProblemSpec additionalData(Class<? extends U> specType, Action<? super U> config);

It allows producers to specify an action that configures additional data. The spec types, and their corresponding custom additional data types are predefined, with the (functionally sealed) AdditionalData and AdditonalDataSpec interfaces. The PR add support for three data types: generic data (ie the old string map), type validation data, and deprecation data.

We PR does not expose these new types on the Tooling API events, although it establishes the pattern on how to do it, should the use-case arise. For now, all additional data types are exposed with the original string map format with a clear documentation that the content of the map is subject of change between the releases.

donat · 2024-05-10T10:11:57Z

...model-core/src/main/java/org/gradle/internal/reflect/validation/TypeAwareProblemBuilder.java

@@ -25,8 +25,6 @@
 public interface TypeAwareProblemBuilder extends InternalProblemSpec {
 TypeAwareProblemBuilder withAnnotationType(@Nullable Class<?> classWithAnnotationAttached);

- TypeAwareProblemBuilder typeIsIrrelevantInErrorMessage();


We can identify which type validation problems should not have the type in the error message based on the problem ID; we no longer need to have a separate field for that.

wolfs · 2024-05-13T09:43:04Z

...ent/src/main/java/org/gradle/plugin/devel/tasks/internal/ValidationProblemSerialization.java

@@ -78,6 +87,7 @@ public static GsonBuilder createGsonBuilder() {
 gsonBuilder.registerTypeHierarchyAdapter(DocLink.class, new DocLinkAdapter());
 gsonBuilder.registerTypeHierarchyAdapter(ProblemLocation.class, new LocationAdapter());
 gsonBuilder.registerTypeHierarchyAdapter(ProblemCategory.class, new ProblemCategoryAdapter());
+ gsonBuilder.registerTypeHierarchyAdapter(AdditionalData.class, new AdditionalDataAdapter());


🤔 I didn't see this code before. Why are we using JSON for ValidationProblemSerialization while using Kryo for most other things?

Thank you for the review!
I am not sure. The JSON-based serialisation was already there when we got started with the Problems API integration.

It might be interesting to find out what is going on there.

I introduced the JSON part, it used to be just a file that we would read on the other end.
with no structured information.

wolfs · 2024-05-13T09:44:32Z

...s/ide/problems-api/src/main/java/org/gradle/api/problems/internal/DefaultProblemBuilder.java

-public class DefaultProblemBuilder implements InternalProblemBuilder {
+public class DefaultProblemBuilder implements InternalProblemBuilder {
+
+ private static List<Class<?>> supportedAdditionalDataTypes = ImmutableList.<Class<?>>of(


🤔 Do you already have plans how to handle extensibility here?

For now, I want to keep the AdditionalData as sealed class. Gradle should control what are the allowed types and disallow any extensibility from third parties.
If we have a reasonable use-case for third parties, only then I'd proceed with extensibility. The idea is to do what we do around model building: producers register what types of data they want to expose. Consumers would declare what kind of data they expect and the problems API would only forward the known items. I'd only consider this for spec/implementation if we have a very compelling use-case.

wolfs · 2024-05-13T09:45:00Z

...s/ide/problems-api/src/main/java/org/gradle/api/problems/internal/DefaultProblemBuilder.java


-public class DefaultProblemBuilder implements InternalProblemBuilder {
+public class DefaultProblemBuilder implements InternalProblemBuilder {


❓ Is there a spec for this change to help understand the reasoning?

I considered this as an internal refactoring, so I don't have any spec for this. At the same time, I'll add some description very soon to this PR.

I've updated the PR's description to reflect to your question.

wolfs · 2024-05-13T09:51:23Z

platforms/ide/problems-api/src/main/java/org/gradle/api/problems/internal/DefaultProblem.java

@@ -86,7 +85,7 @@ public RuntimeException getException() {
 }

 @Override
- public Map<String, Object> getAdditionalData() {
+ public AdditionalData<?> getAdditionalData() {


🤔 I was expecting something like this:

Problem gets a type parameter describing what kind of data it uses.

ProblemDefinition should also have the type parameter, though it probably doesn't need it in the first place

This getter returns the type parameter.

I understand that you need the type of the builder when adding the additional data. Though as soon as it has been created, you don't need the builder type any more, right? I'll leave a comment to the method in InternalProblemSpec.

That was essentially my first attempt, but the change got so big that I decided to step back and deal with the removal of the Map type for additionalData. Which is this PR.

wolfs · 2024-05-13T09:54:44Z

...rms/ide/problems-api/src/main/java/org/gradle/api/problems/internal/InternalProblemSpec.java

 */
- InternalProblemSpec additionalData(String key, Object value);
+ <T extends AdditionalData<U>, U> InternalProblemSpec additionalData(Class<? extends AdditionalData<U>> type, Action<? super U> config);


Suggested change

<T extends AdditionalData<U>, U> InternalProblemSpec additionalData(Class<? extends AdditionalData<U>> type, Action<? super U> config);

<T extends AdditionalData, U extends Builder<T>> InternalProblemSpec additionalData(Class<U> builderType, Action<? super U> config);

❓ Would something like this work? So you'd need to specify the builder type when adding additional data instead of specifying the builder with the additional data itself. That way consumers don't need to worry about the builder type.

That's fair, I'll take a stab at it.

The nice thing about the current implementation is the ease of use of the API. For example, when you write something like

.additionalData(DeprecationData.class,

The type system will only allow you to pass an object of type Action<DeprecationDataSpec> as a second parameter.

That way consumers don't need to worry about the builder type.

It's not exactly a builder, but a spec type; a companion interface declaring what are the allowed configuration options for particular additional data type. Is this something that you'd disallow seeing on the consumer-side?

I think what you'd pass in the API is the spec type, and then the spec type should know the additional data it can build. The consumer should only be interested in the actual data, not what was used to construct it, right?

Fixed; ptal

wolfs · 2024-05-13T09:56:54Z

...rms/ide/tooling-api/src/main/java/org/gradle/tooling/events/problems/TypeValidationData.java

+ */
+@Incubating
+@NonNullApi
+public interface TypeValidationData extends AdditionalData {


❌ This is missing a type parameter.

This is a Tooling API event type; it does not have the same type parameters you have it on the producer side (ie in org.gradle.api.problems.internal.TypeValidationData).

Why do we expose the TypeValidationDatavia the TAPI? Is this something we want to have for Android Studio?

I wanted to expose something to showcase how we can expose custom additional data types. It doesn't have to be TypeValidationData, but now you mentioned it, we probably should not expose anything new on the Tooling API without a clear use-case.

I've removed the tapi event subtype representing type validation problems.

donat · 2024-05-16T14:29:04Z

@bot-gradle test this

bot-gradle · 2024-05-16T14:29:13Z

I've triggered the following builds for you. Click here to see all build failures.

PullRequestFeedback build

The internal Problems API allows clients to attach additional data. The data is currently stored in a map object. This change introduces strong, domain-specific types for the same daa.

donat · 2024-05-16T15:05:47Z

@bot-gradle test this

bot-gradle · 2024-05-16T15:05:54Z

I've triggered the following builds for you. Click here to see all build failures.

PullRequestFeedback build

gradle deleted a comment from donat Apr 25, 2024

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch from 88e9456 to d46e542 Compare April 25, 2024 12:13

gradle deleted a comment from donat Apr 25, 2024

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch 2 times, most recently from 23c1a4c to 8629122 Compare April 26, 2024 13:39

donat changed the title ~~Donat/problems/strongly typed additional data 2~~ Introduce strong t Apr 26, 2024

donat changed the title ~~Introduce strong t~~ Introduce strong types for InternalProblemSpec.additionalData() Apr 26, 2024

gradle deleted a comment from donat Apr 26, 2024

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch from 8629122 to 8c46ac0 Compare May 2, 2024 07:31

gradle deleted a comment from donat May 2, 2024

gitstream-cm bot added the 30 min review label May 2, 2024

donat changed the title ~~Introduce strong types for InternalProblemSpec.additionalData()~~ Use strong types for additional data in problem reports May 2, 2024

donat changed the title ~~Use strong types for additional data in problem reports~~ Use strong types for additional data in problem reports May 2, 2024

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch from 8c46ac0 to 6f51fd3 Compare May 2, 2024 08:04

gradle deleted a comment from donat May 2, 2024

donat self-assigned this May 7, 2024

donat mentioned this pull request May 8, 2024

Fix task selection problem reports #29050

Merged

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch from 6f51fd3 to de41444 Compare May 9, 2024 14:50

gradle deleted a comment from donat May 9, 2024

gradle deleted a comment from donat May 10, 2024

donat commented May 10, 2024

View reviewed changes

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch from 5e482f1 to 6ff6f30 Compare May 10, 2024 15:40

donat marked this pull request as ready for review May 10, 2024 15:40

donat requested review from a team as code owners May 10, 2024 15:40

donat requested review from ghale, bamboo and mlopatkin and removed request for a team, ghale, bamboo and mlopatkin May 10, 2024 15:40

wolfs reviewed May 13, 2024

View reviewed changes

gitstream-cm bot added the build-script-change label May 14, 2024

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch from af6e537 to 0cbcad5 Compare May 15, 2024 07:01

gitstream-cm bot removed the build-script-change label May 15, 2024

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch 3 times, most recently from 4c7e2ae to e59995c Compare May 16, 2024 14:10

gradle deleted a comment from donat May 16, 2024

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch from e59995c to cb6a9ca Compare May 16, 2024 14:24

gradle deleted a comment from donat May 16, 2024

Introduce strong types for InternalProblemSpec.additionalData

0d02c1c

The internal Problems API allows clients to attach additional data. The data is currently stored in a map object. This change introduces strong, domain-specific types for the same daa.

donat force-pushed the donat/problems/strongly-typed-additional-data-2 branch from cb6a9ca to 0d02c1c Compare May 16, 2024 15:05

gradle deleted a comment from donat May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use strong types for additional data in problem reports #28964

Use strong types for additional data in problem reports #28964

donat commented Apr 25, 2024 •

edited

donat May 10, 2024

wolfs May 13, 2024

donat May 13, 2024

wolfs May 13, 2024

reinsch82 May 13, 2024

donat May 16, 2024

wolfs May 13, 2024

donat May 13, 2024

wolfs May 13, 2024

donat May 13, 2024

donat May 16, 2024

wolfs May 13, 2024

donat May 13, 2024

wolfs May 13, 2024

donat May 13, 2024

donat May 14, 2024

wolfs May 14, 2024

donat May 16, 2024

wolfs May 13, 2024

donat May 13, 2024

wolfs May 13, 2024

donat May 14, 2024

donat May 16, 2024

donat commented May 16, 2024

bot-gradle commented May 16, 2024

donat commented May 16, 2024

bot-gradle commented May 16, 2024


		public class DefaultProblemBuilder implements InternalProblemBuilder {
		public class DefaultProblemBuilder implements InternalProblemBuilder {

	<T extends AdditionalData<U>, U> InternalProblemSpec additionalData(Class<? extends AdditionalData<U>> type, Action<? super U> config);
	<T extends AdditionalData, U extends Builder<T>> InternalProblemSpec additionalData(Class<U> builderType, Action<? super U> config);

Use strong types for additional data in problem reports #28964

Are you sure you want to change the base?

Use strong types for additional data in problem reports #28964

Conversation

donat commented Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donat commented May 16, 2024

bot-gradle commented May 16, 2024

donat commented May 16, 2024

bot-gradle commented May 16, 2024

donat commented Apr 25, 2024 •

edited