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
Merged
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
9daa044
Setup initial `models` and `controllers` folder
confused-Techie Sep 6, 2023
a1d593a
Add new documentation of plans
confused-Techie Sep 6, 2023
28c22e4
Create `setupEndpoints` logic, and utilize pattern in new controller
confused-Techie Sep 7, 2023
b29cbdd
Refine the `endpoint` object, bake in expected interactions
confused-Techie Sep 8, 2023
93aa064
More endpoints, pre/post logic functions, docs object
confused-Techie Sep 9, 2023
ae3f29d
Small leanup
confused-Techie Sep 9, 2023
0864d96
Use `param` objects
confused-Techie Sep 11, 2023
2564295
Implement additional endpoints
confused-Techie Sep 11, 2023
4a45d42
Installation endpoint
confused-Techie Sep 11, 2023
be5bc7c
More docs
confused-Techie Sep 11, 2023
123fcdd
New endpoint, better error handling, support for messages
confused-Techie Sep 12, 2023
c89ce74
More endpoints, new http cycle functions
confused-Techie Sep 12, 2023
445adf1
Begin work on tests, transferring over everything possible
confused-Techie Sep 12, 2023
b65c581
Migrate everything but oauth handlers
confused-Techie Sep 13, 2023
97bdd56
Fix `successStatus`
confused-Techie Sep 13, 2023
39d961a
Migrate last handlers, support RAW endpoint types for increased contr…
confused-Techie Sep 14, 2023
f1bad98
Enable more tests, work on in module docs
confused-Techie Sep 14, 2023
6eed248
Even better docs. Use the `endpoint.params` object to get details of …
confused-Techie Sep 14, 2023
8964ae1
Vastly improved documentation of new modules
confused-Techie Sep 15, 2023
7708f48
New params, more docs
confused-Techie Sep 15, 2023
18c5e9b
Add more tests back, fix short assignment within SSO
confused-Techie Sep 15, 2023
3fdcf05
`getStars` tests
confused-Techie Sep 15, 2023
701d0e3
100% coverage in `getStars.js`
confused-Techie Sep 15, 2023
0d2cca7
Tests, bug fixes, strict objects
confused-Techie Sep 16, 2023
e302c2d
Total test coverage of `getUsers.js`
confused-Techie Sep 17, 2023
10768d2
Move `matchesSuccessObject()` to expect extension
confused-Techie Sep 17, 2023
1f47b11
Theme testing, stop outdated migrations
confused-Techie Sep 18, 2023
fafca78
Remove logging
confused-Techie Sep 18, 2023
db669bb
Add back a few tests, fix endpoint registration issues
confused-Techie Sep 18, 2023
8d28377
Use envrons for finding serverurl in testing
confused-Techie Sep 18, 2023
eb0e5bd
Rewrite root testing following new procedure
confused-Techie Sep 18, 2023
104cf0b
Testing for HTTP handling, refactored root testing, update testing
confused-Techie Sep 18, 2023
70a4a8f
Finish TODO of reading from filesystem instead of hardcoded dev retur…
confused-Techie Sep 19, 2023
03c4eeb
Add `breaks` in setupEndpoint to ensure we stop handling in the switc…
confused-Techie Sep 19, 2023
81e91e3
Tests for `getThemesFeatured`
confused-Techie Sep 19, 2023
1f3d6b7
Remove dev logging
confused-Techie Sep 19, 2023
d33183a
Cleanup `package.json`
confused-Techie Sep 19, 2023
90d40ce
Move `storage.js` from `ServerStatusObject`
confused-Techie Sep 19, 2023
37def97
`getThemesSearch` tests
confused-Techie Sep 19, 2023
c74f305
Testing for `getPackagesPackageName` integrate Joi for testing object…
confused-Techie Sep 19, 2023
2ea0e16
Improved testing of package objects, fixed bugs found by this, implem…
confused-Techie Sep 20, 2023
7b138ea
Ensure we properly return a `204` here, as PPM expects it
confused-Techie Sep 26, 2023
59f4399
Implement testing for featured packages, minor cleanup
confused-Techie Sep 26, 2023
0269e14
Add return data to `deletePackagesPackageName`
confused-Techie Sep 26, 2023
1dc2f47
Additional endpoint testing
confused-Techie Oct 20, 2023
836c2b5
Migrate and rewrite majority of remaining tests
confused-Techie Oct 23, 2023
fce91b7
Properly ignore our new tests folder
confused-Techie Oct 23, 2023
54b1f1c
Address code scanning alerts
confused-Techie Oct 24, 2023
cc95221
Add `DELETE /api/packages/:packageName` tests
confused-Techie Nov 8, 2023
5248970
Add in some more critical tests
confused-Techie Nov 9, 2023
a5d247a
Major repo cleanup
confused-Techie Nov 9, 2023
d17bd0d
Ensure Codacy ignores migration files
confused-Techie Nov 9, 2023
004005e
Rename refactor-DONT_KEEP_THIS_FILE.md to refactor-docs.md
confused-Techie Dec 10, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions docs/resources/refactor-DONT_KEEP_THIS_FILE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
# DON'T KEEP THIS FILE

