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

Disallowing empty parens (introduced in v.2.5.12) is a breaking change #309

Closed
sonya opened this issue Jun 21, 2024 · 1 comment
Closed

Comments

@sonya
Copy link

sonya commented Jun 21, 2024

What happened?

In v2.5.12, a change (#293) was made to return a parse error if the user supplied a GraphQL request where the query or root was followed by an empty argument list (). Empty argument lists were previously allowed up to v2.5.11.

Although the change was made to comply with the GraphQL spec and match implementation of other libraries, it breaks all existing clients who have been supplying empty argument lists. This makes upgrading very difficult for servers with diverse client traffic.

What did you expect?

Queries performed on v2.5.11 will continue to work after upgrading to v2.5.12.

Minimal graphql.schema and models to reproduce

type Query {
  FooList(id: ID) [Foo!]
}

type Foo {
  id: ID
}
type Foo struct {
  ID *string `json:"id"`
}

Queries that look like this work with v2.5.11

query Foo {
  FooList() {
    id
  }
}

With v2.5.12, queries with an empty argument list return

{
  "data": null,
  "errors": [
    {
      "extensions": {
        "code": "GRAPHQL_PARSE_FAILED"
      }
      "locaions": [
        {
          "column": 12,
          "line": 1
        }
      ],
      "message": "expected at least one definition, found )"
    }
  ]
}

versions

github.com/vektah/gqlparser/v2 v2.5.12
go version go1.22.2 darwin/arm64

@StevenACoffman
Copy link
Collaborator

You are entirely correct that this is a behavior change, and I regret the extra work this may have caused you and others. This library has not easily exposed configuration settings to allow people to opt-in to behavioral changes.

As a result, I am not able to guarantee preserving the backward compatibility of gqlparser for past, existing, or future bugs. Additionally, maintaining compatibility to an evolving GraphQL spec may also sometimes require breaking backward compatibility. The Go language itself has this backward compatibility guarantee:

Although we expect that the vast majority of programs will maintain this compatibility over time, it is impossible to guarantee that no future change will break any program. This document is an attempt to set expectations for the compatibility of Go 1 software in the future. There are a number of ways in which a program that compiles and runs today may fail to do so after a future point release. They are all unlikely but worth recording.

  • Security. A security issue in the specification or implementation may come to light whose resolution requires breaking compatibility. We reserve the right to address such security issues.
  • Unspecified behavior. The Go specification tries to be explicit about most properties of the language, but there are some aspects that are undefined. Programs that depend on such unspecified behavior may break in future releases.
  • Specification errors. If it becomes necessary to address an inconsistency or incompleteness in the specification, resolving the issue could affect the meaning or legality of existing programs. We reserve the right to address such issues, including updating the implementations. Except for security issues, no incompatible changes to the specification would be made.
  • Bugs. If a compiler or library has a bug that violates the specification, a program that depends on the buggy behavior may break if the bug is fixed. We reserve the right to fix such bugs.
  • Struct literals. For the addition of features in later point releases, it may be necessary to add fields to exported structs in the API. Code that uses unkeyed struct literals (such as pkg.T{3, "x"}) to create values of these types would fail to compile after such a change. However, code that uses keyed literals (pkg.T{A: 3, B: "x"}) will continue to compile after such a change. We will update such data structures in a way that allows keyed struct literals to remain compatible, although unkeyed literals may fail to compile. (There are also more intricate cases involving nested data structures or interfaces, but they have the same resolution.) We therefore recommend that composite literals whose type is defined in a separate package should use the keyed notation.
  • Methods. As with struct fields, it may be necessary to add methods to types. Under some circumstances, such as when the type is embedded in a struct along with another type, the addition of the new method may break the struct by creating a conflict with an existing method of the other embedded type. We cannot protect against this rare case and do not guarantee compatibility should it arise.
  • Dot imports. If a program imports a standard package using import . "path", additional names defined in the imported package in future releases may conflict with other names defined in the program. We do not recommend the use of import . outside of tests, and using it may cause a program to fail to compile in future releases.

The GraphQL documentation gives these examples showing omitting parenthesis for queries when there are no arguments:

type Query {
  books: [Book]
  authors: [Author]
}

And the spec states that if you try to add empty parenthesis like:

query GetSomething() { # <-- empty arguments list here
  user() { # <-- empty argument list here
    name
  }
}

There should be a validation error in Line 1 and Line 2 each because in GraphQL, Arguments must contain at least one Argument. In GraphQL spec, the notation [list] means one or more -- see spec definition

Note that the entire "Arguments" portion is optional in both contexts, therefore query GetSomething { ... } is allowed and so is user { ... }, but if the braces () are present they must contain something.

The graphql-js reference implementation reports errors on both cases correctly.

This parser used to allow empty parenthesis, but we are trying to maintain spec compliance. The change was made here:
#293

I again regret the additional work this forced upon you and others, but I hope you can understand my reasoning and motivation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants