search

smallrye / smallrye-graphql

Implementation for MicroProfile GraphQL
Apache License 2.0
160 stars 92 forks source link

Typed Client: all classes for the possibleTypes have to be present #2224

Open jmini opened 2 weeks ago

jmini commented 2 weeks ago

On the GitLab GraphQL server (no password required), the WorkItemWidget type (kind INTERFACE) has multiple possible types.

You can run a query like this:

query {
  workItemsByReference(contextNamespacePath: "gitlab-org",
    refs: [
      "&9290"
    ]) {
    nodes {
      id
      iid
      title
      webUrl
      workItemType {
        name
      }
      widgets {
        ... on WorkItemWidgetLabels {
          labels {
            nodes {
              id
              title
            }
          }
        }
        ... on WorkItemWidgetLinkedItems {
          blocked
          blockedByCount
          blockingCount
          linkedItems {
            nodes {
              linkId
              linkType
              linkCreatedAt
              linkUpdatedAt
              workItem {
                id
              }
            }
          }
        }
      }
    }
  }
}

https://gitlab.com/-/graphql-explorer

With the java typed client, you need to have all the sub-types declared (as empty class), even if you are only interested in two widgets.

import org.eclipse.microprofile.graphql.Name;

import io.smallrye.graphql.api.Union;
import jakarta.json.bind.annotation.JsonbSubtype;
import jakarta.json.bind.annotation.JsonbTypeInfo;

@Union
@JsonbTypeInfo(key = "__typename", value = {
        @JsonbSubtype(alias = "WorkItemWidgetAssignees", type = WorkItemWidgetAssignees.class),
        @JsonbSubtype(alias = "WorkItemWidgetAwardEmoji", type = WorkItemWidgetAwardEmoji.class),
        @JsonbSubtype(alias = "WorkItemWidgetColor", type = WorkItemWidgetColor.class),
        @JsonbSubtype(alias = "WorkItemWidgetCrmContacts", type = WorkItemWidgetCrmContacts.class),
        @JsonbSubtype(alias = "WorkItemWidgetCurrentUserTodos", type = WorkItemWidgetCurrentUserTodos.class),
        @JsonbSubtype(alias = "WorkItemWidgetDescription", type = WorkItemWidgetDescription.class),
        @JsonbSubtype(alias = "WorkItemWidgetDesigns", type = WorkItemWidgetDesigns.class),
        @JsonbSubtype(alias = "WorkItemWidgetDevelopment", type = WorkItemWidgetDevelopment.class),
        @JsonbSubtype(alias = "WorkItemWidgetHealthStatus", type = WorkItemWidgetHealthStatus.class),
        @JsonbSubtype(alias = "WorkItemWidgetHierarchy", type = WorkItemWidgetHierarchy.class),
        @JsonbSubtype(alias = "WorkItemWidgetIteration", type = WorkItemWidgetIteration.class),
        @JsonbSubtype(alias = "WorkItemWidgetLabels", type = WorkItemWidgetLabels.class),
        @JsonbSubtype(alias = "WorkItemWidgetLinkedItems", type = WorkItemWidgetLinkedItems.class),
        @JsonbSubtype(alias = "WorkItemWidgetMilestone", type = WorkItemWidgetMilestone.class),
        @JsonbSubtype(alias = "WorkItemWidgetNotes", type = WorkItemWidgetNotes.class),
        @JsonbSubtype(alias = "WorkItemWidgetNotifications", type = WorkItemWidgetNotifications.class),
        @JsonbSubtype(alias = "WorkItemWidgetParticipants", type = WorkItemWidgetParticipants.class),
        @JsonbSubtype(alias = "WorkItemWidgetRolledupDates", type = WorkItemWidgetRolledupDates.class),
        @JsonbSubtype(alias = "WorkItemWidgetStartAndDueDate", type = WorkItemWidgetStartAndDueDate.class),
        @JsonbSubtype(alias = "WorkItemWidgetStatus", type = WorkItemWidgetStatus.class),
        @JsonbSubtype(alias = "WorkItemWidgetTimeTracking", type = WorkItemWidgetTimeTracking.class),
        @JsonbSubtype(alias = "WorkItemWidgetWeight", type = WorkItemWidgetWeight.class)
})
@Name("WorkItemWidget")
public interface WorkItemWidget {
}

