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:
Read through the multi-thousand long file main.js to determine where this endpoint was kicked off.
Once determined, we could ensure things worked there by also checking query.js
Then we could trace the call through the appropriate handler module, which also consisted of several huge files
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.
Requirements
Filling out the template is required.
All new code requires tests to ensure against regressions.
[ ] 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:
main.jsto determine where this endpoint was kicked off.query.jshandlermodule, which also consisted of several huge filesThis 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
setupEndpointsas 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
ServerStatusObjectreturn. 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/modelsobjects, 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
contextobject to every single endpoint on every call. Thiscontextobject 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.