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

[IMPROVEMENT] Compress JSON instead of sending it in a "nice" format #9057

Closed
rafaelks opened this issue Dec 8, 2017 · 10 comments · Fixed by #9068
Closed

[IMPROVEMENT] Compress JSON instead of sending it in a "nice" format #9057

rafaelks opened this issue Dec 8, 2017 · 10 comments · Fixed by #9068
Assignees
Milestone

Comments

@rafaelks
Copy link
Contributor

rafaelks commented Dec 8, 2017

Description:

Today the REST APIs are returning with spaces and line-breakers. We could remove everything and reduce the payload size with that.

Server Setup Information:

  • Version of Rocket.Chat Server: 0.60.0-develop

Steps to Reproduce:

  1. Request any REST API;

Expected behavior:

  • It should return compressed;

Example of commands API:

{"commands":[{"command":"invite-all-from","clientOnly":false},{"command":"slackbridge-import","clientOnly":false},{"command":"gimme","params":"your_message_optional","description":"Slash_Gimme_Description","clientOnly":false},{"command":"lennyface","params":"your_message_optional","description":"Slash_LennyFace_Description","clientOnly":false},{"command":"shrug","params":"your_message_optional","description":"Slash_Shrug_Description","clientOnly":false},{"command":"tableflip","params":"your_message_optional","description":"Slash_Tableflip_Description","clientOnly":false},{"command":"unflip","params":"your_message_optional","description":"Slash_TableUnflip_Description","clientOnly":false},{"command":"create","clientOnly":false},{"command":"help","clientOnly":false},{"command":"invite","clientOnly":false},{"command":"invite-all-to","clientOnly":false},{"command":"archive","clientOnly":false},{"command":"join","clientOnly":false},{"command":"kick","clientOnly":false},{"command":"leave","clientOnly":false},{"command":"part","clientOnly":false},{"command":"me","params":"your_message","description":"Displays_action_text","clientOnly":false},{"command":"msg","clientOnly":false},{"command":"mute","clientOnly":false},{"command":"unmute","clientOnly":false},{"command":"topic","params":"Slash_Topic_Params","description":"Slash_Topic_Description","clientOnly":false},{"command":"unarchive","clientOnly":false}],"offset":0,"count":22,"total":22,"success":true,"developerWarning":"[WARNING]: The \"usernames\" field has been removed for performance reasons. Please use the \"*.members\" endpoint to get a list of members/users in a room."}

Actual behavior:

{
  "commands": [
    {
      "command": "invite-all-from",
      "clientOnly": false
    },
    {
      "command": "slackbridge-import",
      "clientOnly": false
    },
    {
      "command": "gimme",
      "params": "your_message_optional",
      "description": "Slash_Gimme_Description",
      "clientOnly": false
    },
    {
      "command": "lennyface",
      "params": "your_message_optional",
      "description": "Slash_LennyFace_Description",
      "clientOnly": false
    },
    {
      "command": "shrug",
      "params": "your_message_optional",
      "description": "Slash_Shrug_Description",
      "clientOnly": false
    },
    {
      "command": "tableflip",
      "params": "your_message_optional",
      "description": "Slash_Tableflip_Description",
      "clientOnly": false
    },
    {
      "command": "unflip",
      "params": "your_message_optional",
      "description": "Slash_TableUnflip_Description",
      "clientOnly": false
    },
    {
      "command": "create",
      "clientOnly": false
    },
    {
      "command": "help",
      "clientOnly": false
    },
    {
      "command": "invite",
      "clientOnly": false
    },
    {
      "command": "invite-all-to",
      "clientOnly": false
    },
    {
      "command": "archive",
      "clientOnly": false
    },
    {
      "command": "join",
      "clientOnly": false
    },
    {
      "command": "kick",
      "clientOnly": false
    },
    {
      "command": "leave",
      "clientOnly": false
    },
    {
      "command": "part",
      "clientOnly": false
    },
    {
      "command": "me",
      "params": "your_message",
      "description": "Displays_action_text",
      "clientOnly": false
    },
    {
      "command": "msg",
      "clientOnly": false
    },
    {
      "command": "mute",
      "clientOnly": false
    },
    {
      "command": "unmute",
      "clientOnly": false
    },
    {
      "command": "topic",
      "params": "Slash_Topic_Params",
      "description": "Slash_Topic_Description",
      "clientOnly": false
    },
    {
      "command": "unarchive",
      "clientOnly": false
    }
  ],
  "offset": 0,
  "count": 22,
  "total": 22,
  "success": true,
  "developerWarning": "[WARNING]: The \"usernames\" field has been removed for performance reasons. Please use the \"*.members\" endpoint to get a list of members/users in a room."
}
@graywolf336
Copy link
Contributor

TODO: prettyJson = NODE_ENV !== 'production';

@anuardaher
Copy link

I would like to be assigned on this one.

@graywolf336
Copy link
Contributor

@anuardaher feel free to submit a pull request to add this functionality. 👍

@vibhorgupta-gh
Copy link

I'd like to work on this issue. Could we gZip the incoming JSON response to compress it down?

@graywolf336
Copy link
Contributor

No. We will leave the gzipping to the web server, such as caddy or ngnix or Apache.

But feel free to submit the pull request for what the issue here describes. 👍🏼

@vibhorgupta-gh
Copy link

Is the ngnix(/caddy/Apache) configuration already done in the repo? I couldn't pinpoint the code suggesting that.
Also, you mean that gzipping will be left to the server, the issue only requires, say, configuration of something like ngnix, and we're good to go?

@geekgonecrazy
Copy link
Contributor

You are over thinking it 😁

Basically the idea is turn pretty print off and let reverse proxies take care of compression. In the API declaration there is a property to enable pretty print. So just flipping it off if node_env=production is all that's needed :)

@JSzaszvari
Copy link
Contributor

@geekgonecrazy thank you. I wanted to say something but didn't as I'm pretty grumpy today.

as that song goes, "you took the words right out of my mouth"

@vibhorgupta-gh
Copy link

@geekgonecrazy Yep, I got that but I thought maybe I was oversimplifying it :P
So I was looking for API declaration in the code and failed to find any. Could you please direct me to the piece of code I might be missing?

@vibhorgupta-gh
Copy link

Nevermind, I figured it out. Here is a PR regarding the same.

Just about the time I referenced this PR, I noticed that @graywolf336 worked on the same issue as well :D #9070

@rodrigok rodrigok added this to the 0.60.0 milestone Dec 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants