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

Add MongoDB test application #7

Merged
merged 21 commits into from
May 6, 2022
Merged

Add MongoDB test application #7

merged 21 commits into from
May 6, 2022

Conversation

AlexLLKong
Copy link
Collaborator

image

Test application to demonstrate rudimentary CRUD operations on locally hosted node application and node instance.

There isn't much error handling.

@nok-ko
Copy link
Collaborator

nok-ko commented May 4, 2022

I've included yargs as a dependency for command-line argument parsing, and made the app automatically try to connect to the MongoDB instance at mongodb://localhost:27017, using the database called test. If it fails, it tells you about it:

cli_demo.mp4

@nok-ko
Copy link
Collaborator

nok-ko commented May 4, 2022

@AlexLLKong I think it's best to only open the connection once, and close it at the end of the app's lifetime – seems like that's the best practice.

I've made that change in the latest commit.

@nok-ko
Copy link
Collaborator

nok-ko commented May 4, 2022

I've added some npm scripts:

  • npm start to start the server, using your local MongoDB instance
  • npm prod to do the same using the instance at db.nokko.me
  • npm test to run tests – currently none are defined, but I would like to use Jest for this purpose in the future.

image

@nok-ko
Copy link
Collaborator

nok-ko commented May 4, 2022

Don't have proper tests yet, but everything works circa the latest commit:

image

TBD:

  • Add code that initializes the database with all the collections we need, if they don't exist yet
    • (Need schema from Alex)
  • Make the app log in to the Mongo instance with the shakeguard user+pass
    • Store the password secretly somehow – investigate the correct way to do this
  • Make a common cleanup() function that closes any open file handles/other resources in case the process needs to exit mid-run. Not such a big concern, but would be nice to not corrupt anything when exiting "nicely."
  • Make the existing Express routes respond with semantic error codes when failing – currently all responses are 200 OK and status is indicated in the response body. 👀

@AlexLLKong
Copy link
Collaborator Author

image
image

Using Insomnia to test endpoints. Postman was giving me issues and I'm too lazy to use curl.

Next up will be logout followed by signup.

@AlexLLKong
Copy link
Collaborator Author

image

Logout "works" but I wasn't able to work out a test for when it fails. Which probably means I'm not doing it right.

Copy link
Collaborator

@nok-ko nok-ko left a comment

Choose a reason for hiding this comment

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

Good stuff! Are the Insomnia tests only on your local machine? Can you make them available to the rest of the team through the repo somehow?

@@ -82,6 +84,15 @@ try {


app.use(express.json());
app.use(session(
{
secret: "shhhh...secret",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, still need to figure out how to keep secrets properly. Hng.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Going to write a proper secret-grabbing thing after #9 is merged.

mongoDbTest.js Outdated
res.status(401).send("Invalid email");
} else if (results.length > 1) {
// Many users with same email
// TODO: This is bad and should let someone know
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth keeping a log file for this kind of thing.

mongoDbTest.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@nok-ko nok-ko left a comment

Choose a reason for hiding this comment

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

Not sure what error code this should be, not sure when the error case would be hit… not sure of much of anything, but this shouldn't be a 400. Minor quibble, really – don't worry.

mongoDbTest.js Outdated Show resolved Hide resolved
@nok-ko nok-ko mentioned this pull request May 4, 2022
12 tasks
@nok-ko nok-ko linked an issue May 4, 2022 that may be closed by this pull request
11 tasks
@AlexLLKong
Copy link
Collaborator Author

image
image

Making a user account works. No sanitization at the moment though

mongoDbTest.js Outdated
const date = new Date().toISOString();
const achievements = req.body.achievements;
const admin = false;
const kit = req.body.kit;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the kit be created without any involvement from the code calling into the API?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is a user able to start building a kit before signing up? If not, then I guess that can be changed to a default skeleton object

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the expected value for the kit property in a user's document? I think this might just be me being confused, I should take a look at the schema.

headers: {
'Content-Type' : 'application/json'
},
body: JSON.stringify({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess whether we send the request data in the body or the query parameters doesn't really matter, but I thought we were going to use query parameters. 😅

If you don't want to change it, just say so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, actually, don't change this! It makes sense for this to be in a body parameter, at least from reading this blog post.

General rule seems to be:

  • Path parameters for getting a specific thing out of the API
    • For example: GET /people/katy_p
  • Query parameters for filtering, or otherwise limiting the output of a query for lots of things
    • For example: GET /people/katy_p/friends?limit=10&nameLike=Alex
  • Body parameters for supplying data needed to fulfill a request to the endpoint
    • For example: POST /create-cat, body: {name:"Fluffy"}

@nok-ko
Copy link
Collaborator

nok-ko commented May 5, 2022

Once #9 is merged, we should rebase this branch on top of main and make sure everything still works.

@AlexLLKong AlexLLKong requested a review from nok-ko May 5, 2022 21:18
Copy link
Collaborator

@nok-ko nok-ko left a comment

Choose a reason for hiding this comment

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

Mostly good, take a look at my suggestions though.

mongoDbTest.js Outdated Show resolved Hide resolved
mongoDbTest.js Outdated Show resolved Hide resolved
mongoDbTest.js Outdated Show resolved Hide resolved
mongoDbTest.js Outdated Show resolved Hide resolved
mongoDbTest.js Show resolved Hide resolved
mongoDbTest.js Outdated Show resolved Hide resolved
mongoDbTest.js Show resolved Hide resolved
mongoDbTest.js Outdated Show resolved Hide resolved
mongoDbTest.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@nok-ko
Copy link
Collaborator

nok-ko commented May 6, 2022

AT LAST! Latest commit should super-duper fix #8. @AlexLLKong if you could make up a random session secret on the db.nokko.me server and use my new shakeguardSecrets.js module to load it in, that'd be great. (Secrets are in /home/shakeguard/.secrets if you don't recall.)

Here:

app.use(session(
	{
	  secret: "shhhh...secret", // load it in from the secrets object instead!
	  name: "ShakeGuardSessionID",
	  resave: false,
	  // create a unique identifier for that client
	  saveUninitialized: true
	})
  );

Copy link
Collaborator

@nok-ko nok-ko left a comment

Choose a reason for hiding this comment

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

Right, that seems to fix everything I brought up, excellent work. Maybe do a nice thing for our front-end folks and document what all the messages mean somewhere – API.md or something? Maybe there's a better way, or maybe it's a bit early to put effort into that kind of thing, but would be nice. 👍

Copy link
Collaborator

@nok-ko nok-ko left a comment

Choose a reason for hiding this comment

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

Whoops.

package.json Show resolved Hide resolved
@AlexLLKong AlexLLKong marked this pull request as ready for review May 6, 2022 22:02
@AlexLLKong AlexLLKong merged commit 0044dbc into dev May 6, 2022
nok-ko pushed a commit that referenced this pull request May 20, 2022
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 API between Front- and Back-end code
2 participants