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

fix: Some HTTP requests sent by apps don't have their data parsed into JSON #30560

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

d-gubert
Copy link
Member

@d-gubert d-gubert commented Oct 3, 2023

Proposed changes (including videos or screenshots)

Removes the validation in the jsonParser function in @rocket.chat/server-fetch package to allow the API to stringify the body of requests of any method, as long as the request has the content type "application/json"

This was the default behavior prior to adopting the package.

Issue(s)

Steps to test or reproduce

Further comments

The validation checking for either POST, PUT or DELETE request methods before stringifying the body of the request broke compatibility with the Apps-Engine API. Besides that, it would make sense for any request with the header Content-Type: application/json and a serializable object as body to have this object stringified - otherwise the request will be sent with a body of [object Object] - and stringifying the content of other methods (GET, HEAD, CONNECT, etc.) is not detrimental to the API in any way

@changeset-bot
Copy link

changeset-bot bot commented Oct 3, 2023

🦋 Changeset detected

Latest commit: 794f7ca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 31 packages
Name Type
@rocket.chat/server-fetch Patch
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/models Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

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

@d-gubert d-gubert marked this pull request as draft October 3, 2023 23:15
@d-gubert d-gubert changed the title Remove unnecessary method validation on the server http client fix: Remove unnecessary method validation on the server http client Oct 3, 2023
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #30560 (794f7ca) into develop (389bac8) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #30560      +/-   ##
===========================================
- Coverage    51.28%   51.24%   -0.04%     
===========================================
  Files          811      804       -7     
  Lines        15074    15109      +35     
  Branches      2752     2767      +15     
===========================================
+ Hits          7731     7743      +12     
- Misses        6935     6941       +6     
- Partials       408      425      +17     
Flag Coverage Δ
e2e 48.57% <ø> (+0.01%) ⬆️
unit 64.80% <ø> (ø)

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

@d-gubert d-gubert changed the title fix: Remove unnecessary method validation on the server http client fix: Allow data being JSON stringified for any HTTP method on server-fetch Oct 4, 2023
@d-gubert d-gubert changed the title fix: Allow data being JSON stringified for any HTTP method on server-fetch fix: Some HTTP requests sent by apps don't have their data parsed into JSON Oct 4, 2023
@d-gubert d-gubert marked this pull request as ready for review October 4, 2023 13:49
@d-gubert d-gubert requested a review from a team October 4, 2023 13:49
Copy link
Contributor

@geekgonecrazy geekgonecrazy left a comment

Choose a reason for hiding this comment

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

Would still like to understand why this check was even added?

Copy link
Contributor

@murtaza98 murtaza98 left a comment

Choose a reason for hiding this comment

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

Verified that this fix works with the help of Salesforce App...where we'd initially detected this issue

@murtaza98
Copy link
Contributor

Thanks @d-gubert 🙌

@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Oct 4, 2023
@ggazzo ggazzo added this to the 6.5.0 milestone Oct 9, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Oct 11, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Oct 12, 2023
@kodiakhq kodiakhq bot merged commit 3536342 into develop Oct 12, 2023
@kodiakhq kodiakhq bot deleted the fix/server-fetch-stringify branch October 12, 2023 23:05
gabriellsh added a commit that referenced this pull request Oct 16, 2023
…/mentionBot

* 'develop' of github.com:RocketChat/Rocket.Chat:
  feat: add tooltip to badge mentions (#30590)
  chore: improve `Tag` a11y link (#30636)
  refactor: Replace `useForm` in favor of RHF on `AppInstallPage` (#30634)
  fix: Improve FileProxy Handling, set Content-Type (#30427)
  refactor: `EditRoomInfo` to typescript (#28318)
  fix: mobile ringing notification missing call id (#30614)
  fix: Some HTTP requests sent by apps don't have their data parsed into JSON (#30560)
  test: More tests for groups kick (#30536)
  fix: Threads breaking after sending messages too fast (#30622)
  chore: Remove text decoration from room tag (#30606)
  i18n: Language update from LingoHub 🤖 on 2023-10-10Z (#30613)
  fix: File attachments have no content when exporting room messages as file (#30596)
  fix: use setImmediate to handle username update (#30500)
  chore: `AnalyticsReports` visual adequacy (#30617)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA skipped stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants