Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid error being returned: Fields "first" conflict because they have differing arguments. #943

Closed
emwalker opened this issue Jun 5, 2022 · 11 comments
Labels
bug Something isn't working

Comments

@emwalker
Copy link

emwalker commented Jun 5, 2022

First off, thank you for the excellent library. I'm enjoying using it.

Expected Behavior

Query fragments are intended to be used by (e.g., React/Relay) components without needing to coordinate their fragment queries globally across the app and across the consolidated query that is constructed by combining the fragments. This allows components to be developed in isolation without having side effects for other components referring to the same fields. My understanding is that a fragment should provide a namespace of sorts in cases where the same field is being requested across different fragments with different values for the arguments and should work similarly to aliasing the common field with different aliases.

Actual Behavior

When two fragments referring to the same field use different values of the first: argument for a Relay connection, the following error is reported:

Fields "first" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.

Here is the query that triggers this error:

Fragment query
query TopicSearchPage_query_Query(
  $viewerId: ID $orgLogin: String!
  $repoName: String
  $repoIds: [ID!]
  $topicId: ID!
  $searchString: String!
) {
  view(viewerId: $viewerId, currentOrganizationLogin: $orgLogin, currentRepositoryName: $repoName, repositoryIds: $repoIds) {
    viewer {
      ...Link_viewer
      id
    }
    currentRepository {
      displayName
      ...Breadcrumbs_repository
      id
    }
    ...Link_view
    ...Topic_view
    topic(id: $topicId) {
      ...TopicSearchPage_topic_4ES8AM
      id
    }
  }
}

fragment Breadcrumbs_repository on Repository {
  displayName
  isPrivate
  organization {
    login
    defaultRepository {
      displayName
      rootTopic {
        name
        resourcePath
        id
      }
      id
    }
    id
  }
}

fragment Link_link on Link {
  id
  loading
  newlyAdded
  title
  url
  repository {
    displayColor
    id
  }
  parentTopics(first: 1000) {
    edges {
      node {
        displayName: name
        resourcePath
        id
      }
    }
  }
}

fragment Link_view on View {
  currentRepository {
    name
    id
  }
}

fragment Link_viewer on User {
  isGuest
}

fragment TopicSearchPage_topic_4ES8AM on Topic {
  id
  name
  resourcePath
  parentTopics(first: 100) {
    edges {
      node {
        display: name
        resourcePath
        id
      }
    }
  }
  search(searchString: $searchString) {
    edges {
      node {
        __typename
        ... on Topic {
          id
          ...Topic_topic
        }
        ... on Link {
          id
          ...Link_link
        }
      }
    }
  }
}

fragment Topic_topic on Topic {
  description
  displayName: name
  id
  loading
  newlyAdded
  resourcePath
  viewerCanUpdate
  repository {
    name
    displayColor
    id
  }
  parentTopics(first: 100) {
    edges {
      node {
        displayName: name
        resourcePath
        id
      }
    }
  }
}

fragment Topic_view on View {
  currentRepository {
    id
  }
}

In this case, parentTopics(first: 1000) conflicts with parentTopics(first: 100), even though the shared field is referred to in different fragments. A workaround is to adjust the argument value to be the same (e.g, 100). This means the different components have to know about one another and coordinate their behavior. But the components could be from different teams or even different libraries, so that kind of low-level coordination shouldn't be needed.

I believe this is the code doing the validation. Some kind of fragment-related namespacing might be needed to avoid the validation error.

Specifications

  • Version: async-graphql 3.0.38
  • Platform: Linux / Ubuntu
  • Subsystem: N/A
@emwalker emwalker added the bug Something isn't working label Jun 5, 2022
@emwalker emwalker changed the title Invalid error being returned: Fields "{}" conflict because they have differing arguments. Invalid error being returned: Fields "first" conflict because they have differing arguments. Jun 5, 2022
@sunli829
Copy link
Collaborator

sunli829 commented Jun 7, 2022

Thanks for reporting this issue, I need to check out the GraphQL spec. 😁

@sunli829
Copy link
Collaborator

sunli829 commented Jun 7, 2022

fieldA and fieldB must have identical sets of arguments.

It looks like this is not allowed in the GraphQL spec.

https://spec.graphql.org/October2021/#example-00fbf

@emwalker
Copy link
Author

emwalker commented Jun 7, 2022

Please correct me if I'm wrong, but I think that's just saying you can't merge the fields in this case. I.e., they need to be handled separately, as you would aliased fields. I believe this is what https://github.com/99designs/gqlgen does, which handled the query at the top of this report without issues. I'll see if I can dig into the code and see how they're handling this case.

@sunli829
Copy link
Collaborator

sunli829 commented Jun 7, 2022

Please let me know if there is any progress. 😄

@emwalker
Copy link
Author

emwalker commented Jun 11, 2022

I looked into this a little today. The issue is subtle because fragments are involved, and I didn't find much discussion in the spec around fragments. But in this case the parentTopics(first: 100) is on a Topic type, and the parentTopics(first: 1000) is on a Link type, and both types are included in a union type that can either be a Topic or a Link (the following has been simplified):

union SearchResultItem = Topic | Link

type SearchResultItemEdge {
  node: SearchResultItem!
}

type SearchResultItemConnection {
  edges: [SearchResultItemEdge]
}

type Topic {
  search(
    searchString: String!,
    first: Int,
    after: String,
    last: Int,
    before: String
  ): SearchResultItemConnection!
}

Here is the relevant part of the query above:

  search(searchString: $searchString) {
    edges {
      node {
        __typename
        ... on Topic {
          id
          ...Topic_topic
        }
        ... on Link {
          id
          ...Link_link
        }
      }
    }
  }

In cases where there is a union, the field has the same name, the arguments differ, and the parent field types, which are members of the union, differ, it seems this is allowed and that the field is not merged across the different types in the union:

2022-06-11_11-36

So the validation and field collection code probably need to take into account the type that is including the field with the differing arguments.

I looked into the gqlgen project. I didn't find a test handling this case, but the code seems to behave this way.

@sunli829
Copy link
Collaborator

Thanks a lot, I'll dig into it later. 🙂

@sunli829
Copy link
Collaborator

I fixed this on the master branch, can you help test it? 🙂 @emwalker

@emwalker
Copy link
Author

@sunli829 I will try to test this soon.

For the moment, I'm seeing this error when I switch to master that I'm not yet sure how to resolve:

2022-06-24_21-20

This is the code where this happens:

#[post("/graphql")]
async fn index(
    state: web::Data<State>,
    gql_request: GraphQLRequest,
    req: HttpRequest,
) -> GraphQLResponse {
    let user_info = user_id_from_header(req);
    let viewer = state.authenticate(user_info).await;
    let repo = state.create_repo(viewer);
    let request = gql_request.into_inner().data(repo);

    state.schema.execute(request).await.into()
}

@sunli829
Copy link
Collaborator

You need to update these two dependencies:

async-graphql = { git = "https://github.com/async-graphql/async-graphql", branch = "master" }
async-graphql-actix-web = { git = "https://github.com/async-graphql/async-graphql", branch = "master" }

@emwalker
Copy link
Author

That got things working. After switching to master, I can confirm that the field arguments on the different types in the union are now allowed to differ. Thanks for putting time into this.

@sunli829
Copy link
Collaborator

Released in 4.0.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants