chore: remove restivus #34999

ggazzo · 2025-01-22T00:16:45Z

As part of the initiative to move rest to an external package, we must to remove dependencies related to meteor.
Restivus was a old library created to have access to webrouter, this is no longer the default, we have express exposed and its how we should create new endpoints.

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

dionisio-bot · 2025-01-22T00:16:48Z

Looks like this PR is not ready to merge, because of the following issues:

This PR is missing the 'stat: QA assured' label
This PR is targeting the wrong base branch. It should target 7.4.0, but it targets 7.3.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

changeset-bot · 2025-01-22T00:16:49Z

⚠️ No Changeset found

Latest commit: 72fcf02

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2025-01-22T00:22:56Z

PR Preview Action v1.6.0
🚀 View preview at https://RocketChat.github.io/Rocket.Chat/pr-preview/pr-34999/
Built to branch `gh-pages` at 2025-01-31 22:27 UTC. Preview will be ready when the GitHub Pages deployment is complete.

codecov · 2025-01-22T00:23:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.27%. Comparing base (271894f) to head (72fcf02).
Report is 34 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #34999      +/-   ##
===========================================
+ Coverage    59.17%   59.27%   +0.09%     
===========================================
  Files         2822     2824       +2     
  Lines        68069    68240     +171     
  Branches     15136    15143       +7     
===========================================
+ Hits         40283    40450     +167     
- Misses       24956    24958       +2     
- Partials      2830     2832       +2

