Server re-build with latest AdonisJS framework & Typescript #47
Conversation
vraravam
force-pushed
the
v2-rebuild
branch
9 times, most recently
from
4491a7b
to
59e7f16
Compare
|
Hey @cino, I had some available time and pushed 4 new commits to the server overhaul. All dependencies are up-to-date now, no more CVEs are reported in prod code, no TS/ESLint issues and made sure that all adonis commands work as expected. I think this migration is moving to be in a great state, but ofc most of the routes have to be rewritten still. Let's see, maybe some other Ferdium contributors want to jump in and help as well, this is probably one of the biggest tech debt we have to solve. |
|
Hi all! I've been trying to learn a bit of AdonisJS so I can push this PR forward. So far I was able to update the following endpoints:
Tested all 3 with Ferdium and they were all looking fine. Let me know what you think (pushing bellow). Important security note: I'm using the API token guard of adonis auth for handling API authentication, which is different from the JWT we used to have (now it is not supported natively by adonis). I've set an expiration of 7 days, but let me know what you think about this. For this to work I had to add a new migration to introduce two more columns in the Tokens table. Tomorrow, hopefully, I'll start with the Service or Workspace endpoints x) @cino you were tagged on the Login and Signup endpoints - if you could review my work I would really apreciate it :D Thank you! |
|
Hey again! I just refactored the code to use JWT (so that we don't have to change anything on the Ferdium Client to accomodate that change - that is something we can do in the future, if needed). I've also finished porting all the API endpoints!! 😄 But I'll be glad if someone could help me test them so that we can validate what I've done. The majority of them are working properly (tested with Ferdium Client), but some specific ones I havent tested (check the TODOS in the files) @ferdium/reviewers I'm pinging everyone in hope that someone can review my code, given that this substantialy pushes this PR forward. @cino, sorry to tag you again, but I'm trying to test the web endpoints and I don't seem to be able to past the initial page (no access to login, or dashboad). All I get is: ... or if I click on the button to go to dashboard: |
| @column() | ||
| public name: string; | ||
|
|
||
| @column.dateTime() | ||
| public expires_at: DateTime; |
There was a problem hiding this comment.
Why did you add these new fields? How are they going to be backwards compatible?
There was a problem hiding this comment.
That is a great question. I'm not entirely sure if it is ok to add them like so or if we need to provide a @default value. I've never used Adonis before so I'm not sure how to handle it.
If you test the auth jwt or even the api you will noticed that this 2 fields were added in the Adonis upgrade.
Generally, we have a bigger problem than this... I've tried to test the db in production and it fails to even initiate migrations (even without this new migration I added). We will probably need to code a script to migrate old databases to the new database schema manually
| emailValidated: true, | ||
| features: {}, | ||
| firstname: auth.user.username, | ||
| id: '82c1cf9d-ab58-4da2-b55e-aaa41d2142d8', |
There was a problem hiding this comment.
This is the same hardcoded id for every user?
There was a problem hiding this comment.
Copy-pasted from the previous code.... It was strange to me as well xD
| isPremium: true, | ||
| isSubscriptionOwner: true, |
There was a problem hiding this comment.
We might want to drop these fields since Ferdium is fully OSS, but probably better to split it off in a PR that would happen after this one.
There was a problem hiding this comment.
I think this is here to circunvent the fact that we are OSS. Forcing features that depend on those fields to be set to true on the Ferdium-App
| emailValidated: true, | ||
| features: {}, | ||
| firstname: auth.user.username, | ||
| id: '82c1cf9d-ab58-4da2-b55e-aaa41d2142d8', |
| .length > 0 | ||
| ); // eslint-disable-line no-await-in-loop | ||
| // eslint-disable-next-line no-await-in-loop, unicorn/no-await-expression-member | ||
| (await Service.query().where('serviceId', serviceId)).length > 0 |
There was a problem hiding this comment.
Does this code even make sense? It is querying for a service but then the result of the call is not assigned to any variable?
There was a problem hiding this comment.
This is inside a while block. so it will run while it is > 0 (don't like it either but lets make improvements after we merge this to codebase)
SpecialAro
changed the title
|
Amazing work! 🎉 I noticed a couple of improvement possiblities:
|
Thank you for your comment and review @mcmxcdev. Indeed I haven't focused too much on tests up until now. I've pushed some commits that fix the majority of the fails but I still have to fix the tests with the transfer endpoint because it seems that the endpoint data is being seen as body json (if you look at the controller) and not so much as file - so those tests are not up to date. I'm commenting them out for now if you agree (later we can improve tests) |
I apologize for the size of this PR but as we discussed earlier on Discord it's impossible to upgrade the dependencies without heavily restructuring the project. For anyone wanting to have a look at the code please open this in an editor or make use of github.dev by pressing the . (dot) on your keyboard.
So for now, this is a WIP Pull request so anyone can contribute and see where we are at the moment.
There are a few rules I've committed to while doing this "v2":
Other improvements:
Routes:
Dashboard (Conditional, only if enabled)
API:
GitHub
General:
This also closes #70