Alright, so lets do a big time refactor, because it's fun, and I get bothered looking at the same code for too long or something.

Essentially here's the new idea:

## HTTP Handling

Stop treating it as if it was special. HTTP handling is essentially only a utility function that's easily **replicable** and should be treated as such.

The only part of an HTTP handling process that matters is the logic that's preformed. The logic of returning the data depending on states of SSO's or adding pagination or even erroring out is insanely easily replicable.

So we should abstract away from hardcoding endless functions for HTTP handling as much as possible. So here's my idea:

Every endpoint is it's own tiny module. This module should export at least two things:

* `logic()` This function will be called to handle the actual logic of the endpoint, passing all relevant data to it
* `params()` This function will return a parameter object consisting of all query parameters that this endpoint expects to receive
* `endpoint` The endpoint object will then provide the endpoint logic with everything else it needs to define any given endpoint.

From here the `main.js` module should instead import all modules needed, and iterate through them to create every single endpoint as needed. This may result in a slightly longer startup time, but overall I hope the increase in code readability and less duplication will be worth it.

So this means that every module is imported, the `endpoint` object is read to setup the endpoint, and from there, it's made available as an endpoint via express, which can then, once hit, use the `params()` function to prepare the query parameters, and then pass those off to the `logic()` function.

### `endpoint` Structure

The `path` here is an array since in some instances, we want to accept multiple paths, such as `POST /api/packages` and `POST /api/themes`.

```javascript
const endpoint = {
// Can be "GET", "POST", "DELETE"
method: "GET",
paths: [ "/api/themes" ],
// Can be "generic" or "auth"
rate_limit: "generic",
options: {
// This would be the headers to return for `HTTP OPTIONS` req:
Allow: "GET",
"X-Content-Type-Options": "nosniff"
}
};
```

## Returning HTTP handling

Again, the logic here is easily replicable. So we shouldn't make it special. And if finally doing a proper rewrite, we can incorporate a proper SSO, and have a different type for every single return. This way the actual handling of any return, can instead be a factor of a `httpReturn()` function of the SSO itself, rather than baked in logic. So that way we can keep the return logic as unique as needed, as the uniqueness depends solely on the uniqueness of the SSO being returned.

## Tests

(As always the bane of existence)

With this refactor, we no longer need true "integration" tests. As integration can be tested on if the proper endpoints being hit call the proper endpoint.logic() function. Beyond that the majority of "integration" testing would be relegated to interactions with external services working as expected.

Meaning the only tests we would likely need are:

* `tests` This would be the vast majority of tests, able to be generic, and not needing any fancy setup
* `database` This suite of tests should purely test if DB calls do what we expect
* `integration` A small suite of full integration tests is never a bad idea. To test that API calls have the intended effects on the DB. With a huge focus on having the intended effects. As we are seeing some examples where the expected data is not appearing or being applied to the DB as we want.
* `external` We don't do this currently. But a suite of external tests that are run on a maybe monthly basis is not a bad idea. This could allow us to ensure external APIs are returning data as expected.

---

I think this is plenty to focus on now. At the very least the changes described here would likely mean a rewrite of about or over half the entire codebase. But if all goes to plan, would mean that every single piece of logic is more modular, keeping logic related to itself within the same file, and if tests are effected as hoped, would mean a much more robust testing solution, that who knows, may actually be able to achieve near 100% testing coverage.

One side effect of all this change, means the possibility of generating documentation of the API based totally on the documentation itself, where we no longer would be reliant on my own `@confused-techie/quick-webserver-docs` module, nor having to ensure comments are updated.

## Documentation

Alright, so some cool ideas about documentation.

Within `./src/parameters` we have modules named after the common name of any given parameter.

Then to ensure our OpenAPI docs are ALWAYS up to date with the actual code, we take the `params` object of every single endpoint module, and we actually iterate through the `params` there and match those against these modules within `parameters`.

So that the parameters within our docs will always match exactly with what's actually used. This does mean the name we use for the parameter within code has to match up with the names of the files.

Additionally, for the content, we can reference a fileName from ./tests/models so that we can accomplish the following:

* Use that object's `schema` and `example` to generate an object
* We can also possibly use this to ensure the return matches what we expect.
54 changes: 54 additions & 0 deletions jest.OUTDATED.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
const config = {
setupFilesAfterEnv: ["<rootDir>/test/global.setup.jest.js"],
verbose: true,
collectCoverage: true,
coverageReporters: ["text", "clover"],
coveragePathIgnorePatterns: [
"<rootDir>/src/tests_integration/fixtures/**",
"<rootDir>/test/fixtures/**",
"<rootDir>/node_modules/**",
],
projects: [
{
displayName: "Integration-Tests",
globalSetup: "<rootDir>/node_modules/@databases/pg-test/jest/globalSetup",
globalTeardown:
"<rootDir>/node_modules/@databases/pg-test/jest/globalTeardown",
setupFilesAfterEnv: [
"<rootDir>/test/handlers.setup.jest.js",
"<rootDir>/test/global.setup.jest.js",
],
testMatch: [
"<rootDir>/test/*.integration.test.js",
"<rootDir>/test/database/**/**.js",
],
},
{
displayName: "Unit-Tests",
setupFilesAfterEnv: ["<rootDir>/test/global.setup.jest.js"],
testMatch: [
"<rootDir>/test/*.unit.test.js",
"<rootDir>/test/handlers/**/**.test.js",
"<rootDir>/test/controllers/**.test.js"
],
},
{
displayName: "VCS-Tests",
setupFilesAfterEnv: ["<rootDir>/test/global.setup.jest.js"],
testMatch: ["<rootDir>/test/*.vcs.test.js"],
},
{
displayName: "Handler-Tests",
globalSetup: "<rootDir>/node_modules/@databases/pg-test/jest/globalSetup",
globalTeardown:
"<rootDir>/node_modules/@databases/pg-test/jest/globalTeardown",
setupFilesAfterEnv: [
"<rootDir>/test/handlers.setup.jest.js",
"<rootDir>/test/global.setup.jest.js",
],
testMatch: ["<rootDir>/test/*.handler.integration.test.js"],
},
],
};

module.exports = config;
55 changes: 22 additions & 33 deletions jest.config.js
Original file line number Diff line number Diff line change
@@ -1,53 +1,42 @@
const config = {
setupFilesAfterEnv: ["<rootDir>/test/global.setup.jest.js"],
setupFilesAfterEnv: [
"<rootDir>/tests/helpers/global.setup.jest.js"
],
verbose: true,
collectCoverage: true,
coverageReporters: ["text", "clover"],
coverageReporters: [
"text",
"clover"
],
coveragePathIgnorePatterns: [
"<rootDir>/src/tests_integration/fixtures/**",
"<rootDir>/test/fixtures/**",
"<rootDir>/node_modules/**",
"<rootDir>/tests/",
"<rootDir>/test/",
"<rootDir>/node_modules/"
],
projects: [
{
displayName: "Integration-Tests",
globalSetup: "<rootDir>/node_modules/@databases/pg-test/jest/globalSetup",
globalTeardown:
"<rootDir>/node_modules/@databases/pg-test/jest/globalTeardown",
globalTeardown: "<rootDir>/node_modules/@databases/pg-test/jest/globalTeardown",
setupFilesAfterEnv: [
"<rootDir>/test/handlers.setup.jest.js",
"<rootDir>/test/global.setup.jest.js",
"<rootDir>/tests/helpers/handlers.setup.jest.js",
"<rootDir>/tests/helpers/global.setup.jest.js"
],
testMatch: [
"<rootDir>/test/*.integration.test.js",
"<rootDir>/test/database/**/**.js",
],
"<rootDir>/tests/database/**.test.js",
"<rootDir>/tests/http/**.test.js",
]
},
{
displayName: "Unit-Tests",
setupFilesAfterEnv: ["<rootDir>/test/global.setup.jest.js"],
testMatch: [
"<rootDir>/test/*.unit.test.js",
"<rootDir>/test/handlers/**/**.test.js",
],
},
{
displayName: "VCS-Tests",
setupFilesAfterEnv: ["<rootDir>/test/global.setup.jest.js"],
testMatch: ["<rootDir>/test/*.vcs.test.js"],
},
{
displayName: "Handler-Tests",
globalSetup: "<rootDir>/node_modules/@databases/pg-test/jest/globalSetup",
globalTeardown:
"<rootDir>/node_modules/@databases/pg-test/jest/globalTeardown",
setupFilesAfterEnv: [
"<rootDir>/test/handlers.setup.jest.js",
"<rootDir>/test/global.setup.jest.js",
"<rootDir>/tests/helpers/global.setup.jest.js"
],
testMatch: ["<rootDir>/test/*.handler.integration.test.js"],
},
],
testMatch: [
"<rootDir>/tests/unit/**/**.test.js"
]
}
]
};

module.exports = config;
4 changes: 1 addition & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,13 @@
"test:unit": "cross-env NODE_ENV=test PULSAR_STATUS=dev jest --selectProjects Unit-Tests",
"test:integration": "cross-env NODE_ENV=test PULSAR_STATUS=dev jest --selectProjects Integration-Tests",
"start:dev": "cross-env PULSAR_STATUS=dev node ./src/dev_server.js",
"test": "cross-env NODE_ENV=test PULSAR_STATUS=dev jest --selectProjects Integration-Tests Unit-Tests VCS-Tests",
"test": "cross-env NODE_ENV=test PULSAR_STATUS=dev jest --selectProjects Integration-Tests Unit-Tests --runInBand",
"test:vcs": "cross-env NODE_ENV=test PULSAR_STATUS=dev MOCK_GH=false jest --selectProjects VCS-Tests",
"test:handlers": "cross-env NODE_ENV=test PULSAR_STATUS=dev jest --selectProjects Handler-Tests",
"api-docs": "quick-webserver-docs -i ./src/main.js -o ./docs/reference/API_Definition.md",
"lint": "prettier --check -u -w .",
"complex": "cr --newmi --config .complexrc .",
"js-docs": "jsdoc2md -c ./jsdoc.conf.js ./src/*.js ./src/handlers/*.js ./docs/resources/jsdoc_typedef.js > ./docs/reference/Source_Documentation.md",
"contributors:add": "all-contributors add",
"test_search": "node ./scripts/tools/search.js",
"migrations": "pg-migrations apply --directory ./scripts/migrations",
"ignore": "compactignore",
"tool:delete": "node ./scripts/tools/manual-delete-package.js",
Expand Down
120 changes: 120 additions & 0 deletions scripts/deprecated-migrations/0008-migrated-initial.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
-- Enter our Test data into the Database.

