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

[master < T0869] Implement GraphQL compatibility #1018

Merged
merged 33 commits into from
Jul 31, 2023

Conversation

gvolfing
Copy link
Contributor

@gvolfing gvolfing commented Jun 16, 2023

Closes #869

Initial version of an e2e testing framework that can be used to run
graphql queries end to end with memgraph. For the 'create' graphql query
to work, the awesome memgraph function, randomUUID, had to be added.
Further more a temporary query module is loaded in the workload to
emulate the initial query to the database. This should be changed later.
@gvolfing gvolfing mentioned this pull request May 18, 2023
1 task
The incoming queries should be inspected and certain keywords should be
swapped into memgraph specific ones. As this is modifying the query
string itself, the interpreter should not be modified because of this.
@gvolfing gvolfing self-assigned this Jun 22, 2023
@gitbuda gitbuda added this to the mg-v2.10.0 milestone Jun 22, 2023
The added procedures can not be named mgp.*, that way they won't show up
in the module registry. The path to the GraphQL library specifying js
files has been changed.
The util.validate procedure could not be added as a simple query
procedure, because the procedure itself does not return anything, which
is something we do not support atm, and we do not have the ability to
evaluate expressions from query procedures. They it has been solved, is
that the during parsing, we check if the given CALL procedure does not
YIELD, and if the fully qualified name of the procedure matches
util.validate and based on this, we can instantiate an appropriate
cursur and put that into the pull-chain instead of the regular
CallProcedureCursor. On top of this '_' is sometimes generated as a
variable name we did not support. Support for that has been added as
well.
Add config js files to be able to test graphql capabilites against basic
CRUD operations. Add a setup file that is installing the dependencies.
src/memgraph.cpp Outdated Show resolved Hide resolved
gvolfing and others added 7 commits July 20, 2023 16:10
Add tests to be able to test the basic CRUD functionality of memgraph
when running it against GraphQL.
Instead of hardcoding the mappings in memory, use a json config file to
read in the mappings between procedure names. Add flag
'callable_mappings_path' to be able to specify the path to said config
file. Make names more generic.
Copy link
Contributor

@andrejtonev andrejtonev left a comment

Choose a reason for hiding this comment

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

Looks OK, main issue I have is that the CALL without YIELD logic should be generic and not implemented to work just for the validate procedure. Other than that, it's mostly just some small comments.
I also think we are calling FindAlias twice when we could call it just once.

init Show resolved Hide resolved
src/query/frontend/ast/ast.hpp Outdated Show resolved Hide resolved
src/query/frontend/ast/cypher_main_visitor.cpp Outdated Show resolved Hide resolved
src/query/frontend/ast/cypher_main_visitor.cpp Outdated Show resolved Hide resolved
src/query/plan/operator.cpp Outdated Show resolved Hide resolved
src/query/procedure/callable_alias_mapper.cpp Outdated Show resolved Hide resolved
src/query/procedure/callable_alias_mapper.cpp Outdated Show resolved Hide resolved
src/query/procedure/callable_alias_mapper.cpp Outdated Show resolved Hide resolved
src/query/procedure/module.cpp Show resolved Hide resolved
@gvolfing gvolfing marked this pull request as ready for review July 25, 2023 10:55
Copy link
Contributor

@as51340 as51340 left a comment

Choose a reason for hiding this comment

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

Nice job gabor! I added some comments, let me know what you think

init Show resolved Hide resolved
src/query/plan/operator.cpp Show resolved Hide resolved
src/query/plan/operator.cpp Outdated Show resolved Hide resolved
src/query/plan/operator.cpp Outdated Show resolved Hide resolved
tests/e2e/graphql/graphql_server.py Outdated Show resolved Hide resolved
tests/e2e/graphql/graphql_crud.py Outdated Show resolved Hide resolved
tests/e2e/graphql/graphql_crud.py Outdated Show resolved Hide resolved
tests/setup.sh Show resolved Hide resolved
src/query/frontend/ast/cypher_main_visitor.cpp Outdated Show resolved Hide resolved
@memgraph memgraph deleted a comment from as51340 Jul 25, 2023
Copy link
Contributor

@as51340 as51340 left a comment

Choose a reason for hiding this comment

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

