Refactor users route to be split into controller/service/repository layers

[jazhen]

Requesting suggestions/opinions on the flow and code.

New proposed flow:

[zoobot]

Is this back to draft mode or was it always draft mode?

[jazhen]

This was in draft mode from the start. Just wanted any feedback on this somewhat rough draft.

[jazhen]

I can start working on other routes if no major issues.

[zoobot]

Is there any way to have this be pure functions?

Personally I'd prefer going the route of typescript than heading the backend into a bunch of classes.
You can set interfaces with typescript which would make for more strict/easily testable code.

[zoobot]

The current backend structure seems simple, easy to read, and less code.
I am not sure I have the context for this extensive a change.
What's the benefit of adding the abstraction and more verbose code?

[jazhen]

The benefit is mainly in the unit tests. Currently we only have integration tests, which requires us to spin up the database and are slower due to that. Adding the abstraction and dependency injection would allow us to unit test the controllers and services without using a real database.

It would also allow us to break up larger functions into different services that we can unit test individually, rather that rely on integration tests to test a huge block of code. This is not hugely relevant for this users route, but would be beneficial for the trees router, for example.

Ultimately I think it will allow us to have code that's more resilient to changes. And make running the tests more easy, since I know that that has been a pain point.

[zoobot]

Is there any way to keep readability, simplicity, AND do unit tests?
Easier to read code seems like it will be inherently easier to test and maintain over the long term.
Is there a mock db package we can use? Is there a middle ground?

[jazhen]

If we want to add unit tests, I think we have a choice between two options:

1. We intercept and return mocked responses for each external dependency call (e.g. database call). This works, but I think it's better to limit the amount of mocking when we have control over the external dependencies we would mock. Mocking adds complexity to the unit tests and makes them less maintainable. We need to add N mocks to each test depending on the amount of dependencies that we are coupled to. Ideally the tests are a form of documentation/contract on our app is supposed to function. A person who is new to the project would be able to view the tests to understand how the different parts of our app works.

2. We need to have a way to remove the dependency on the real database for the unit tests. The only way to do so would be to switch the real database for a fake implementation of the database (e.g. in-memory db, fake/mock db) using dependency injection. Splitting the backend into controller/service/repository layers is a well-established architecture pattern to make sure apps follow the SOLID principles.

Obviously, I have a bias for #2 since that is what this draft is implementing. There is a trade off between having code that is easily readable since it's located all in one place and having code that is split up, but can also be more readable since each part is simpler. Having code that is split up makes it harder to connect the dots on how the flow works, but I think that's inevitable as we keep on adding features. I would argue that we abstract things so that developers don't need to understand the entire flow to start making changes, even though abstraction adds more code and complexity. It doesn't necessarily make things less readable. It can make the things more readable by abstracting the code that is unimportant to understand the core functionality of a single unit.

Open to feedback/thoughts on this.

[zoobot]

Can we use jest to mock the db without moving the code base over to OOP?

[zoobot]

I am just data gathering/researching here. I know you've probably done a ton of work on this already so just trying to get on the same page and look around to see what exists for mocking dbs for node.
https://sammeechward.com/mocking-a-database-with-jest-in-javascript/

https://jestjs.io/docs/setup-teardown
https://jestjs.io/docs/mock-functions

I have not tried these but they look good
https://www.npmjs.com/package/supertest
https://www.atdatabases.org/docs/pg-test

This uses a tmp docker just for the tests. It's surprisingly fast but does cross the line a bit to integration tests. I've read that testing an actual test db does stop a good amount of bugs.
https://github.com/balena-io/open-balena-api/blob/master/package.json
https://github.com/balena-io/open-balena-api/blob/master/automation/fasttest.sh

[jazhen]

Thanks for doing this research Rose! I think the last two links you provided could be useful for our integration tests. Currently we are adding a bunch of data to our dev docker database, which do not get removed/reset unless the developer manually resets the docker container themselves. I was working on creating a temp docker env for tests, but it fell by the wayside while working on this.

Can we use jest to mock the db without moving the code base over to OOP?

We can mock the db, but that wouldn't change the ability to unit test. We would have to mock the api call too (e.g. POST /api/trees), which is what supertest does. This doesn't feel good to me because the unit test would be almost the same as the integration tests, except we replace external calls with mocks where needed. The integration tests should test the api call from end to end, only caring about the input and output (could also call these E2E tests). Everything in between does not get tested. The unit tests should be smaller scale than the integration tests and target those gaps. What I am proposing is a change to make our code more unit testable through splitting the code into parts (e.g. controller/services) that require minimal (if any) external calls.

In addition, mocking carries a higher maintenance cost than creating a fake database. For example, if we mock the same database call (e.g. get user from database) for a series of tests against a single endpoint and that mock breaks we would need to update that mock in every branching path, e.g. happy path 1, happy path 2, failure path 1, failure path 2, ... Sometimes it's unavoidable, but in this case we can create a fake database that would be the only thing we need to maintain.

Is there any way to have this be pure functions? Personally I'd prefer going the route of typescript than heading the backend into a bunch of classes. You can set interfaces with typescript which would make for more strict/easily testable code.

I also wanted to response to this comment, which I missed earlier. I am onboard with moving to Typescript, but I don't know that it would solve the core issue you had, which is having classes vs. pure functions. Since Typescript has interfaces, it has better support for having a class interface that the prod and fake repository class would implement (vs. the hacky solution that's required in JavaScript). I think that these repositories are the only place I'm using classes/OOP, so we could just change it to be functional instead. This would however require more maintenance since the implementations (i.e. prod and fake repository) could diverge from each other.

Overall, the goal of these changes are to allow for unit testing, which should improve maintainability by giving more hints on which part of the system is broken. This is done through dependency injection, which will allow us to easily plug and play different dependencies (e.g. plugging in a fake test database). This also minimizes and insulates the code changes that one specific area (e.g .service, function, etc.) has, which would help with making modifications.

[zoobot]

Hey @jazhen Let me try this refactor on the sources backend tonight.

[jazhen]

@zoobot You're free to try it out. The code is still a bit rough as I didn't want to spend extra time if it wasn't going to be used. The changed code shouldn't have affected the sources in any way so it should be fine to use.

[zoobot]

Hey @jazhen Is it ok if I leave this open for now? I am just getting started with testing backend and I need to set up a testdb so wanted to check out this branch to see if you've dealt with passing around a testdb. Should have time later and then can close it

[jazhen]

@zoobot - Sure no problem. Just going through and cleaning up some things. Leave this open as long as you need. Thanks for asking!

[zoobot]

Thanks!

[Tijani-zainab]

@jazhen you're back! 🥳

[jazhen]

@Tijani-zainab Hope you're doing well!

[Tijani-zainab]

I'm doing good, Thank you for asking!
Great to have you back!! @jazhen

[jazhen]

@Tijani-zainab I'm not quite back. Just needed to tie up some loose ends. But I'm glad to hear you're doing good. 🙂

[Tijani-zainab]

Aw, that's sad. Hope we see you again soon!
I miss the meetings lately (school work, hope to get back soon!), but missed seeing you at the meetings whenever I get to attend!!

[zoobot]

+1 What Tijani said. I miss you both at meetings!

[Tijani-zainab]

Missed you too @zoobot!
attending tomorrow, hope to see you there!! 😌