INSERT INTO packages (pointer, package_type, name, creation_method, downloads, stargazers_count, data, original_stargazers)
VALUES (
'd28c7ce5-c9c4-4fb6-a499-a7c6dcec355b', 'package', 'language-css', 'user made', 400004, 1,
'{"name": "language-css", "readme": "Cool readme", "metadata": {"bugs": {"url": "https://github.com/atom/language-css/issues"},
"name": "language-css", "engines": {"atom": "*","node":"*"},"license":"MIT","version":"0.45.7","homepage":"http://atom.github.io/language-css",
"keywords":["tree-sitter"],"repository":{"url":"https://github.com/atom/language-css.git","type":"git"},"description":"CSS Support in Atom",
"dependencies":{"tree-sitter-css":"^0.19.0"},"devDependencies":{"coffeelint":"^1.10.1"}},"repository":{"url":"https://github.com/atom/langauge-css",
"type":"git"}}', 76
), (
'd27dbd37-e58e-4e02-b804-9e3e6ae02fb1', 'package', 'language-cpp', 'user made', 849156, 1,
'{"name": "language-cpp", "description": "C++ Support in Atom", "keywords": ["tree-sitter"]}', 91
), (
'ee87223f-65ab-4a1d-8f45-09fcf8e64423', 'package', 'hydrogen', 'Migrated from Atom.io', 2562844, 1,
'{"name": "hydrogen", "readme": "Hydrogen Readme", "metadata": { "main": "./dist/main", "name": "Hydrogen",
"author": "nteract contributors", "engines": {"atom": ">=1.28.0 <2.0.0"}, "license": "MIT", "version": "2.16.3"}}', 821
), (
'aea26882-8459-4725-82ad-41bf7aa608c3', 'package', 'atom-clock', 'Migrated from Atom.io', 1090899, 1,
'{"name": "atom-clock", "readme": "Atom-clok!", "metadata": { "main": "./lib/atom-clock", "name": "atom-clock",
"author": { "url": "https://github.com/b3by", "name": "Antonio Bevilacqua", "email": "[email protected]"}}}', 528
), (
'1e19da12-322a-4b37-99ff-64f866cc0cfa', 'package', 'hey-pane', 'Migrated from Atom.io', 206804, 1,
'{"name": "hey-pane", "readme": "hey-pane!", "metadata": { "main": "./lib/hey-pane", "license": "MIT"}}', 176
), (
'a0ef01cb-720e-4c0d-80c5-f0ed441f31fc', 'theme', 'atom-material-ui', 'Migrated from Atom.io', 2509605, 1,
'{"name": "atom-material-ui", "readme": "ATOM!"}', 1772
), (
'28952de5-ddbf-41a8-8d87-5d7e9d7ad7ac', 'theme', 'atom-material-syntax', 'Migrated from Atom.io', 1743927, 1,
'{"name": "atom-material-syntax"}', 1309
), (
'504cd079-a6a4-4435-aa06-daab631b1243', 'theme', 'atom-dark-material-ui', 'Migrated from Atom.io', 300, 1,
'{"name": "atom-dark-material-ui"}', 2
);

INSERT INTO names (name, pointer)
VALUES (
'language-css', 'd28c7ce5-c9c4-4fb6-a499-a7c6dcec355b'
), (
'language-cpp', 'd27dbd37-e58e-4e02-b804-9e3e6ae02fb1'
), (
'hydrogen', 'ee87223f-65ab-4a1d-8f45-09fcf8e64423'
), (
'atom-clock', 'aea26882-8459-4725-82ad-41bf7aa608c3'
), (
'hey-pane', '1e19da12-322a-4b37-99ff-64f866cc0cfa'
), (
'atom-material-ui', 'a0ef01cb-720e-4c0d-80c5-f0ed441f31fc'
), (
'atom-material-syntax', '28952de5-ddbf-41a8-8d87-5d7e9d7ad7ac'
), (
'atom-dark-material-ui', '504cd079-a6a4-4435-aa06-daab631b1243'
);

