MaxLeiter / MaxLeiter/Drift

[Proposal] (Server) Decouble DB from routes (from domain logic)

Open
#79 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
1.4k
Forks
58
PR merge metrics
No merged PRs in 30d

Description

I actually wanted to propose a slightly different change, and decouple the controller with the database.
Right now tests are very slow because they spool a whole DB, and any new changes will be very sequilize bound. Maybe it's because I don't really like sequalize, but I would like to have the domain logic (e.g.: hashing the password, or failing if there are no file) to live outside the class Post extends Model and leave the current Post class as a mere DTO to the database

Current implementation

Lets explore how one route would change. /posts/create is a big ofender:

// server/src/routes/posts.ts
posts.post(
	"/create",
	jwt,
	celebrate({                                 // ---- All good. This is a route responsability 🆗 
		body: {
			title: Joi.string().required(),
			files: Joi.any().required(),
			visibility: Joi.string()
				.custom(postVisibilitySchema, "valid visibility")
				.required(),
			userId: Joi.string().required(),
			password: Joi.string().optional(),
			//  expiresAt, allow to be null
			expiresAt: Joi.date().optional().allow(null, ""),
			parentId: Joi.string().optional().allow(null, "")
		}
	}),
	async (req, res, next) => {
		try {
			// check if all files have titles
			const files = req.body.files as File[]
			const fileTitles = files.map((file) => file.title)
			const missingTitles = fileTitles.filter((title) => title === "")
			if (missingTitles.length > 0) {    // ---- This is an invariant on all post->files. There should never be a post without titles 🙅 
				throw new Error("All files must have a title")
			}

			if (files.length === 0) {    // ---- This again is an invariant on all post->files. 🙅 
				throw new Error("You must submit at least one file")
			}

			let hashedPassword: string = ""
			if (req.body.visibility === "protected") {    // ---- This again is transformation of the Post creation; not the router🙅 
				hashedPassword = crypto
					.createHash("sha256")
					.update(req.body.password)
					.digest("hex")
			}

			const newPost = new Post({
				title: req.body.title,
				visibility: req.body.visibility,
				password: hashedPassword,
				expiresAt: req.body.expiresAt
			})

			await newPost.save()  // ---- Now we are crosing a lot of boundries, directly accesing the DB🙅 
			await newPost.$add("users", req.body.userId)
			const newFiles = await Promise.all(
				files.map(async (file) => {
					const html = getHtmlFromFile(file)
					const newFile = new File({
						title: file.title || "",
						content: file.content,
						sha: crypto
							.createHash("sha256")
							.update(file.content)
							.digest("hex")
							.toString(),
						html: html || "",
						userId: req.body.userId,
						postId: newPost.id
					})
					await newFile.save()
					return newFile
				})
			)

			await Promise.all(
				newFiles.map(async (file) => {
					await newPost.$add("files", file.id)  // ---- We have to manage timings on how sequalize like things to be stored🙅 
					await newPost.save()
				})
			)
			if (req.body.parentId) {
				// const parentPost = await Post.findOne({
				// 	where: { id: req.body.parentId }
				// })
				// if (parentPost) {
				// 	await parentPost.$add("children", newPost.id)
				// 	await parentPost.save()
				// }
				const parentPost = await Post.findByPk(req.body.parentId)
				if (parentPost) {
					newPost.$set("parent", req.body.parentId)
					await newPost.save()
				} else {
					throw new Error("Parent post not found")
				}
			}

			res.json(newPost)
		} catch (e) {
			res.status(400).json(e)
		}
	}
)

Proposed implementation

// server/src/routes/posts.ts
function render({ title, files, visibility, userId, password: hashedPassword, expiresAt }): PostResponse { 
	return { title, files, visibility, userId, password: hashedPassword, expiresAt }
}

posts.post(
	"/create",
	jwt,
	celebrate(...),
	hashPassword,
	async (req, res, next) => {
		try {
                        const { title, files, visibility, userId, hashedPassword, expiresAt, parentId } = req.body

                        const newPost = await postService.create({ title, files, visibility, userId, password: hashedPassword, expiresAt, parentId })
			res.json(render(newPost))
		} catch (e) {
			res.status(400).json(e)
		}
	}
)

We can then discuss how to implement the dependency injection mechanism of the postService.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read server/src/routes/posts.ts and the /posts/create flow, focusing on the validation, hashing, and Sequelize persistence shown in the issue. Then consider the proposed postService and dependency-injection direction; done would mean the route delegates post creation and no longer directly manages the database operations.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend, database
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.