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.

Sign up for GitHub

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

Jump to bottom

Reserved keywords in column references break parser (fix #3597) #3927

Merged
merged 4 commits into from
Apr 2, 2020
Merged

Reserved keywords in column references break parser (fix #3597) #3927

merged 4 commits into from
Apr 2, 2020

Conversation

sordina
Copy link
Contributor

@sordina sordina commented Feb 24, 2020 •
edited
Loading

Description

This pull-request is intended to fix issues related to #3597 "Field naming starting with reserved keywords breaks updates ".

The bug is located in the parser, therefore the fix is as follows:

  • Fix bug in parser
  • Add tests to engine
  • Update project.cabal to reference new version of the parser
source-repository-package
  type: git
  location: https://github.com/hasura/graphql-parser-hs.git
  tag: 1380495a7b3269b70a7ab3081d745a5f54171a9c

Affected components

  • Server
  • Tests

Related Issues

Solution and Design

The solution was to add more nuance to the parser as can be seen in the related parser pull request: hasura/graphql-parser-hs#21

This is however only a temporary fix as the parser should probably be split into lexer/parser phases to much more effectively express the tokenisation of such identities as caused the problem here. Also this would make the code a lot more readable as it was quite difficult to narrow down the bug for a new contributor to the codebase (me!).

Steps to test and verify

Run pytest against the new and old versions to see the fix addressed:

pytest ... -vv test_graphql_mutations.py::TestGraphqlInsertNullPrefixedColumnOnConflict

You can also reproduce manually in the console if preferred.

Catalog upgrade

Does this PR change Hasura Catalog version?

  • No

Metadata

Does this PR add a new Metadata feature?

  • No

GraphQL

  • No new GraphQL schema is generated

Breaking changes

  • No Breaking changes

@netlify
Copy link

netlify bot commented Feb 24, 2020 •
edited
Loading

Deploy preview for hasura-docs ready!

Built with commit fc0d73d

https://deploy-preview-3927--hasura-docs.netlify.com

@sordina sordina mentioned this pull request Feb 24, 2020
Closed
@hasura-bot
Copy link
Contributor

Review app for commit 5b4fdbd deployed to Heroku: https://hge-ci-pull-3927.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3927-5b4fdbd9

@sordina sordina assigned paf31 and 0x777 and unassigned paf31 and 0x777 Feb 25, 2020
@sordina
Copy link
Contributor Author

sordina commented Feb 25, 2020

@lexi-lambda do you want to review, or should I assign to someone else?

@lexi-lambda
Copy link
Contributor

@0x777 Has been doing most of the server reviews temporarily while I’ve been working on this refactoring project, so he’s probably the one to review this.

@0x777 0x777 requested a review from paf31 February 25, 2020 06:12
paf31
paf31 reviewed Feb 25, 2020
View reviewed changes
server/tests-py/test_graphql_mutations.py
@@ -184,6 +184,18 @@ def dir(cls):
return "queries/graphql_mutation/insert/constraints"


@pytest.mark.parametrize("transport", ['http', 'websocket'])
@usefixtures('per_class_tests_db_state')
class TestGraphqlInsertNullPrefixedColumnOnConflict:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could probably have been added to one of the existing insert classes, since we can setup multiple unrelated tables for different tests in one class setup file. I don't think we need to change it now though.

paf31
paf31 previously approved these changes Feb 25, 2020
View reviewed changes
@sordina sordina changed the title WIP: Reserved keywords in column references break parser (fix #3597) Reserved keywords in column references break parser (fix #3597) Mar 5, 2020
@sordina
Copy link
Contributor Author

sordina commented Mar 5, 2020

Hi @lexi-lambda, is there anything you'd like me to do on this one before it can be reviewed for merge?

@hasura-bot
Copy link
Contributor

Review app for commit b181a42 deployed to Heroku: https://hge-ci-pull-3927.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3927-b181a42a

@hasura-bot
Copy link
Contributor

Review app for commit 8ca72dd deployed to Heroku: https://hge-ci-pull-3927.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3927-8ca72ddf

@nizar-m nizar-m force-pushed the lyndon/issue-3597-null-column-prefixes branch from 8ca72dd to 9694633 Compare March 5, 2020 15:00
@hasura-bot
Copy link
Contributor

Review app for commit fc0d73d deployed to Heroku: https://hge-ci-pull-3927.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3927-fc0d73d3

@hasura-bot
Copy link
Contributor

Review app for commit 3aa9c32 deployed to Heroku: https://hge-ci-pull-3927.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3927-3aa9c32e

@hasura-bot
Copy link
Contributor

Review app for commit e019cac deployed to Heroku: https://hge-ci-pull-3927.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3927-e019cacb

@lexi-lambda lexi-lambda force-pushed the lyndon/issue-3597-null-column-prefixes branch from bc45925 to 849250f Compare April 1, 2020 22:03
@sordina sordina requested review from a team as code owners April 1, 2020 22:03
@hasura-bot
Copy link
Contributor

Review app for commit 849250f deployed to Heroku: https://hge-ci-pull-3927.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3927-849250f1

marionschleifer
marionschleifer approved these changes Apr 2, 2020
View reviewed changes
Copy link
Contributor

@marionschleifer marionschleifer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog approved.

@lexi-lambda lexi-lambda merged commit 85b7c55 into hasura:master Apr 2, 2020
@hasura-bot
Copy link
Contributor

Review app https://hge-ci-pull-3927.herokuapp.com is deleted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lexi-lambda lexi-lambda lexi-lambda approved these changes

@marionschleifer marionschleifer marionschleifer approved these changes

@paf31 paf31 paf31 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sordina @hasura-bot @lexi-lambda @paf31 @marionschleifer @0x777