INSERT INTO versions (package, status, semver, license, engine, meta)
VALUES (
'd28c7ce5-c9c4-4fb6-a499-a7c6dcec355b', 'published', '0.45.7', 'MIT', '{"atom": "*", "node": "*"}',
'{"name": "language-css", "description": "CSS Support in Atom", "keywords": ["tree-sitter"],
"tarball_url": "https://github.com/pulsar-edit/language-css"}'
), (
'd28c7ce5-c9c4-4fb6-a499-a7c6dcec355b', 'latest', '0.46.0', 'MIT', '{"atom": "*", "node": "*"}',
'{"name": "language-css", "description": "CSS Support in Atom", "keywords": ["tree-sitter"]}'
), (
'd28c7ce5-c9c4-4fb6-a499-a7c6dcec355b', 'published', '0.45.0', 'MIT', '{"atom": "*", "node": "*"}',
'{"name":"language-css", "description": "CSS Support in Atom", "keywords": ["tree-sitter"]}'
), (
'd27dbd37-e58e-4e02-b804-9e3e6ae02fb1', 'published', '0.11.8', 'MIT', '{"atom": "*", "node": "*"}',
'{"name": "language-cpp", "description": "C++ Support in Atom", "keywords": ["tree-sitter"]}'
), (
'd27dbd37-e58e-4e02-b804-9e3e6ae02fb1', 'latest', '0.11.9', 'MIT', '{"atom": "*", "node": "*"}',
'{"name": "language-cpp", "description": "C++ Support in Atom", "keywords": ["tree-sitter"]}'
), (
'ee87223f-65ab-4a1d-8f45-09fcf8e64423', 'latest', '2.16.3', 'MIT', '{"atom": "*"}',
'{"name": "Hydrogen", "dist": {"tarball": "https://www.atom.io/api/packages/hydrogen/version/2.16.3/tarball"}}'
), (
'aea26882-8459-4725-82ad-41bf7aa608c3', 'latest', '0.1.18', 'MIT', '{"atom": "*"}',
'{"name": "atom-clock", "dist": {"tarball": "https://www.atom.io/api/packages/atom-clock/version/1.18.0/tarball"}}'
), (
'1e19da12-322a-4b37-99ff-64f866cc0cfa', 'latest', '1.2.0', 'MIT', '{"atom": "*"}',
'{"name":"hey-pane", "dist": {"tarball": "https://www.atom.io/api/packages/hydrogen/version/1.2.0/tarball"}}'
), (
'a0ef01cb-720e-4c0d-80c5-f0ed441f31fc', 'latest', '2.1.3', 'MIT', '{"atom": "*"}',
'{"name": "atom-material-ui", "dist": {"tarball": "https://www.atom.io/api/packages/atom-material-ui/version/2.1.3/tarball"}}'
), (
'28952de5-ddbf-41a8-8d87-5d7e9d7ad7ac', 'latest', '1.0.8', 'MIT', '{"atom":"*"}',
'{"name": "atom-material-syntax", "dist": {"tarball":"https://www.atom/io/api/packages/atom-material-syntax/version/1.0.8/tarball"}}'
), (
'504cd079-a6a4-4435-aa06-daab631b1243', 'latest', '1.0.0', 'MIT', '{"atom": "*"}',
'{"name":"atom-dark-material-ui", "dist": {"tarball": "https://www.atom.io/api/packages/atom-dark-material-ui/versions/1.0.0/tarball"}}'
);

INSERT INTO users (username, node_id, avatar)
VALUES (
'dever', 'dever-nodeid', 'https://roadtonowhere.com'
), (
'no_perm_user', 'no-perm-user-nodeid', 'https://roadtonowhere.com'
), (
'admin_user', 'admin-user-nodeid', 'https://roadtonowhere.com'
), (
'has-no-stars', 'has-no-stars-nodeid', 'https://roadtonowhere.com'
), (
'has-all-stars', 'has-all-stars-nodeid', 'https://roadtonowhere.com'
);

INSERT INTO stars (package, userid)
VALUES (
'd28c7ce5-c9c4-4fb6-a499-a7c6dcec355b', 5
), (
'd27dbd37-e58e-4e02-b804-9e3e6ae02fb1', 5
), (
'ee87223f-65ab-4a1d-8f45-09fcf8e64423', 5
), (
'aea26882-8459-4725-82ad-41bf7aa608c3', 5
), (
'1e19da12-322a-4b37-99ff-64f866cc0cfa', 5
), (
'a0ef01cb-720e-4c0d-80c5-f0ed441f31fc', 5
), (
'28952de5-ddbf-41a8-8d87-5d7e9d7ad7ac', 5
);
Loading