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

Modularity Refactor - Stop treating HTTP like it's special #199

Merged
merged 53 commits into from
Dec 13, 2023

Conversation

confused-Techie
Copy link
Member

@confused-Techie confused-Techie commented Sep 6, 2023

Requirements

  • Filling out the template is required.

  • All new code requires tests to ensure against regressions.

    • However, if your PR contains zero code changes, feel free to select the checkmark below to indicate so.
  • Have you ran tests against this code?

  • This PR contains zero code changes.

Description of the Change

This PR is obviously quite huge, and aims to do quite a bit. But first some backstory on why I thought this PR was needed.

It's no secret that this repo has become unwieldy and quite large. Which large isn't an issue, but the way it was structed became troublesome. For example lets say someone reported an error on a certain endpoint, to trace the calls made here would look something like this:

  1. Read through the multi-thousand long file main.js to determine where this endpoint was kicked off.
  2. Once determined, we could ensure things worked there by also checking query.js
  3. Then we could trace the call through the appropriate handler module, which also consisted of several huge files
  4. Then finally once the right function was found could we properly investigate the function for the source of the bug.

This become a process that I did not want to make often, and resulted in the whole of the backend being troublesome and in short, not fun, to work with.

This PR aims to fix this, as well as start the work on a few other things.

This PR, in short, adds a new REST API builder on top of ExpressJS. Here we use setupEndpoints as an endpoint builder, taking all the data declare within the endpoint module and adding it in here, setting it up appropriately. This now means that all data specific to any given endpoint is fully defined within it's endpoint module, rather than being messily strewn about through several modules. It also means that if one endpoint is being built successfully, they all are, and we can assume as long as the backend works we only generally need to investigate that singular endpoint module.

While moving everything into this tinier more compact and reasonable endpoint modules I've migrated to a more robust ServerStatusObject return. One which has a better API to utilize, meaning it can retain more data than what is being returned to the user, which was an issue with the previously used one. This means that if an error occurs, even if not ever given to the user themselves, we can still log critical troubleshooting information server side, within the same object. Which will hopefully help us assist our users better than ever before.

Additionally, with these new endpoint modules, I've set the groundwork for something also exciting. Previously the API has been documented in two locations, a local Markdown file here, and the hand crafted Swagger specification created by @Spiker985, but now, this endpoint module is able to declare specifics of it's endpoint that can be ingested by a script to automatically create an updated swagger document with ease. While not yet implemented, this potentially means that the swagger document will become the source of truth for the Pulsar backend API, and will be updated automatically, not to some code comments left by us, but instead by the actual objects handled and utilized within the actual API endpoint!

Speaking of the bonus of these new endpoint modules, where the objects declared on the module are used to build the endpoint, which can also be used to build the swagger document, but even can be used to ensure the endpoint returns the exact data needed via tests. The new custom Jest Expect extension toMatchEndpointSuccessObject() is able to test a returned object from an endpoint against what it says it needs to return on success, which can then test this exact object against one of the ./tests/models objects, which is also used to build the swagger document. Basically, we declare our objects once, and each endpoint says what it should be returning, the tests then ensure that this is what's actually being returned, and Swagger can then tell users that this is what should be returned. It makes it all rather cyclical, and in my opinion awesome.

Lastly, with the changes to how endpoints are created, I've also made the decision to import a context object to every single endpoint on every call. This context object handles providing imported modules for nearly all other features that an endpoint will need to utilize, which makes testing our system boundaries easier than ever, since there's no need for Jest specific, or complex mocks of modules, we instead can just define custom functions on this context object during tests.

This last change in particular means that for the first time ever on this repo, we are actually able to test the publication and deletion of packages from the backend. Since previously these calls were so connected to actual API usage on GitHub, we couldn't deconstruct those API calls from the function itself, but now we can!


I realize that this is a huge diff, it was a huge undertaking, especially considering it's taken me three months to complete. So I don't expect a full review, and instead we will likely have to trust that the tests are honest and real, but of course once merged and pushed to production, I'll be ready and watching to ensure everything has worked, exactly how we expect.

Thanks a ton for any feedback, and of course feel free to ask any questions.

src/models/paginateSso.js Fixed Show fixed Hide fixed
src/models/paginateSso.js Fixed Show fixed Hide fixed
src/models/paginateSso.js Fixed Show fixed Hide fixed
src/models/ssoPaginate.js Fixed Show fixed Hide fixed
src/models/ssoPaginate.js Fixed Show fixed Hide fixed
src/models/sso.js Fixed Show fixed Hide fixed
src/controllers/getUsers.js Fixed Show fixed Hide fixed
src/models/sso.js Fixed Show fixed Hide fixed
src/models/sso.js Fixed Show fixed Hide fixed
src/handlers/nonMigratedHandlers.js Fixed Show fixed Hide fixed
src/handlers/nonMigratedHandlers.js Fixed Show fixed Hide fixed
src/handlers/nonMigratedHandlers.js Fixed Show fixed Hide fixed
src/setupEndpoints.js Fixed Show fixed Hide fixed
src/models/sso.js Fixed Show fixed Hide fixed
* Added more tests
* Fixed a previously uncaught bug of the type of data returned within the user object
* Created a system for strictly checking the objects returned from endpoints, to ensure they match what's intended
tests/models/packageObjectFullArray.js Fixed Show fixed Hide fixed
tests/http/getThemes.test.js Fixed Show fixed Hide fixed
tests/http/getThemesSearch.test.js Fixed Show fixed Hide fixed
@confused-Techie confused-Techie marked this pull request as ready for review November 9, 2023 05:53
Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Just commenting that I acknowledged this on Discord.

It feels a bit over my head levels of comprehension, too much so to review it, but no-one told you not to merge this, and they were given ample time to object, so... Y'know?

EDIT to add: Also, I will be around if anyone is needed to test / help with anything, and so on. I can at least offer that much.

@confused-Techie confused-Techie merged commit 3867143 into main Dec 13, 2023
5 of 6 checks passed
@confused-Techie confused-Techie deleted the use-controllers branch December 13, 2023 02:35
@confused-Techie confused-Techie mentioned this pull request Dec 13, 2023
2 tasks
@confused-Techie confused-Techie mentioned this pull request Jan 13, 2024
2 tasks
@confused-Techie confused-Techie mentioned this pull request Jan 20, 2024
2 tasks
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.

2 participants