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

Different "starred" field format in REST API "dm.history" and "channels.messages" #12218

Closed
luciofm opened this issue Sep 30, 2018 · 5 comments
Closed

Comments

@luciofm
Copy link

luciofm commented Sep 30, 2018

Previously the the API would always return an JSON_ARRAY, even when there were just one user that starred a message.

In the latest RC version (0.70.0) this changed and now the API sends an JSON_OBJECT when there is only one star, and JSON_ARRAY when there is more than one.

Android can't handle this very well, we expect that a API field is ALWAYS the same type, always a String, or always a boolean, or always an array or an object, but never multiple types for the same field name.

    loadMissingMessages  D  Error fetching channel history
                         D  chat.rocket.common.RocketChatInvalidResponseException: Expected BEGIN_ARRAY but was BEGIN_OBJECT at path $.messages[0].starred(url='https://open.rocket.chat/api/v1/dm.history?roomId=*******&count=50&oldest=2018-09-21T15%3A21%3A49.646Z')
                         D      at chat.rocket.core.internal.rest.RestClientKt.handleResponse(RestClient.kt:76)
@rafaelks rafaelks changed the title REST API "history" methods changed "starred" format. [BUG] REST API "history" methods changed "starred" format. Oct 1, 2018
@rafaelks rafaelks changed the title [BUG] REST API "history" methods changed "starred" format. [REGRESSION] REST API "history" methods changed "starred" format. Oct 1, 2018
@rodrigok
Copy link
Member

rodrigok commented Oct 1, 2018

@luciofm the code that returns the messages was not changed since 2 years ago.

Can you show examples of the return of these 2 situations?

@rafaelks
Copy link
Contributor

rafaelks commented Oct 1, 2018

@rodrigok FYI he's talking about the REST API, not the WebSocket method.

RocketChat.API.v1.addRoute('channels.messages', { authRequired: true }, {
get() {
const findResult = findChannelByIdOrName({
params: this.requestParams(),
checkedArchived: false,
});
const { offset, count } = this.getPaginationItems();
const { sort, fields, query } = this.parseJsonQuery();
const ourQuery = Object.assign({}, query, { rid: findResult._id });
// Special check for the permissions
if (RocketChat.authz.hasPermission(this.userId, 'view-joined-room') && !RocketChat.models.Subscriptions.findOneByRoomIdAndUserId(findResult._id, this.userId, { fields: { _id: 1 } })) {
return RocketChat.API.v1.unauthorized();
}
if (!RocketChat.authz.hasPermission(this.userId, 'view-c-room')) {
return RocketChat.API.v1.unauthorized();
}
const cursor = RocketChat.models.Messages.find(ourQuery, {
sort: sort ? sort : { ts: -1 },
skip: offset,
limit: count,
fields,
});
const total = cursor.count();
const messages = cursor.fetch();
return RocketChat.API.v1.success({
messages,
count: messages.length,
offset,
total,
});
},
});

@MarcosSpessatto
Copy link
Contributor

@rafaelks the endpoint described above was another: /api/v1/dm.history

@rodrigok
Copy link
Member

rodrigok commented Oct 1, 2018

@rafaelks as @MarcosSpessatto said, Lucio posted a call for dm.history which calls that Meteor method.

The dm.history should always return an object, the REST channels.messages should always return an array, and the last message should always return an array.

The problem here is that we are not processing the starred array on that 2 places and we should since we should only return the user's stars, and the message contains the stars of all users who starred the message.

I already talked with @sampaiodiego and he will organize how to solve this.

@sampaiodiego sampaiodiego changed the title [REGRESSION] REST API "history" methods changed "starred" format. Different "starred" field format in REST API "dm.history" and "channels.messages" Oct 2, 2018
@sampaiodiego
Copy link
Member

sampaiodiego commented Oct 2, 2018

I've changed the issue title to reflect the issue.

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

No branches or pull requests

5 participants