Exercise 5 #7

RespectableRuessel · 2020-12-23T15:55:48Z

Pull request for exercise 5.
Replaced InMemoryDatasource with a Neo4j database. Now, results of mutations will be persistent.

Implemented connection to Neo4j
Refactored mutations and queries
Implemented tests for login and signup

Link to the review

…nd-Frameworks/no-js-pls into exercise-4

…velopment

FrauMauz

schon sehr komplex mittlerweile... man sieht, dass viel Arbeit drin steckt !

FrauMauz · 2021-01-05T15:30:30Z

backend/README.md

+We wanted to broaden our horizon by using a graph database because we
+only used RDBM systems.
+Also the Neo4j `cypher` language looked interesting compared to regular SQL.
+And we wanted to support local businesses in these dark times.


why you choosed Neo4J - short but totally ok and honest :) one 🌟 for you

FrauMauz · 2021-01-05T15:31:23Z

backend/README.md

+```
+$ npm run dev
+```
+but you need to start the Neo4j database service first.


nice and detailed instructions + scenario architecture choosen ...1x 🌟 for you

FrauMauz · 2021-01-05T15:32:43Z

backend/jest.config.js

  preset: 'ts-jest',
  testEnvironment: 'node',
-};
+  testTimeout: 10000


how did it work with the timeout of 10000? did you try other testTimeouts?

I am also not sure why this is timeout is important? Did it fail without it?

FrauMauz · 2021-01-05T15:34:19Z

backend/.gitignore

 lib
+.env
+
+dev.env


ahhh the environment variables are added

instead of ignore the .env , you could use git-crypt

FrauMauz · 2021-01-05T15:35:32Z

backend/package.json

+    "dev": "dotenv -e dev.env -- nodemon -e ts src/index.ts",
    "lint": "eslint . --ext .ts",
-    "test": "jest"
+    "test": "dotenv -e test.env -- jest"


environment variables for testing 👍

FrauMauz · 2021-01-05T16:12:20Z

backend/src/test/post.spec.ts

+import {NEO4J_PASSWORD, NEO4J_URI, NEO4J_USER} from "../config/env.config";
+
+const drv = driver(NEO4J_URI, auth.basic(NEO4J_USER, NEO4J_PASSWORD));
+const db: Neo4JDatasource = new Neo4JDatasource(drv);


new Neo4JDatasource(drv); !!! good

FrauMauz · 2021-01-05T16:13:23Z

backend/src/test/post.spec.ts