If one is missing (as in this example) you get this stacktrace:

java.lang.RuntimeException: SRGQLDC035010: Cannot instantiate WorkItemWidgetEmailParticipants
    at io.smallrye.graphql.client.impl.typesafe.reflection.TypeInfo.lambda$subtype$9(TypeInfo.java:474)
    at java.base/java.util.Optional.orElseThrow(Optional.java:403)
    at io.smallrye.graphql.client.impl.typesafe.reflection.TypeInfo.subtype(TypeInfo.java:474)
    at io.smallrye.graphql.client.impl.typesafe.json.JsonObjectReader.readObject(JsonObjectReader.java:32)
    at io.smallrye.graphql.client.impl.typesafe.json.JsonObjectReader.read(JsonObjectReader.java:27)
    at io.smallrye.graphql.client.impl.typesafe.json.JsonReader.read(JsonReader.java:62)
    at io.smallrye.graphql.client.impl.typesafe.json.JsonReader.readJson(JsonReader.java:37)
    at io.smallrye.graphql.client.impl.typesafe.json.JsonArrayReader.readItem(JsonArrayReader.java:41)
    at io.smallrye.graphql.client.impl.typesafe.json.JsonArrayReader.lambda$read$0(JsonArrayReader.java:33)

Setting allowUnexpectedResponseFields does not help here.

        WorkitemClientApi gqlApi = TypesafeGraphQLClientBuilder.newBuilder()
                .endpoint("https://gitlab.com/api/graphql")
                .header("Authorization", "Bearer " + gitlabToken) //no needed for the discussed read query
                .allowUnexpectedResponseFields(true)
                .build(WorkitemClientApi.class);

Complete project: https://github.com/jmini/gitlab-experiments/tree/main/smallrye-graphql-client

I am not sure if something can be done about this, but I wanted to share this with you (and it might help other users).

jmini commented 2 weeks ago

Also I don't know if the error message SRGQLDC035010 is used somewhere else, but it would be better to say:

Cannot instantiate WorkItemWidgetEmailParticipants (not defined as sub-type of WorkItemWidget)

To help developers to understand what is going on.

jmini commented 2 weeks ago

The fix is simple for this error is simple.

For my example project: https://github.com/jmini/gitlab-experiments/commit/7a4edce7cb3e530983054cebd66508bf294ee468

jmartisk commented 2 weeks ago

We could theoretically ignore unknown subtypes, but it doesn't sound very safe to me, it could lead to unexpected issues, for example, if the user wrongly declares the name of the type, then the object in the response would be silently skipped. The improved exception message makes sense to me...

jmartisk commented 2 weeks ago

@t1 any opinion on this?

jmini commented 2 weeks ago

We could theoretically ignore unknown subtypes, but it doesn't sound very safe to me, it could lead to unexpected issues, for example, if the user wrongly declares the name of the type, then the object in the response would be silently skipped.

Yes I am also not sure about what is correct.

What is sure with the current pattern:

1) when GitLab is adding a new widget type, this breaks our client until we add something like https://github.com/jmini/gitlab-experiments/commit/7a4edce7cb3e530983054cebd66508bf294ee468

2) I had to add many empty classes just to make the Json de-serialization happy, even if in my case (at graphQL query level) I am only interested in ... on WorkItemWidgetLabels and ... on WorkItemWidgetLinkedItems. (I could have reused a WorkItemWidgetDummy and mapped all the cases I am not interested in to this class, but the @JsonbSubtype has to be complete, for each of the possible __typename values)

Maybe the Server API design is not ideal:

The WorkItem Type (kind OBJECT) have this field:

widgets [WorkItemWidget!] Collection of widgets that belong to the work item.

Without any filtering possibility. and the WorkItemWidget has tons of subtypes.

t1 commented 1 week ago

Do I get this right: it would work, if you would only receive the types you are interested in? In other words: it only fails, if you actually receive a type that you don't expect? So you want to filter some types on the client side. That feels to be going against a design goals of GraphQL: the client specifies exactly what they want.

