[BUG] Unneeded warning in payload of REST API calls

[cardoso]

Happening on 0.60.0

https://unstable.rocket.chat/api/v1/info
https://unstable.rocket.chat/api/v1/service.configurations

{
  "info": {
    "version": "0.60.0-develop"
  },
  "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."
}

[rafaelks]

#9054 complements it.

[vibhorgupta-gh]

The reason this developerWarning pops up in API responses is that result.developerWarning property is specified in the success function, which fires upon a successful API call. A simple commenting out of the statement should suffice, but there is a catch.

// TODO: Remove this after three versions have been released. That means at 0.64 this should be gone. ;)

This statement right here rests just above the before mentioned line of code that could be commented out to resolve the issue. I am not exactly sure why this should be removed just yet when it says that it should be gone at 0.64, and the current version is just 0.60.

That is what I could gather from the situation, and here is a PR regarding the same #9072

[graywolf336]

Why the warning? Because the default field selector has changed and removes all of the usernames from any of the returned models from the database. So, technically this warning isn't unneeded as we, Rocket.Chat, don't have a great way to get the message out to people that something has changed which is possibly breaking to the consumers of the rest api. However, the way the message is sent could be improved.

• Don't send it if the NODE_ENV is something other than development
• Only send it on ones that it possibly has the chance of being applied to, however this is a huge list
• Don't send it at all if Rocket.Chat had a means of communicating (possibly) breaking changes.

So, that is why I closed your pull request @VibhorCodecianGupta as simply commenting it out is not the correct way to polish the warning. There is a reason that comment is placed there, as it was placed by me and you can see that if you use the git history of the file it is in.

[vibhorgupta-gh]

I see. That is what I suspected, apologies. I knew the comment was there for a reason, but now I get you that there needs to be a way to communicate that to the consumers of the api. The first bullet point seems like a sweet fix. May I implement this? @graywolf336

[graywolf336]

@VibhorCodecianGupta have you submitted a pull request for the first bullet point?

[vibhorgupta-gh]

Hello @graywolf336 , apologies for the late response. Something unavoidable came up. The mentioned PR implements the first bullet point, i.e the warning shows in the payload response only if NODE_ENV is development.
As for long-term solutions, a bot integration in Rocket.Chat to communicate this kind of breaking changes in apis or other changes may come in handy. For e.g., as you might know, Zulip has a commit tracking bot which posts information about pushed commits. Maybe something could be implemented on those lines?

The respective PR is #9213

[vibhorgupta-gh]

Hi, I was working on this issue @graywolf336 and requested a review too! Should I consider my PR closed?

[rodrigok]

Hi @VibhorCodecianGupta

I don't think that warning should show only for dev environments, people using the API from a production app will miss that warning.

A good addition would be a setting to disable warnings in API (off by default).

But your PR, as it is right now, is not what we consider correct.