Comments about existing stuff and few smaller things

init Show resolved Hide resolved
src/query/plan/operator.cpp Outdated Show resolved Hide resolved
src/query/plan/operator.cpp Outdated Show resolved Hide resolved
src/query/procedure/callable_alias_mapper.cpp Outdated Show resolved Hide resolved
src/query/procedure/callable_alias_mapper.cpp Outdated Show resolved Hide resolved
tests/e2e/graphql/graphql_crud.py Outdated Show resolved Hide resolved
tests/e2e/graphql/graphql_crud.py Outdated Show resolved Hide resolved
Added additional sleepint to the startup process as it seems that other
parts of the testing setup are not finished initializing as the GraphQL
server itself. Reversed logic when reading the alias mapping config.
src/memgraph.cpp Outdated Show resolved Hide resolved
Make sure nothing runs on the dedicated port upon process startup, this
can be problematic if the process was stuck before, than the subsequent
process spawn attempts will be meaningless.
There was a slight linking issue with adding query realated objects,
like TypedValue to utils, so a templated class has been used instead.
A concepts that enforces the used functions of TypedValue has been
created and a Dummy mock version of it is being used in the unit tests.
Copy link
Contributor

@as51340 as51340 left a comment

Choose a reason for hiding this comment

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

I really like the refactoring job you made and have just 2-3 more questions/suggestions.

init Show resolved Hide resolved
tests/e2e/graphql/workloads.yaml Show resolved Hide resolved
src/utils/java_string_formatter.hpp Show resolved Hide resolved
@katarinasupe
Copy link
Contributor

Trying to run create mutation and get the following error message:
"message": "There is no procedure named 'dbms.components'."
The flag callable_mappings_path is set to /home/ubuntu/memgraph/build/settings-graphql.json and the content of that is:

{
"dbms.components": "mgps.components",
"apoc.util.validate": "mgps.validate"
}

What am I doing wrong here?

@gvolfing
Copy link
Contributor Author

@katarinasupe
We need another query module to be able to communicate with the GraphQL Server.
Take the one that already exist on memgraph/tests/e2e/graphql/temporary_procedures/mgps.py copy that somewhere and specify the path to its containing directory with the --query-modules-directory flag, that should load the query module on startup, so the alias mapping should find it when the GraphQL Library sends it through bolt protocol.

@katarinasupe
Copy link
Contributor

By running delete query, I got Error message: Query should either create or update something, or return results! when wanting to run MATCH (n)-[r]->(m) RETURN r, n, m after deletion. I tried running that query two times, and I did not succeed. Then I ran MATCH (n) RETURN n, got weird results in Lab and then I managed to run MATCH (n)-[r]->(m) RETURN r, n, m successfully. After running the delete, and doing other stuff in db, then running MATCH (n) RETURN n again, I randomly reproduce the same error.

Weird results in Lab (this is style related and style depends on Labels + properties so I would say it's that kind of issues, something doing with Node objects not being good):
Screenshot 2023-07-28 at 16 51 36

logs:

[2023-07-28 14:51:03.967] [memgraph_log] [debug] [Run] 'MATCH (this:`Post`)
WHERE this.content = $param0
DETACH DELETE this'
[2023-07-28 14:51:04.043] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:04.150] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:07.042] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:07.146] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:10.040] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:10.144] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:12.254] [memgraph_log] [debug] [Run] 'MATCH (n)-[r]->(m)'
[2023-07-28 14:51:12.255] [memgraph_log] [trace] Error message: Query should either create or update something, or return results!
[2023-07-28 14:51:12.318] [memgraph_log] [trace] Reset received
[2023-07-28 14:51:13.044] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:13.159] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:16.040] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:16.157] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:17.110] [memgraph_log] [debug] [Run] 'MATCH (n)-[r]->(m)'
[2023-07-28 14:51:17.110] [memgraph_log] [trace] Error message: Query should either create or update something, or return results!
[2023-07-28 14:51:17.162] [memgraph_log] [trace] Reset received
[2023-07-28 14:51:19.076] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:19.191] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:22.044] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:22.156] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:24.618] [memgraph_log] [debug] [Run] 'MATCH (n)

