Skip to content

feat: Add OpenAPI Support to Statistics API #35692

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

Open
wants to merge 65 commits into
base: develop
Choose a base branch
from

Conversation

ahmed-n-abdeltwab
Copy link

@ahmed-n-abdeltwab ahmed-n-abdeltwab commented Apr 3, 2025

Description:
This PR integrates OpenAPI support into the Rocket.Chat API, migrate of Rocket.Chat API endpoints to the new OpenAPI pattern. The update includes improved API documentation, enhanced type safety, and response validation using AJV.

Key Changes:

  • Implement the OpenAPI pattern with better route handling.
  • Added AJV-based schema validation for API responses to ensure type safety.
  • Improved the code structure using chained route methods for better maintainability and readability.
  • No breaking changes

Issue Reference:
Relates to #34983, part of the ongoing OpenAPI integration effort.

Testing:

  • Validated that the endpoints return correctly formatted responses.
  • Verified AJV schema validation for the responses.
  • Ensured that the API documentation matches the updated response schema.
  • No breaking changes

Endpoints Migration:

  • statistics:
    • Get Last Statistics
    • Get Statistics List

Looking forward to your feedback! 🚀


Description

This pull request introduces OpenAPI support to the statistics API in the ahmed-n-abdeltwab/Rocket.Chat repository. The changes are made in the apps/meteor/app/api/server/v1/stats.ts file. The update involves refactoring the statistics API routes to incorporate typed validation using ajv schema definitions. This enhancement aims to improve type safety and request validation while preserving the existing functionality of the API. The changes are proposed from the feat/OpenAPI branch to be merged into the develop branch.

ahmed-n-abdeltwab and others added 10 commits April 2, 2025 06:36
Replaced nested ternary operators with a cleaner lookup-based approach
for better readability and maintainability. No functional changes.

Related to RocketChat#34983, but does not fully resolve it.
Replaced any type with unknown for the avoid of using any. No functional changes.
Replaced implicit any fallbacks with explicit unknown types.
No functional changes.
Related to RocketChat#34983, but does not fully resolve it.
Copy link
Contributor

dionisio-bot bot commented Apr 3, 2025

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

  • This PR is missing the 'stat: QA assured' label

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Apr 3, 2025

⚠️ No Changeset found

Latest commit: bd9be3a

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

@CLAassistant
Copy link

CLAassistant commented Apr 3, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ cardoso
✅ ggazzo
❌ ahmed-n-abdeltwab