So I think it would be better (for the world, not necessarily for you at this moment ;-), if the API would allow that type of filtering on the widgets field... something like widgets(typeFilter: ["WorkItemWidgetLabels", "WorkItemWidgetLinkedItems"]). That would also reduce the amount of data that the server had to produce and transport to the client. I have no idea how big these things can be, but it looks like this could make a relevant difference... even without fixing the problems you have. Maybe you can suggest that to GitLab?

Despite me being reluctant to add this, I've looked at the Typesafe Client code: the change would actually be quite simple. The TypeInfo#subtype method could return an Optional, and readObject method could ignore an empty value. If we actually do decide to do that, I think we should absolutely make this an opt-in with a new config option.

Do you understand or even agree with my point of view?

BTW: I completely agree on improving on the error message.

jmini commented 1 week ago

So I think it would be better (for the world, not necessarily for you at this moment ;-), if the API would allow that type of filtering on the widgets field... something like widgets(typeFilter: ["WorkItemWidgetLabels", "WorkItemWidgetLinkedItems"]).

I think we all agree on that point, but the GitLab server is what it is. Because you asked, I will open the issue in their tracker, but I am not sure they will change their design.

I will not try to defend their pattern (that seems to be very UI/frontent driven at the end), but I don't think that there is too much data transported, since only the ... on WorkItemWidgetXXXX are contributing to the response. If you check my example query, you will see in the response:

          "widgets": [
            {},
            {},
            {},
            {
              ... content of WorkItemWidgetLabels
            },
            {},
            {},
            {},
            {},
            {},
            {},
            {
              ... content of WorkItemWidgetLinkedItems
            },
            {},
            {},
            {},
            {},
            {},
            {}
          ]

I agree that those {} are noisy, but they are not generating too much data.

For me this discussion is about the deserialization at client side.

Currently we need one entry for each of the possible __typename value that is use as key to discriminate between the subtypes.

I think we should absolutely make this an opt-in with a new config option.

For me this would go in the same direction as .allowUnexpectedResponseFields(true). Something you do not really need an idealistic word.

With that new config set to true my goal would be an attempt to make the client more robust and ignore those empty {} objects. I could even reduce my model to:

@Union
@JsonbTypeInfo(key = "__typename", value = {
        @JsonbSubtype(alias = "WorkItemWidgetLabels", type = WorkItemWidgetLabels.class),
        @JsonbSubtype(alias = "WorkItemWidgetLinkedItems", type = WorkItemWidgetLinkedItems.class)
})
@Name("WorkItemWidget")
public interface WorkItemWidget {
}

--> which is great, because I only need those two case.

Without this config, I have to fix the client like this https://github.com/jmini/gitlab-experiments/commit/7a4edce7cb3e530983054cebd66508bf294ee468 on each schema change (each addition of a new Widget sub-type is breaking)

But indeed, for users that have more control of the GraphQL server, this new config would not be necessary.

I can understand if you reject this idea of introducing a config. Then this would just be a known fact and users like me needs to update their model, which is fine (maybe this GitLab use-case is a corner case)

In all cases, the error message can be improved to help identifying which sub-type declaration is missing in which interface.

t1 commented 1 week ago

Of course! You're right. The "filtering" already happens (practically) via the fragments you specify; the empty {}s are just a bit ugly.

So forget what I've said before. This is a useful feature. Do you think you could create a PR for this?

jmini commented 1 week ago

I am not yet really familiar with the code base. I would need some pointers:

  1. can the signature of io.smallrye.graphql.client.impl.typesafe.reflection.TypeInfo.subtype(String) be changed to Optional<TypeInfo>
  2. where are you defining the config and reading them?
  3. where are the tests? And how do you extend them (is there a backend somewhere)?
t1 commented 1 day ago

sorry for the delay...

  1. can the signature of io.smallrye.graphql.client.impl.typesafe.reflection.TypeInfo.subtype(String) be changed to Optional

yes, sure.

  1. where are you defining the config and reading them?

We use MP Config. As a lot of config keys depend on a logical API client name (a.k.a. configKey) or API interface name, they like my-api/mp-graphql/url. But I'm not sure if we want that for this, too. What do you think?

  1. where are the tests? And how do you extend them (is there a backend somewhere)?

The tests live in the client/tck module. They end with Behavior instead of Test, which is more BDD like. Maybe UnionBeharior is a good test class to add your test.