confused-techie / atom-backend Goto Github PK

Is your feature request related to a problem? Please describe.
Some unit tests are broken.

Describe the solution you'd like
Fix them

For the Engine Filter, you'll need to find a version of the package that has the object engines.atom with a compatible engine version. Taking a SemVer compatible version, and comparing it to a lowest, and highest compatible engine version.

Once a compatible version is found, add it to an object titled 'metadata' at the root of the package object.

For reference of the structure of the object that will be given to you, refer here.

For reference of the structure of the object your are helping to create refer here.

Again keep in mind you only need to create the 'metadata' key and add the appropriate data. Do not worry about removing any data present in the Package Object Short this will be handled later.

You'll want to place this logic into the src/collection.js file at the shown lines.

(pack = The Server Package Object; engine = The valid SemVer Engine Version wanted.)

https://github.com/confused-Techie/atom-community-server-backend-JS/blob/ff9ab750b8546b1ad19972c2dfc48c2babdf6953/src/collection.js#L209-L212

As recommended by @Digitalone1 instead of having endless nested functions, with error checking tailing success logic, place error logic at the lead, and get the codebase more flat. Similar to what's seen within package_handler.POSTPackages()

The tests report:

● GET /api/packages/:packageName › Valid package, gives correct object
    expect(received).toBe(expected) // Object.is equality
    Expected: "what-a-package"
    Received: undefined
      30 |   test("Valid package, gives correct object", async () => {
      31 |     const res = await request(app).get("/api/packages/what-a-package");
    > 32 |     expect(res.body.name).toBe("what-a-package");
         |                           ^
      33 |   });
      34 |   test("Invalid Package, gives 'Not Found'", async () => {
      35 |     const res = await request(app).get("/api/packages/invalid-package");
      at Object.toBe (src/tests/main.test.js:32:27)

This is because the query.engine returns false on finding an invalid or non-existant engine, but when the collection.EngineFilter checks the validity of the engine specified, it returns an empty object. A Fix is already in progress that instead returns an unmodified package, so that any consumers of collection.EngineFilter can use it safely without concern over a valid engine being given.

Is this feature Currently Implemented in any way with the upstream API Server

• Yes
• No

Describe the solution you'd like
To accomplish the Featured Packages, as suggested by @Spiker985, we can every week run a task, that looks at every packages download change over the week. And whichever has the highest change is used on the featured page. Maybe trunicated to 30 items. Then once the week is over this is checked again. This can be accomplished by each time its run take a snapshot of the download count, maybe under download_count_snapshot which contains the download count at that time, for it to be looked at again the next week. This new download_count_snapshot will need to be added when a package is installed, and pruned during show, but otherwise should be relatively easy to implement.

Document the API endpoints in 'main.js' using previous documented endpoints as reference.

The documentation can almost literally be taken from Atom's current documentation and inserted following my Quick-WebServer-Docs package.

Once added, the markdown for this documentation will created automatically.

In the below code, verify that 'prov' is a string in the 'owner/repo' format.
Then feel free to remove the todo.

https://github.com/confused-Techie/atom-community-server-backend-JS/blob/156ed2c85aed5f73717d945425a39a0357c9a606/src/query.js#L81-L90

With the database schema defined, the next step in this process is to setup the create scripts.

Some users here are much more familiar with SQL than I am myself, so I'm opening this up to the community for assistance.

Things to note, further details of the different tables in use are available on the pinned discussion.
Additionally the database is running on Postgresql.

We will need create scripts for:

• names
• stars
• users
• packages
• versions

In ./src/test_integration/main.test.js there is a test.todo() at the end of POST /api/packages that needs to be completed. The last tests need to actually modify the data, and attempt to publish a package.

In the above view, you can see the stars are duplicated. This package only contains 821 stars. This is because we are adding the star values incorrectly in utils.

Additionally because we are keeping a count of original_stargazers and stargazers_count we should add these together as numbers, not a string, while also modifying the backend to zero out our current stars, since both values are identical incorrectly currently

Where: ./src/test_integration/main.test.js
What:
The tests for Get /api/packages have a test.todo() at the end, since they need additional tests to ensure the proper functionality of all supported query parameters.

Within the ./src/handlers/package_handler.js file some functions have been migrated to the new DB schema, and some have not been.

The functions that have NOT been migrated are marked with @todo on the JSDoc Comments above the function.

Some functions may need more work than simply using the new Schema, and those should be marked accordingly based on the text of the @todo tag.

Once a function has been successfully migrated, this // TODO can be removed. But this are full migrations, meaning every aspect of the function needs to be modified and tested.

Important to note that NO other handlers have been migrated at this time.

Looks like theres been an oversight, the original docs don't make any mention of themes.

Yet on the original Atom.io Website you are able to browse and search themes.

We currently only implement /api/themes/featured. So this must be expanded.

Now considering the huge rework that may be needed to support every single endpoint properly, I'll propose the following.

Since we have placed such care into meeting the original spec, it would seem wrong to deviate now, and we likely could achieve full support of the Themes slug with the following.

• /api/themes: This would need a new endpoint and a new Database call, keep in mind packages that are themes do have a key in package.json with "theme": "syntax" or "theme": "ui", so having those filtered via that key would allow us to have a full listing of themes on this endpoint.
• /api/themes/featured: This endpoint is already supported and requires zero work.
• /api/themes/search: Again this would require new handling, again filtering like done on /api/themes
• /api/themes/:themeName: Once we are into any endpoints that deal with a singular specific theme we should be safe to stop the specific handling. By having these endpoints in main.js simply redirect to the equivalent functions in package_handler.js

Looking at the last GH Test action run, its status is: The job running on runner Hosted Agent has exceeded the maximum execution time of 360 minutes.

It seems that the engine match has no failed when no engine is provided. Further investigate is needed into why this happens. The last logs available during the test are as follows:

● GET /api/packages/:packageName › Valid package, gives correct object
    TypeError: engine.match is not a function
      240 |   let reg =
      241 |     /(^\W*)([0-9]*).([0-9]*).([0-9]*)\s(\W*)([0-9]*).([0-9]*).([0-9]*)$/;
    > 242 |   let raw_engine = engine.match(/^([0-9]*).([0-9]*).([0-9]*)/);
          |                           ^
      243 |   let engine_semver = {
      244 |     major: raw_engine[1],
      245 |     minor: raw_engine[2],
      at Object.match [as EngineFilter] (src/collection.js:242:27)
      at Object.EngineFilter [as GETPackagesDetails] (src/handlers/package_handler.js:241:31)
      at src/main.js:219:3
  ● GET /api/packages/:packageName › Valid package, gives correct object
    thrown: "Exceeded timeout of 5000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."
      28 |
      29 | describe("GET /api/packages/:packageName", () => {
    > 30 |   test("Valid package, gives correct object", async () => {
         |   ^
      31 |     const res = await request(app).get("/api/packages/what-a-package");
      32 |     expect(res.body.name).toBe("what-a-package");
      33 |   });
      at test (src/tests/main.test.js:30:3)
      at Object.describe (src/tests/main.test.js:29:1)

Hello folks, I hope we are all excited about our soon incoming 1.0 release in time for December.

Theres a few misc tasks that I'd hope to complete in time, so lets see if anyone is up to helping out here, I'll list each type of task below, and feel free to expand in the comments.

• Remove any missed debugging console.log()s
• Migrate error.js into common_handlers.js. Personally I feel it raises the barrier to entry by having two separate api return helpers, especially with one of these handling the logging and the other not. So ideally everything can be moved over to common_handlers each endpoint then gets it's logging, and everything that uses error.js will be moved over to common_handler so that error.js can be deleted completely.
• Pair down our code. This checkmark will realistically never be able to be clicked with certainty, but really this just means a few things.
  • Throughout the codebase, there are some ditched efforts to make our code more compact, but now themselves add to pointless bloat. These should be removed.
  • Once everything is tested and confirmed as working, as in when we are stable, I'd like to remove as many of the database functions as possible. A lot of these are carry-overs from the old JSON data structure.
  • Any usages of auth: req.get("Authorization") in a handler should be replaced to query.auth(req) as this has built in support for error handling.
  • I'd like to implement even more numeric error codes into error handling to make reviewing the logs easier.
• Migrate to new logging format, and add more logs under Trace Logging Level. By migrating our existing logs and generating many new ones, this gets us primed to enable the full feature set in prod, where we will have better visibility into errors that may be occurring on the backend, and allow a much easier debugging and issue resolution time.
• Add Missing theme endpoints
• Add Missing users endpoints

The tests in ./src/tests_integration/main.test.js need to be completed, replacing the test.todo() with tests that modify data, meaning to remove a star from a package.

I know this could be asked on DIscord, but here I can track it better.

I tried to run tests on my system, but failed. Installed docker and npm on Arch Linux.

But on npm run test I got

> [email protected] test
> set NODE_ENV=test&& set PULSAR_STATUS=dev&& jest

sh: row 1: jest: command not found

What am I missing?

Codacy states we have to specify a base for parteInt and I did it, but it's still missing. I think GH Action is removing it because 10 it's the default, but Codacy says to specify it always.

Personally I agree with Codacy since "different implementations may behave differently and it also improves readability", but how to stop modifying it from GH Action?

If there's no option for GH Action, could we set Codacy to ignore it?

I saw Codacy reported an issue on the database URL regex.

@confused-Techie Maybe you can use this one: https://regex101.com/r/P9ibTR/1

Seems an oversight during initial development lead to what could cause confusion.

Most exported functions use CamelCase rather than camelCase which could cause confusion since traditionally CamelCase is used for classes in JS.

When possible it's best to convert the usage of CamelCase for functions to camelCase, while leaving classes alone otherwise.

In ./src/handlers/package_handlers.js postPackagesVersion() there intends to be a way to publish a package while renaming it.

But as of now there is no defined methodology on our end of how to handle such a rename, and confirm that this is renaming the correct package.

Discussion of this issue can be opened here, or a solution can be brought up via PR.

While this was temporarily added in, the feature has been postponed to the first non-parity release. That is 2.0.0

This concern was originally addressed in issue #12

To properly filter for banned packages, as well as vulgar package names, we can use @confused-techie/watchyourlanguage and the built in utils.IsPackageNameBanned()

Since this was already previously written to save some effort later I will leave the excerpts here.

const utils = require("../utils.js");
const { WatchYourLanguage } = require("@confused-techie/watchyourlanguage");

async function POSTPackages(req, res) {
...
  // Additionally we want to check if the package is from a ban list.
  let notBannedName = await utils.IsPackageNameBanned(params.repository.split("/")[1]);

  if (!notBannedName.ok) {
    // in the future we may want to actually return that the name is banned.
    // But for now we will return that its a bad repo.
    await common.BadRepoJSON(req, res);
    return;
  }

  // also lets make sure the name doesn't contain vulgar language

  let language = new WatchYourLanguage();

  let containsBadWord = language.searchWithin(params.repository.split("/")[1]);

  if (containsBadWord) {
    // getting a true boolean here, indicates vulgar language is likely present.
    // which again we will for now, just return bad repo.
    await common.BadRepoJSON(req, res);
    return;
  }
...

Additionally we will want to implement a requirements error within common, that returns the indication of why a package has been denied. Listing some of the requirements for publishing packages, resulting in something like the following.

{
  "message": "The following package doesn't meet criteria: ${REASON_OF_DENIAL}"
  "criteria": [
    "Package Name must not contain vulgarity.",
    "Package Name must not be a previously banned package name.",
    "Package Name must be URL-Safe."
  ]
}

Currently all API endpoints should be documented using quick-webserver-docs, and the codebase is being documented with JSDoc. But the documentation for the codebase itself is severely lacking.

It would be desirable to have everything documented to allow it easier to worked on by new contributors.

Is this feature Currently Implemented in any way with the upstream API Server

• Yes
• No

Is your feature request related to a problem? Please describe.
Yes, spam and malicious packages can clutter the ecosystem, reduce the final product, and hinder the user experience. Plus increase support time dealing with these cases.

Describe the solution you'd like
Since we don't have the resources for a highly advanced solution, the simplest method would be a report counter on a package. Allowing a user to submit report a package, and have that add to the counter. Which could then lower the packages exposure on search results. And possible if enough reports are acquired, even remove a package all together.

If a Search is executed that only has a single page of results, and the query passes page=1 as a non-programmer would, then they are given a not found result, since the database is trying to find the 1st page of results from a zero-indexed position.

This should be resolved in a such a way to allow this to happen.

Or otherwise to investigate further with how Atom.io implements this feature and mirror that functionality.

Due to the substantial changes in data storage and access. The entire codebase will likely need a rewrite.

Below is a flat list of all files within the code base, to assist in tracking their refactor status.

Any files that have been completed will get a checkmark.

• common_handler.js
• oauth_handler.js
• package_handler.js
• star_handler.js
• theme_hadnler.js
• update_handler.js
• user_handler.js
• cache.js
• collection.js
• config.js
• data.js
• database.js
• debug_utils.js
• error.js
• git.js
• logger.js
• main.js
• query.js
• resources.js
• search.js
• server.js
• storage.js
• users.js
• utils.js

As the original API doesn't support OPTIONS at all, it would be a fantastic feature to implement, to support end users in whatever their environment may be.

Currently this gets caught by the Sitemap 404 as a disallowed method.

I don't know why the regex I proposed was flagged by codacy as exponential and polynomial backtracking.

At least I suppose it's like that since @confused-Techie changed it. f24202e

Anyway the current one it's still flagged the same.

For an unknown reason the tests are failing while using collection.Direction to order packages in a specific direction.

The direction seems almost random, and further investigation is needed.

This function still needs to be completed, in its half complete state.

The function has some STEPS: at the start describing what needs to be done, as well as has a JSDoc description at the top describing how it should behave.

N/A

Reminder to myself it's not 4am

Alright one more oversight that's been pointed out, seems there is another undocumented API endpoint that's been discovered,

But we have two undocumented endpoints to cover, and another SQL Database change we need. Which on that last part @Digitalone1 I'd appreciate any assistance you can offer.

• /api/users: This will be an authenticated Endpoint, which returns the authenticating Users information back to them.
• /api/users/:userName: This endpoint will have no authentication, and will return the public details of another user. The big kicker here, this endpoint needs to return all of said users packages they have published.

A few notes:

Firstly these endpoints are undocumented because they don't seem to actual exist on the backend API. But they exist on the frontend website. But since of course this API is supposed to include everything, I feel that it should be added here.

Secondly, the reason I mention a Database change, is because currently there is no foolproof way to determine which packages belong to which user. This is something we do by checking what they have access to on GitHub itself. Meaning we may need to add in another column on the packages that links the user to the package they are publishing, while they publish it.

Which brings me to my third point. What about already published and migrated packages? Do we make any attempt to link them to a user?

Which if we did, would mean that during a users onboarding process we would have to either use the username to determine ownership, which I really don't think is a good idea, because then otherwise we have to get a list of the repos they have access to and check that against every package in the system. This would become expensive fast, and doesn't really seem practical.

So if we decide to let all migrated packages remained orphaned, that's fine and something we can do, but if we do I recommend that for v2 we try and think of how we can let users adopt a migrated package, if they need to. Could be done during any updates, or have a dedicated new endpoint in v2 that would allow them to attempt to adopt a package.

Which the later suggestion is more what I feel is possible for us here, since it would be problematic to do a manual adoption any way I see it.

But would love to hear your thoughts, and that we are able to get these other steps done no matter what we decide to do for existing packages

This function has some commented out code, of items to still finish.

This function has a description in JSDoc at the top describing how it should behave, and it still needs to be completed.

For the time being this returns common.notSupported()

• Which are the limitations for search queries? I though to limit them to a maximum of 50 characters, but there should be another limitation. Maybe converting all non word characters as space?
• Where can I see which are the properties of packages objects? Could be an hint to make a sort algorithm.
• Package pointers are very difficult to understand. If you could explain hare the steps to make a new package and set its pointer, I could help in the remaining unfinished methods.

We check for lowercase names in javascript code, but this is not enough to preserve the integrity of the database since a potential issue in the code could still insert a package with uppercase characters which is not what we want.

So to prevent this we could add a CHECK Constraint on the names columns.

@confused-Techie If you agree, please execute the following SQL code on the database (one line at a time).

ALTER TABLE names ADD CONSTRAINT lowercase_names CHECK (name = LOWER(name));
ALTER TABLE packages ADD CONSTRAINT lowercase_names CHECK (name = LOWER(name));

I tested them on my local database and they're working. Before I tested with UPPER(name) and I got

ERROR: check constraint of relation "names" is violated by some row

Which is what I wanted because I inserted only lowercase names. So if you execute the code with LOWER and got the same error, it means one or more names are violating our policy of having lowercase names and this issue should be resolved.

@confused-Techie Let me know!

There are some packages that have malicious intent, or otherwise are harmful to the ecosystem, that should be pruned. The list of those can be created here.

@confused-Techie I was testing the code but the tests stuck at main.test,js.

I thought it was something I made wrong, but I couldn't fix the issue. In another branch I found out the tests was blocking even removing my new commits.

So I removed also your commits and the test can complete on my system.

Are you sure you complete the tests before the merge?

Currently sort only returns the packages if sort is set to downloads, and then doesn't even sort them.

This needs to be written supporting each sort method, with the data in mind.

In ./src/handlers/package_handler.js getPackagesSearch it is currently using the built in search on the database. But ideally I would like to see the ability to use the custom LCS search created previously on this.

If anyone has ideas on how this can be accomplished, lets discuss them here, or a PR could be opened with a solution.

Currently a duplicate version of package versions can be published, and this should be prevented on the db side

Is this feature Currently Implemented in any way with the upstream API Server

• Yes
• No

Is your feature request related to a problem? Please describe.
Prevent spam and vulgar packages.

Describe the solution you'd like
Some package names would be banned, disallowing them to ever be uploaded. To help reduce spam names, confusing names, or just vulgar names.

removePackageByName() in database.js currently has a very ugly solution of running multiple commands and checking each for an error.

This should be upgraded into a proper transaction so things can rollback if something were to go wrong during the transaction.

This error is currently being reported from the production instance:

TypeError: Cannot set properties of undefined (setting 'downloads')
    at Object.constructPackageObjectShort (/workspace/src/utils.js:107:23)
    at Object.getPackagesSearch (/workspace/src/handlers/package_handler.js:260:30)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) 

As far as I can tell a confirmed instance this was triggered was:

{
  "textPayload": "HTTP:: ::ffff:127.0.0.1 [2022-11-10T16:15:10.789Z] \"GET /api/packages/search?q=this-package-is-so-normal&filter=package http\" 200 114ms",
  "insertId": "REDACTED",
  "resource": {
    "type": "gae_app",
    "labels": {
      "version_id": "20221101t201412",
      "module_id": "default",
      "project_id": "pulsar-357404",
      "zone": "us6"
    }
  },
  "timestamp": "2022-11-10T16:15:10.790053Z",
  "labels": {
    "clone_id": "REDACTED"
  },
  "logName": "projects/pulsar-357404/logs/stdout",
  "receiveTimestamp": "2022-11-10T16:15:10.925939611Z"
}

This same query can be seen about 10 minutes later causing the same result.

This issue serves to track what endpoints have enough tests to consider complete written for them.
While more tests on a completed endpoint is never a bad thing, this can help use semi-determine the stability of the overall application, since these tests are full integration tests of the whole codebase.

Partial Progress: ⚒

• GET /
• GET /api/login
• GET /api/oauth
• GET /api/packages
• POST /api/packages
• GET /api/packages/featured
• GET /api/packages/search
• GET /api/packages/:packageName
• DELETE /api/packages/:packageName
• POST /api/packages/:packageName/star
• DELETE /api/packages/:packageName/star
• GET /api/packages/:packageName/stargazers
• POST /api/packages/:packageName/versions
• GET /api/packages/:packageName/versions/:versionName
• GET /api/packages/:packageName/versions/:versionName/tarball
• DELETE /api/packages/:packageName/versions/:versionName
• POST /api/packages/:packageName/versions/:versionName/events/uninstall
• GET /api/themes/featured
• GET /api/users/:login/stars
• GET /api/stars
• GET /api/updates - (Using Beta Version)

Important Note:

Some of the endpoints here being tested are fully written, and these tests will ensure they function as expected. Which won't always be the case, so having a failing test doesn't necessarily mean you've written it wrong. But then again some of these endpoints have not been fully written, or in some cases are so early in development that they will return a Not Supported message. Feel free to ask any questions during this process here, or on the Pulsar-Edit Discord

Is this feature Currently Implemented in any way with the upstream API Server

• Yes
• No

Is your feature request related to a problem? Please describe.
No.

Describe the solution you'd like
During search it currently uses Longest Common Subsequence Algorithm on the packages title only to determine the best search match.

It would be cool to see this possible consider other elements.

Increases Rank:

• Updated Recently
  • But not all good packages have recent activity, so it would have to be a low rank increase.
• Downloads
• Stars
• GitHub Stars

Decreases Rank:

• Security Issues?

Otherwise may be good to consider:

• Age of stars
• Age of downloads

@Digitalone1 to allow me to merge your PR, I'll go ahead and continue the conversation of the Search Engine here. Also making it available to any other contributors that may be concerned.

The idea for the current implementation for the search engine is that in ./src/database.js getPointerTable would return an array of every single package name, allowing us to pass this to our custom search engine, which can then provide ranking details on each, where we could then display each package details short to the user based on this ranking.

During development there have been several search algorithm implementations played around with in ./src/search.js but in the end the current one being used is determined by what's in ./app.yaml which is suggested to be lcs or Longest Common Subsequence Algorithm. This algorithm would take a singular name of the package, and the users search term and return an index of how close the two phrases match. A 1.0 (The highest score) would indicate a perfect match, and anything lower is then used as its ranking.

I was hoping to keep the custom search still usable rather than rely on built in search from PostgreSQL, since there are already some ideas to improve this search which those ideas are available on the Backend Project Board

But let me know if there's a better way you can think of to implement this that may improve performance

Now that it was possible to test with the full data set of packages, it seems the current design of the backend must change.

Initially on datasets of 100 packages or less, things seemed rather functional. But once within Cloud File Storage, the application was tested against the actual value of about 11,000 unique packages. At this point the backend became unreasonably slow. A request for all packages sorted and pruned would take <300 seconds, at which point the server hosting the backend, cut off the response due to assumed inoperability. This happens because the backend was trying to read each file one by one.

At this point it was obvious a file store wouldn't work, and there were hopes of moving the full JSON objects into an online Database.
This functionality was initially tested alongside full file store functionality, but even then, interacting with the DB as if it were a file system, while provided improvement, to 40 seconds reads of all packages, with cached reads about 8 ms. This still wasn't enough.

At this point it should seem obvious that huge portions of the codebase will have to be rewritten to work within a database, and abandon all file store functionality.

This goes out as an update to any that are contributing on the project, that while the efforts so far have been fantastic, one huge push is needed to get it worked to our expectations.

In the below code, verify that the 'prov' or provided value, is in fact a valid number, returning either as a string or integer.
Then feel free to remove the 'TODO'

https://github.com/confused-Techie/atom-community-server-backend-JS/blob/156ed2c85aed5f73717d945425a39a0357c9a606/src/query.js#L9-L21

See https://discord.com/channels/992103415163396136/992110062992621598/1046929553261613066 for initial confusion and wild assumptions.

Honestly not sure if this is for PPM or the backend but assume the backend because I imagine the same situation will arise if used for Atom.

Basically it seems that, whilst the backend technically isn't doing anything wrong, it isn't the expected response.

To replicate:

• In "Install packages" click the "Hydrogen" package to go to package details
• Pulsar will error:
  
  

Trying to install leads to a similar problem:

(ppm has similar responses, see the discord link at the top for @Spiker985's results).

It seems that Pulsar is requesting "Hydrogen" and not "hydrogen". You can see this if you click the Hydrogen title in the package install screen, it tries to go to:
https://web.pulsar-edit.dev/packages/Hydrogen which is a 404 and not https://web.pulsar-edit.dev/packages/hydrogen which is the actual package page.

Unsure if related but you can also see in the first image that the download count is missing.

From some very light digging it seems that in the API object it has name: hydrogen" but metadata.name: "Hydrogen. Not sure if this is just coincidence.

Currently there is a mix of both, a helper utility to handle user authentication, as well as handling it directly. While I love the benefit of less duplication, and less internal knowledge of authentication a helper function brings, I find the syntax very clunky and confusing.

If anyone has any thoughts on this I'd be happy to hear them, especially @Digitalone1

The raw/standard user auth handling:

let user = await users.VerifyAuth(params.auth);

if (!user.ok) {
  await common.AuthFail(req, res, user);
  return;
}
// code if user is logged in

The util function usage:

await utils.LocalUserLoggedIn(req, res, params.auth, async () => {
  // code if user is logged in
}