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

26 db migration, type safety, domain layer and insert project api #32

Closed
wants to merge 108 commits into from

Conversation

francisduvivier
Copy link
Collaborator

@francisduvivier francisduvivier commented Nov 26, 2024

Upload API PR

This PR is preparation for enabling app uploads on badgehub. For enabling this, a lot of stuff was changed as described below. A lot of stuff still has to be done as well, but this PR is feature par with what we currently have.

Related PR

badgeteam/badgehub-frontend#16

Done

db-migrate

installed db-migrate and added the new table structure as a migration

sql-tagged-template

all sql code is now using sql-tagged-templates which helps with readability, README.md was also updated for this

Insert Project

You can now insert a project, which will also create an empty version and metadata and this project can then be retrieved

Publish project

The publish patch api on a project will mark the latest version as published now

Write API only enabled in development mode

Since we don't have security on our api yet, I added code that makes sure that the upload api only works when the NODE_ENV is set to development

Auto run db-migrate:up on start

Also added project_statuses_on_badges table and dependencies table

Migrate sample data

  • Sample data was migrated by re-exporting the database with pg_dump

README.md was updated with instructions

Tested with frontend for feature parity compared to master

  • with filtering on category and device working

Kept changes to fronted minimal by keeping current http api almost the same

TODO

Consistent naming:

  • Is it apps or projects, is it devices or badges?

Make a full app upload and download api MVP that the badge can use

Single File upload

You should be able to upload a file and this file should be put in the db and stored on disk using it's hash as the filename, like git does it, so also with 2-letter subdirs ideally.
if the file path is /metadata.json, then data should be extracted and filled in on the app_metadata_jsons table

Single File download

Zip upload

When you upload a zip, file upload should be done for all the files in the zip (so also extracting metadata)
Q: should zip upload also do publish?
A: no, it should not, there is an api for that. optionally, we can make it a query param later

Zip download

(one/many) to many relations from the relations dir

  • The one that is really needed for project upload is the file to version relation, others can wait.
  • This will allow eg. finding all the collaborators on a project, but also all projects that a user has collaborated on...

Maybe change http status codes and message

  • currently, it will return 204 for the write api's, maybe that should be 200 OK?
  • if you do a publish but it is already published, maybe that should be a different status code?

Maybe improve SQL debugging

  • We currently don't see the sql queries printed, even if they fail, maybe we should add that.

Change it to make it clear that draft version is always unpublished

=> so publish should also create a new version

Maybe add more documention regarding the philosophy of the database structure, the file management, the git/metadata.json as source of truth and the port and adapter stuff.

src/__test__/populateDB.ts Fixed Show fixed Hide fixed
src/__test__/populateDB.ts Fixed Show fixed Hide fixed
@paulinevos
Copy link
Collaborator

Does this need to be rebased or does it actually have 102 commits?

.env.example Outdated
@@ -1,6 +1,7 @@
POSTGRES_HOST=db
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change this? Would need to be db on the docker network, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

check, changed back.

@francisduvivier francisduvivier force-pushed the 26-improve-type-safety-for-db-queries branch from 538c13f to 59ce14f Compare November 27, 2024 18:22
@francisduvivier
Copy link
Collaborator Author

Note:

  • I received a comment that this PR is too big for reviewing and I agree, so I am currently in the works of splitting it up.
  • I found that the populateDB.ts script is not updated yet, I'll still do that.

@francisduvivier francisduvivier marked this pull request as draft November 27, 2024 19:08
const name = projectSlugs[id]!;
const slug = name.toLowerCase();
const description = getDescription(name);
const userId = random(userCount);

Check failure

Code scanning / CodeQL

Insecure randomness High

This uses a cryptographically insecure random number generated at
Math.random()
in a security context.

Copilot Autofix AI 25 days ago

To fix the problem, we need to replace the use of Math.random() with a cryptographically secure random number generator. In Node.js, we can use the crypto.randomBytes function to generate secure random bytes and convert them to a number. This ensures that the generated random values are not predictable.

We will:

  1. Import the crypto module from Node.js.
  2. Replace the random function implementation to use crypto.randomBytes to generate secure random numbers.
Suggested changeset 1
src/setupPopulateDBApi.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/setupPopulateDBApi.ts b/src/setupPopulateDBApi.ts
--- a/src/setupPopulateDBApi.ts
+++ b/src/setupPopulateDBApi.ts
@@ -2,2 +2,3 @@
 import express, { Express, Router } from "express";
+import { randomBytes } from "crypto";
 import {
@@ -78,3 +79,5 @@
 function random(n: number) {
-  return Math.floor(Math.random() * n);
+  const randomBuffer = randomBytes(4);
+  const randomNumber = randomBuffer.readUInt32BE(0);
+  return randomNumber % n;
 }
EOF
@@ -2,2 +2,3 @@
import express, { Express, Router } from "express";
import { randomBytes } from "crypto";
import {
@@ -78,3 +79,5 @@
function random(n: number) {
return Math.floor(Math.random() * n);
const randomBuffer = randomBytes(4);
const randomNumber = randomBuffer.readUInt32BE(0);
return randomNumber % n;
}
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@francisduvivier
Copy link
Collaborator Author

I'm closing this PR and replacing it with a cleaner more split up version:

Part 1: #33
Part 2: #35
Part 3: #34

@francisduvivier francisduvivier deleted the 26-improve-type-safety-for-db-queries branch December 9, 2024 16:54
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.

Update DB to match proposed App Metadata Schema and Improve type safety
3 participants