ahmed-n-abdeltwab seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ahmed-n-abdeltwab ahmed-n-abdeltwab changed the title feat: Add OpenAPI Support to stats api (#34983) feat: Add OpenAPI Support to stats api Apr 3, 2025
@ahmed-n-abdeltwab
Copy link
Author

@kody start-review

Copy link

kodus-ai bot commented Apr 3, 2025

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the @kody start-review command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Security
Code Style
Kody Rules
Refactoring
Error Handling
Maintainability
Potential Issues
Documentation And Comments
Performance And Optimization
Breaking Changes

Access your configuration settings here.

@ahmed-n-abdeltwab
Copy link
Author

I usually run yarn lint -- --fix before committing, but this time, after running it, I noticed that it applied fixes across the entire project. As a result, when I added my file to Git and committed, it unintentionally included changes from the entire codebase.

To resolve this, I reverted the unintended changes and manually fixed only my file using the following command:

npx eslint --parser @typescript-eslint/parser --parser-options "project: ['./tsconfig.base.json']" apps/meteor/app/api/server/v1/stats.ts --fix

This ensured that only the intended file was fixed and committed correctly.
Should I report this as an issue, or should we wait to see if it happens again?

@ahmed-n-abdeltwab
Copy link
Author

@kody start-review

Copy link

kodus-ai bot commented Apr 3, 2025

Kody Review Complete

Great news! 🎉
No issues were found that match your current review configurations.

Keep up the excellent work! 🚀

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the @kody start-review command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Security
Code Style
Kody Rules
Refactoring
Error Handling
Maintainability
Potential Issues
Documentation And Comments
Performance And Optimization
Breaking Changes

Access your configuration settings here.

@ahmed-n-abdeltwab ahmed-n-abdeltwab requested a review from ggazzo April 3, 2025 15:32
@ahmed-n-abdeltwab
Copy link
Author

@kody start-review

@ahmed-n-abdeltwab
Copy link
Author

Thanks for your help! I'll leave the changes as they are and focus on fixing the errors. If I have questions about the scripts in the package.json file or need more help, I’ll reach out.

Thanks again

@ahmed-n-abdeltwab
Copy link
Author

I resolved the issue by adding 400 and 401 response schemas to each endpoint and correcting the query properties according to the documentation from the website. Here is the result after running yarn testapi -f 'statistics'.

[Statistics]
    [/statistics]
      ✔ should return an error when the user does not have the necessary permission (311ms)
      ✔ should return an object with the statistics (227ms)
      ✔ should update the statistics when is provided the "refresh:true" query parameter (4473ms)
    [/statistics.list]
      ✔ should return an error when the user does not have the necessary permission (195ms)
      ✔ should return an array with the statistics (230ms)
      ✔ should return an array with the statistics even requested with count and offset params (195ms)


  6 passing (6s)

@cardoso
Copy link
Member

cardoso commented Apr 23, 2025

Awesome! Thank you @ahmed-n-abdeltwab ! Just triggered the CI run

@ahmed-n-abdeltwab
Copy link
Author

You're welcome! Let me know if there’s anything else I can help with

@ahmed-n-abdeltwab
Copy link
Author

ahmed-n-abdeltwab commented Apr 24, 2025

📊 Add Response Schema for Get Last Statistics Endpoint

ggazzo
that's right always use additionalProperties: false

I added the Response Schema and additionalProperties: false for the Get Last Statistics endpoint.

While I was working on it, I found that the API docs are missing some fields or have wrong types. This made me spend time debugging and checking the code. Below are the problems I found from looking at the source code:

🐛 Problems I Found

🔗 I added links to the code to help you check each field.


✅ Test Results

After adding the schema and fixing the problems, I ran the tests and all of them passed:

$ yarn testapi -f 'statistics'

  [Statistics]
    [/statistics]
      ✔ should return an error when the user does not have the necessary permission (315ms)
      ✔ should return an object with the statistics (246ms)
      ✔ should update the statistics when is provided the "refresh:true" query parameter (273ms)
    [/statistics.list]
      ✔ should return an error when the user does not have the necessary permission (228ms)
      ✔ should return an array with the statistics (446ms)
      ✔ should return an array with the statistics even requested with count and offset params (232ms)

  6 passing (2s)

Looking forward to your feedback! 🚀

Copy link
Member

@cardoso cardoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ahmed-n-abdeltwab great job on the summary you just provided. 👏

Looks like you missed some linting errors:

If you can use VSCode and the eslint and prettier extensions, it should be easier to spot these before committing. If you need a lighter-weight editor maybe try zed.dev which has these built-in.

Otherwise, you can always run yarn lint.

Please do this before you push, since the CI won't run the test suite if it fails the lint phase.

I suggested the changes below, but as an exercise I recommend that you make them yourself instead of using github's 'Commit suggestion' button.

@ahmed-n-abdeltwab
Copy link
Author

Thanks a lot! I really appreciated your feedback.

I had already switched my editor to VSCode as you previously recommended, and I installed both the ESLint and Prettier extensions. They definitely helped me catch the linting issues more easily.

I will also run yarn lint before pushing and make the suggested changes manually instead of using the 'Commit suggestion' button.

@ahmed-n-abdeltwab
Copy link
Author

📊 fix: update comments for clarity

  • Updated comments with FIXME and TODO to clarify current issues
  • Ran Prettier and ESLint in VSCode for formatting and linting

🚧 Issues

  • yarn lint fails due to:
    • @rocket.chat/rest-typings module not found
    • ❌ ESLint cannot resolve the 'hono' module

Should I investigate this issue or leave it as is?

@cardoso
Copy link
Member

cardoso commented Apr 24, 2025

@ahmed-n-abdeltwab great job on the comments 👍

Don't worry about lint issues not related to the files you changed.

@ahmed-n-abdeltwab
Copy link
Author

ahmed-n-abdeltwab commented Apr 25, 2025

📊 Fix Response Schema for Get Last Statistics Endpoint

I used the IStats interface as a reference to create the schema manually. I thought that by doing so, I could safely use additionalProperties: false. But the result was different.

For example, my OS is Debian 12, and the returned statistics data includes properties that are not in IStats. It seems like the structure changes depending on the OS or environment — like a dynamic interface. Because of this, it's hard to create a static schema. So, for now, I’ve set all additionalProperties to true.


🐛 Issues That Caused AJV Validation to Fail

  • omnichannelSources: The interface says it's a number or string, but sometimes it returns undefined. I’ve hard coded it and left a comment for someone to review. code

  • totalLivechatRoomsWithPriority: The interface marks this field as required, but it’s actually optional in the response. This caused AJV validation to fail, so I commented it out in the required array. code

  • roomsInsideTeams and federationOverviewData: Same issue as above — they’re marked as required but not always present in the response.

  • lastMessageSentAt: is a JavaScript Date objects and undefined

  • lockedAt: is is a JavaScript Date objects and string.

🔗 I added links to the code to help you check each field.


✅ Test Results

$ yarn testapi -f 'statistics'

 [Statistics]
    [/statistics]
      ✔ should return an error when the user does not have the necessary permission (323ms)
      ✔ should return an object with the statistics (226ms)
      ✔ should update the statistics when is provided the "refresh:true" query parameter (754ms)
    [/statistics.list]
      ✔ should return an error when the user does not have the necessary permission (218ms)
      ✔ should return an array with the statistics (194ms)
      ✔ should return an array with the statistics even requested with count and offset params (339ms)

  6 passing (3s)

✅ Note

I also read this blog: Generating JSON Schema from TypeScript types. It explains a tool called ts-json-schema-generator that can generate JSON Schema directly from TypeScript types.

This tool could help us:

  • Avoid writing JSON Schemas manually
  • Keep validation in sync with our TypeScript types
  • Make future updates easier

If that sounds good, let me know what you think! 🙌

@ahmed-n-abdeltwab ahmed-n-abdeltwab changed the title feat: Add OpenAPI Support to stats api feat: Add OpenAPI Support to Rocket.Chat API Apr 26, 2025
@ahmed-n-abdeltwab
Copy link
Author

📊 Update Response Schema for 401 Get Last Statistics Endpoint

I added the status and message object to Response Schema of 401 for the Get Last Statistics endpoint.

no changes from the original docs. docs link


✅ Test Results

After updating the schema, I ran the tests and all of them passed:

$ yarn testapi -f 'statistics'

  [Statistics]
    [/statistics]
      ✔ should return an error when the user does not have the necessary permission (369ms)
      ✔ should return an object with the statistics (438ms)
      ✔ should update the statistics when is provided the "refresh:true" query parameter (3060ms)
    [/statistics.list]
      ✔ should return an error when the user does not have the necessary permission (297ms)
      ✔ should return an array with the statistics (350ms)
      ✔ should return an array with the statistics even requested with count and offset params (315ms)


  6 passing (7s)

@ahmed-n-abdeltwab ahmed-n-abdeltwab changed the title feat: Add OpenAPI Support to Rocket.Chat API feat: Add OpenAPI Support to Statistics API Apr 26, 2025
@ahmed-n-abdeltwab
Copy link
Author

ahmed-n-abdeltwab commented Apr 26, 2025

📊 Enhance Statistics Endpoints

  • Integrate Statistics Response Schema into the Get Statistics List endpoint.
  • Add 400 and 401 Response Schema to both Get Statistics List and Statistics Telemetry endpoints.

🐛 Problems I Found

The interface marks this properties as required, but it’s actually optional in the response. This caused AJV validation to fail, so I commented it out in the required array.

🔗 I added links to the code to help you check each field.


✅ Test Results

The tests were executed successfully, and all passed.

$ yarn testapi -f 'statistics'

  [Statistics]
    [/statistics]
      ✔ should return an error when the user does not have the necessary permission (350ms)
      ✔ should return an object with the statistics (254ms)
      ✔ should update the statistics when is provided the "refresh:true" query parameter (2364ms)
    [/statistics.list]
      ✔ should return an error when the user does not have the necessary permission (224ms)
      ✔ should return an array with the statistics (304ms)
      ✔ should return an array with the statistics even requested with count and offset params (207ms)


  6 passing (5s)

Looking forward to your feedback! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants