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

Fixes escape character sequences that are not on the last attributes #590

Merged
merged 2 commits into from
Sep 19, 2018

Conversation

adkron
Copy link
Contributor

@adkron adkron commented Aug 1, 2018

Fixes #588

When running with attributes that have escape characters and
on the same line the order of the attributes matters for the outcome. If
the slashes appear in the last attribute, then everything parses
successfully. If the property appears earlier in the list, then an error
is returned instead.

The error:

  validation_errors: [
    %Absinthe.Phase.Error{
      extra: %{},
      locations: [%{column: 0, line: 1}],
      message: "illegal: \") { name } }",
      path: [],
      phase: Absinthe.Phase.Parse
    }
  ]

This change allows the attributes to come in any order.

Amos King @adkron amos@binarynoggin.com

When running with attributes that have escape characters and attributes
on the same line the order of the attributes matters for the outcome. If
the slashes appear in the last attribute then everything parses
successfully. If the attribute appears earlier in the list then an error
is returned instead.

The error:

~~~elixir
  validation_errors: [
    %Absinthe.Phase.Error{
      extra: %{},
      locations: [%{column: 0, line: 1}],
      message: "illegal: \") { name } }",
      path: [],
      phase: Absinthe.Phase.Parse
    }
  ]
~~~

This change allows the attributes to come in any order.

Amos King @adkron <amos@binarynoggin.com>
@metelik
Copy link

metelik commented Aug 1, 2018

i like the solution :) i would embrace the '' between '' in the message

@tapickell
Copy link

👍 Good catch.

@benwilson512
Copy link
Contributor

@bruce can you review this?

@benwilson512
Copy link
Contributor

I believe this may be obviated by our move to NimbleParsec in 1.5, @bruce are you able to confirm that?

@bruce bruce self-assigned this Sep 19, 2018
@bruce
Copy link
Contributor

bruce commented Sep 19, 2018

This tests fine on master; I'm going to merge. The lexer change will disappear with the lexer switch, but the tests will be handy.

@bruce bruce merged commit 28848fd into absinthe-graphql:master Sep 19, 2018
bruce added a commit that referenced this pull request Sep 19, 2018
@bruce
Copy link
Contributor

bruce commented Sep 19, 2018

Confirmed the switch to nimble_parsec fixes this issue as well; merged & tests pass in master and in #503.

Thanks for your work on this, @adkron; even though the fix was superseded, the tests are great to have around. Sorry for the delay in reviewing.

@adkron
Copy link
Contributor Author

adkron commented Sep 19, 2018 via email

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

Successfully merging this pull request may close these issues.

5 participants