Flag	Coverage Δ
unit	`75.17% <ø> (+0.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Ricchkdbanks

Please merge

KevLehman

Not sure if this happened before, but enabling Prometheus and going to localhost:9458 returns this "index" page.

Should we keep? Should we remove?

KevLehman · 2025-01-29T20:13:41Z

apps/meteor/app/integrations/server/api/api.js

+		request.integration = await Integrations.findOne({
+			_id: request.params.integrationId,
+			token: decodeURIComponent(request.params.token),
+		});


Should we have this one on model? Can be done later, a TODO may be enough.
But if we can do it on here :)

Not sure if I followed, do you mean create a 'getByIdAndToken' ? I dont think is related to the objetive of the pull request, this is 100% exactly how the previous code was doing

Yup, that's the idea. Agree on the oos, that's why I suggested the TODO. I'm tracking and fixing some of them.

apps/meteor/app/integrations/server/api/api.js

KevLehman · 2025-01-29T20:16:09Z

apps/meteor/app/livechat/imports/server/rest/departments.ts

@@ -87,6 +87,22 @@ API.v1.addRoute(
 	},
 );

+API.v1.addRoute(
+	'livechat/department/isDepartmentCreationAvailable',


So we have to be more worried about path collisions now :(

This is something that would be great to be shared somewhere so no one gets bitten by it.

why we should be more worried? to you have any extra explanation in mind? now at least we follow the same rules as express, it should be easy to find how it works

Oh, because we apparently used to support things like
path/:_id
path/something

Which express doesn't iirc.

apps/meteor/app/api/server/router.ts

Co-authored-by: Kevin Aleman <[email protected]>

ggazzo · 2025-01-29T21:13:50Z

Not sure if this happened before, but enabling Prometheus and going to localhost:9458 returns this "index" page.

Should we keep? Should we remove?

I think it is the exact same behavior :)

julio-rocketchat · 2025-01-29T23:18:55Z

This PR will definitely help with Iron Bank and with our vulnerability management as a whole. We have some issues related to Restivus since it's an old package that uses old dependencies.

apps/meteor/app/api/server/api.ts

ggazzo force-pushed the chore/remove-restivus branch 22 times, most recently from bd105b6 to 73a9722 Compare January 24, 2025 20:43

ggazzo added this to the 7.4.0 milestone Jan 24, 2025

ggazzo force-pushed the chore/remove-restivus branch 2 times, most recently from 1dee994 to 07336e0 Compare January 24, 2025 22:49

Ricchkdbanks reviewed Jan 24, 2025

View reviewed changes

ggazzo and others added 9 commits January 29, 2025 17:20

rawHandlers

0645a7d

register oauth

4ded709

..

ae65e1f

add testing

8d33d81

extract cors and test it

1f20f2e

rewrite metrics to middleware

65c5379

tracer and logger

8ddcea4

remove new router

3741fb7

Apply suggestions from code review

cd8051d

ggazzo force-pushed the chore/remove-restivus branch from a40a882 to cd8051d Compare January 29, 2025 20:20

KevLehman reviewed Jan 29, 2025

View reviewed changes

ggazzo and others added 3 commits January 29, 2025 17:42

adjust middlewares

4106111

Update apps/meteor/app/integrations/server/api/api.js

5ec7c8b

Co-authored-by: Kevin Aleman <[email protected]>

Update apps/meteor/app/api/server/router.ts

e7b5408

Co-authored-by: Kevin Aleman <[email protected]>

fix default router

1245eb6

remove remaining references to restivus

185b696

github-advanced-security bot found potential problems Jan 30, 2025

View reviewed changes

apps/meteor/app/api/server/api.ts Dismissed Show dismissed Hide dismissed

sampaiodiego and others added 4 commits January 30, 2025 13:39

fix 2fa

9543e3a

restore old cors behavior

2148b4a

handler CORS only on OPTIONS

c12b497

fix rate limiter

f015ebf

ggazzo force-pushed the chore/remove-restivus branch from fd8fb31 to 78b097a Compare January 31, 2025 21:26

type inner function

bc242f5

ggazzo force-pushed the chore/remove-restivus branch from 78b097a to bc242f5 Compare January 31, 2025 21:54

fix rate limiter

72fcf02

sampaiodiego approved these changes Jan 31, 2025

View reviewed changes

ggazzo merged commit 263a7cb into develop Jan 31, 2025
48 checks passed

ggazzo deleted the chore/remove-restivus branch January 31, 2025 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove restivus #34999

chore: remove restivus #34999

ggazzo commented Jan 22, 2025 •

edited

Loading

dionisio-bot bot commented Jan 22, 2025 •

edited

Loading

changeset-bot bot commented Jan 22, 2025 •

edited

Loading

github-actions bot commented Jan 22, 2025 •

edited

Loading

Built to branch `gh-pages` at 2025-01-31 22:27 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

codecov bot commented Jan 22, 2025 •

edited

Loading

Ricchkdbanks left a comment

KevLehman left a comment

KevLehman Jan 29, 2025

ggazzo Jan 29, 2025

KevLehman Jan 29, 2025

KevLehman Jan 29, 2025

ggazzo Jan 29, 2025

KevLehman Jan 29, 2025

ggazzo commented Jan 29, 2025

julio-rocketchat commented Jan 29, 2025

chore: remove restivus #34999

chore: remove restivus #34999

Conversation

ggazzo commented Jan 22, 2025 • edited Loading

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

dionisio-bot bot commented Jan 22, 2025 • edited Loading

changeset-bot bot commented Jan 22, 2025 • edited Loading

⚠️ No Changeset found

github-actions bot commented Jan 22, 2025 • edited Loading

Built to branch gh-pages at 2025-01-31 22:27 UTC. Preview will be ready when the GitHub Pages deployment is complete.

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

Ricchkdbanks left a comment

Choose a reason for hiding this comment

KevLehman left a comment

Choose a reason for hiding this comment

KevLehman Jan 29, 2025

Choose a reason for hiding this comment

ggazzo Jan 29, 2025

Choose a reason for hiding this comment

KevLehman Jan 29, 2025

Choose a reason for hiding this comment

KevLehman Jan 29, 2025

Choose a reason for hiding this comment

ggazzo Jan 29, 2025

Choose a reason for hiding this comment

KevLehman Jan 29, 2025

Choose a reason for hiding this comment

ggazzo commented Jan 29, 2025

julio-rocketchat commented Jan 29, 2025

ggazzo commented Jan 22, 2025 •

edited

Loading

dionisio-bot bot commented Jan 22, 2025 •

edited

Loading

changeset-bot bot commented Jan 22, 2025 •

edited

Loading

github-actions bot commented Jan 22, 2025 •

edited

Loading

Built to branch `gh-pages` at 2025-01-31 22:27 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

codecov bot commented Jan 22, 2025 •

edited

Loading