+        `;
+
+        const data = await mutate({ mutation: SIGNUP_USER, variables: {
+                name:"Guy", email:"GUY@GUY.de", password:"123456789"


for testing.. ok

FrauMauz · 2021-01-05T16:14:57Z

backend/test.env

+NEO4J_URI=bolt://18.204.8.8:33335
+NEO4J_USER=neo4j
+NEO4J_PASSWORD=twos-pages-diagnosis
+NEO4J_DATABASE=


test Variables

⭐ For requesting a review and reviewing another team's PR.
You asked review from us.. and we are sure that you will review us as soon as you have our request. so this star will be yours for sure...

FrauMauz · 2021-01-05T16:16:22Z

backend/src/types/context.ts

+    dataSources: {
+        databaseAPI: DatasourceAPI
+    };
 }


Interface for db ... good

FrauMauz · 2021-01-05T16:18:06Z

backend/src/server.ts

+        id: '789',
+        name: 'Robin',
+        email: 'test3@test.de',
+        password: generateSHA512Hash('test123')


some test users with their data and hash passwords...

medizinmensch

Hi Team!

I really like your thorough Readme, this made it much easier for me to review it! Thanks!

It looks like you have flaky tests. If I do npm run test, I get different errors every time - sometimes also no error.
This is because you're running your test files in parallel - jest is doing that by default. I recommend adding a --runInBand here to change that.

"test": "dotenv -e test.env -- jest --runInBand"

Otherwise a change in your database from one file affects the other.
Once that was solved it looked quite smoothly.

One thing I wonder though... Are you the group that didn't use a locally hosted db at all? So did you only use a hosted/sandbox one online? Is it possible that your test.env contains actual connection data? 😅
As you can guess, this is not how it's supposed to be.
I can only emphasize that starting one with docker is quite easy once you got docker installed. For instance with:

docker run -p 7474:7474 -p 7687:7687 --env=NEO4J_AUTH=NEO4J_USERNAME/NEO4J_PASSWORD neo4j:latest

❌ So feel free to clarify, but it looks like I can't give you a point here:

⭐ For not committing sensitive secrets (e.g. API keys) unencrypted to the repository.

❌ Secondly, I do not see that you deny your requests to your subschema. It would be easiest to set a default deny rule in your main schema so that everything not in that schema gets denied.

⭐ You deny the client to call mutations and queries of your subschema directly.

To conclude:

⭐ For choosing a scenario and writing installation instructions in README.md.
⭐ For explaining WHY you have chosen the scenario in the README.md.
⭐ ⭐ Data created in mutations are persisted and your queries and mutations still work as expected.
⭐ ⭐ Your software tests are free of side effects.
⭐ Your database is never left in an invalid state.
⭐ For requesting a review and reviewing another team's PR.

That makes 8/10 ⭐

medizinmensch · 2021-01-11T19:27:58Z

backend/src/database/Neo4JDatasource.ts

+            tx.run(
+                "MATCH (u:User) WHERE u.id =~ $id RETURN u {.*}", {id: id}
+            ).then(result => {
+                user = result.records[0].get('u')


It's just for fetching the first element of the returned results.

medizinmensch · 2021-01-11T19:55:00Z

backend/jest.config.js

  preset: 'ts-jest',
  testEnvironment: 'node',
-};
+  testTimeout: 10000


I am also not sure why this is timeout is important? Did it fail without it?

RespectableRuessel · 2021-01-13T13:29:14Z

Hi Team!

I really like your thorough Readme, this made it much easier for me to review it! Thanks!

It looks like you have flaky tests. If I do npm run test, I get different errors every time - sometimes also no error.
This is because you're running your test files in parallel - jest is doing that by default. I recommend adding a --runInBand here to change that.
"test": "dotenv -e test.env -- jest --runInBand"
Otherwise a change in your database from one file affects the other.
Once that was solved it looked quite smoothly.

One thing I wonder though... Are you the group that didn't use a locally hosted db at all? So did you only use a hosted/sandbox one online? Is it possible that your test.env contains actual connection data? 😅
As you can guess, this is not how it's supposed to be.
I can only emphasize that starting one with docker is quite easy once you got docker installed. For instance with:
docker run -p 7474:7474 -p 7687:7687 --env=NEO4J_AUTH=NEO4J_USERNAME/NEO4J_PASSWORD neo4j:latest
❌ So feel free to clarify, but it looks like I can't give you a point here:

@medizinmensch Here is my answer to our Neo4j journey of problems:

The problem is that it seems that Neo4j can not be tested locally on your normal machine. I've written a Moodle Post about it because I looked up the issue and nobody who worked with Neo4j had an actual answer. Robert suggested to mock out the session but then you're miss testing the connection and exception handling when your database fails which is not desirable.

The only option we found (without docker) was the sandbox that was presented in the lecture. The problem with docker is that I (for example) cannot use docker because it requires "Hyper-V and Containers Windows features must be enabled" . This is not possible for me because I am using a different hypervisor which is not compatible with Hyper-V.

Unfortunately Neo4j's architecture design is bad for using multiple instances because it runs an own service that (at least in free mode) cannot automatically setup a second database for testing.
There is a in-memory API but it is only accessible through the Java library. Also running a local instance is hard because the god-damn paths include a GUID which is different between users that leads to the problem that a configuration file will not work with other users and on the CI because the paths to the Neo4j file differ.

This was the original post:

Überall steht, dass das leider nicht "einfach" möglich ist [1]. Für die Java / C# Anbindung gibt es die ImpermanentGraphDatabase [2, 3], die es aber nicht für JS oder andere Sprachen implementiert ist.

Empfohlen wird das "Mocken" der Aufrufe, welches uns aber wieder nur zur InMemoryDatasource führt, die wir ja nicht nutzen sollen. Die paar "Lösungen" die es gibt scheinen zusätzlich eine Bearbeitung der neo4j config zu benötigen [4], welches auf der CI kompliziert wird, da die lokalen neo4j Pfade nicht übereinstimmen.

[1] https://stackoverflow.com/questions/49348839/how-to-unit-test-neo4j-in-python

[2] neo4j/neo4j-javascript-driver#211

[3] https://neo4j.com/docs/java-reference/current/java-embedded/include-neo4j/

[4] https://stackoverflow.com/questions/45784232/how-to-create-new-database-in-neo4j

+++ EDIT:

I am also not sure why this is timeout is important? Did it fail without it?

This is because the sandbox is sometimes slow which leads to failed tests.

Testing got different errors/no errors each time they ran, because they ran in parallel. Hence they are tested in sequence now.

= and others added 13 commits November 25, 2020 15:43

Merge branch 'exercise-4' of https://github.com/Systems-Development-a…

276a0cc

…nd-Frameworks/no-js-pls into exercise-4

Added authentification and authorization with graphql-shield

ebf6106

Implemented authorization and authentification

68a5a6d

Seperated environment files: test.dev for testing and dev.test for de…

7a0f5bc

…velopment

Ignore development environment file

c926f71

Added type to dataSources

1895761

Started implementation of neo4j datasource

a931aef

Refactored cipher queries

04394b9

Refactored test to use neo4j implementation

efde44f

Fix no comment in empty function

ebb72ff

Added README with installation instructions and changed test server

b523918

Fixed small format mistake

060c5e1

Readded the requirements section

80077d0

RespectableRuessel requested review from a team and FrauMauz and removed request for FrauMauz January 4, 2021 18:25

FrauMauz approved these changes Jan 5, 2021

View reviewed changes

Merge branch 'main' into exercise-5

0f973ab

RespectableRuessel requested a review from a team January 9, 2021 19:25

FrauMauz mentioned this pull request Jan 10, 2021

Exercise5 Systems-Development-and-Frameworks/dirty_thirties#8

Merged

medizinmensch approved these changes Jan 11, 2021

View reviewed changes

TheKilljoy added 2 commits January 13, 2021 15:59

- Fixed: Flaky Tests as suggested by medizinmensch

a0b54c2

Testing got different errors/no errors each time they ran, because they ran in parallel. Hence they are tested in sequence now.

Update test.env

be85bf3

RespectableRuessel merged commit 44c7bb7 into main Jan 13, 2021

               lib
+              .env
+              dev.env

Exercise 5 #7

Exercise 5 #7

Uh oh!

Conversation

RespectableRuessel commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrauMauz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medizinmensch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RespectableRuessel commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RespectableRuessel commented Dec 23, 2020 •

edited

Loading

medizinmensch left a comment •

edited

Loading

RespectableRuessel commented Jan 13, 2021 •

edited

Loading