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

introspect drops comment new lines which breaks SDL parsing #728

Closed
stephenh opened this issue Aug 12, 2021 · 4 comments · Fixed by #742
Closed

introspect drops comment new lines which breaks SDL parsing #728

stephenh opened this issue Aug 12, 2021 · 4 comments · Fixed by #742
Assignees
Labels

Comments

@stephenh
Copy link

We had an SDL file on our server-side that looked like:

"""
I.e. "an example value"
"""
type Foo { ... }

However rover graph introspect turned this into:

"""I.e. "an example value""""
type Foo { ... }

Which then causes graphql-js's lexer.js to blow up on "unterminated string", i.e. it doesn't like the quadruple quotes.

Perhaps this is really a bug in graphql-js's parsing? Being some lazy and filing it here because we found it via rover introspect changing the SDL (or maybe that is also happening inside of graphql-js, I'm not sure).

I can re-file this with the graphql-js project if that's where the error really is. Thanks!

@stephenh stephenh added bug 🐞 triage issues and PRs that need to be triaged labels Aug 12, 2021
@ericf89
Copy link

ericf89 commented Aug 16, 2021

Seeing this too with a description that terminates in a double quote. 🤔

It's inlined, and makes it so this schema can't be parsed by a rover graph check command later in our pipeline
image

Interestingly, whatever process that publishes the sdl to apollo studio on startup seems to be sending/parsing this correctly
image

On the server being introspected, we're running apollo-server@3.1.2 and graphql-js@15.5.1

graphql/graphql-js#1205 in graphql-js seems related but already solved/closed

@lrlna
Copy link
Member

lrlna commented Aug 16, 2021

@stephenh @ericf89 thank you for reporting this. That definitely seems like a bug in our encoder that ought to be fixed!

@lrlna lrlna removed the triage issues and PRs that need to be triaged label Aug 16, 2021
@lrlna lrlna self-assigned this Aug 16, 2021
lrlna added a commit that referenced this issue Aug 17, 2021
This commit introduces `Description` helper to encode top-level, field-level and input-level descriptions. This helps unify some of the description logic we've had across multiple files.

This commit also checks whether a description string is a block string character or a string character which then determines whether description needs to be multilined.

Fixes #728.
EverlastingBugstopper added a commit that referenced this issue Aug 17, 2021
This commit introduces `Description` helper to encode top-level, field-level and input-level descriptions. This helps unify some of the description logic we've had across multiple files.

This commit also checks whether a description string is a block string character or a string character which then determines whether description needs to be multilined.

Fixes #728.

Co-authored-by: Avery Harnish <avery@apollographql.com>
@lrlna
Copy link
Member

lrlna commented Aug 18, 2021

@stephenh @ericf89 the fix will be part of our next release. I can ping you once it's out.

@ericf89
Copy link

ericf89 commented Aug 18, 2021

Thanks for the quick turn around @lrlna!

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

Successfully merging a pull request may close this issue.

4 participants