RETURN n'
[2023-07-28 14:51:25.042] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:25.147] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:28.041] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:28.146] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:31.054] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:31.169] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:34.049] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:34.155] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:37.041] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:37.145] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:40.044] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:40.181] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:43.043] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'
[2023-07-28 14:51:43.169] [memgraph_log] [debug] [Run] 'MATCH (APP_INTERNAL_EXEC_VAR) return COUNT(APP_INTERNAL_EXEC_VAR) as cnt'
[2023-07-28 14:51:45.331] [memgraph_log] [debug] [Run] 'MATCH (n)-[r]->(m)

RETURN n,r,m'
[2023-07-28 14:51:46.047] [memgraph_log] [debug] [Run] 'SHOW STORAGE INFO'

Here are the steps to reproduce:

  1. Load the dataset. Here is cypherl:
CREATE INDEX ON :__mg_vertex__(__mg_id__);
CREATE (:__mg_vertex__:`User` {__mg_id__: 0, `name`: "katarina", `id`: "06daddb7-1657-4e6a-8a7d-b11ec6d2698f"});
CREATE (:__mg_vertex__:`Post` {__mg_id__: 2, `id`: "69626a62-ac56-4681-bc52-af6f28e97ffb", `content`: "new hello world!"});
CREATE (:__mg_vertex__:`User` {__mg_id__: 3, `name`: "karmen", `id`: "938ab1fd-f70a-4026-933c-9c83a11a8738"});
CREATE (:__mg_vertex__:`User` {__mg_id__: 4, `name`: "ante", `id`: "f4d847df-8c97-4c16-bd06-1141010e2961"});
CREATE (:__mg_vertex__:`User` {__mg_id__: 5, `name`: "buda", `id`: "e79e2279-df6f-4d52-9742-c588b26897c0"});
CREATE (:__mg_vertex__:`Post` {__mg_id__: 7, `id`: "c5c27035-c075-4c07-a96d-9e9f0e1b8ca9", `content`: "Another post!"});
CREATE (:__mg_vertex__:`Post` {__mg_id__: 8, `content`: "New post!"});
MATCH (u:__mg_vertex__), (v:__mg_vertex__) WHERE u.__mg_id__ = 0 AND v.__mg_id__ = 2 CREATE (u)-[:`HAS_POST`]->(v);
MATCH (u:__mg_vertex__), (v:__mg_vertex__) WHERE u.__mg_id__ = 3 AND v.__mg_id__ = 8 CREATE (u)-[:`HAS_POST`]->(v);
MATCH (u:__mg_vertex__), (v:__mg_vertex__) WHERE u.__mg_id__ = 4 AND v.__mg_id__ = 7 CREATE (u)-[:`HAS_POST`]->(v);
DROP INDEX ON :__mg_vertex__(__mg_id__);
MATCH (u) REMOVE u:__mg_vertex__, u.__mg_id__;
  1. Run the following mutation:
mutation {
  deletePosts(where: {content: "New post!"}){
    nodesDeleted,
    relationshipsDeleted
  }
}
  1. Try running MATCH queries in db (I reproduced it with MATCH (n)-[r]->(m) RETURN r, n, m and MATCH (n) RETURN n

@katarinasupe
Copy link
Contributor

@gvolfing please ignore my bug report above. I used the older version of Lab (2.7.0) and hence experienced such issues since I was running with 'Run selected' which was not working well. When I updated to Lab 2.7.1, deletes are working as expected so far.

@katarinasupe
Copy link
Contributor

katarinasupe commented Jul 28, 2023

I tried out some basic queries and aggregation queries. I also tried to create, update and delete mutations. Let me know if something needs to be added. My test examples and dataset can be found in Notion under Testing.

src/memgraph.cpp Outdated Show resolved Hide resolved
@gitbuda gitbuda merged commit 210bea8 into master Jul 31, 2023
6 checks passed
@gitbuda gitbuda deleted the T0869-MG-implement-graphql-compatibility branch July 31, 2023 12:48
as51340 pushed a commit that referenced this pull request Nov 10, 2023
* Add callable mappings feature
* Implement mgps.validate (void procedure)
* Make '_' a valid variable name
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.

Implement GraphQL compatibility
5 participants