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] 'Query' support for channels.list.joined, groups.list, groups.listAll, im.list #9424

Merged
merged 5 commits into from
Feb 17, 2018

Conversation

xbolshe
Copy link
Contributor

@xbolshe xbolshe commented Jan 17, 2018

@RocketChat/core

Closes #9423

New in the database: customFields is added to Subscription to get it from RocketChat.models.Subscriptions.find(ourQuery).fetch().

default

Example after this fix : get all joined channels with customFields.field1 = "aaa-2"

GET http://192.168.2.71:3000/api/v1/channels.list.joined?query=%7B%20%22customFields.field1%22%3A%20%22aaa-2%22%7D HTTP/1.1
Accept-Encoding: gzip,deflate
X-Auth-Token: KfzSC4VlWkhNtcrwvn4SYvSfaia53hyMbNWIcNoCPRu
X-User-Id: EuGBBA3athDSt4Agt
Host: 192.168.2.71:3000
Connection: Keep-Alive
User-Agent: Apache-HttpClient/4.1.1 (java 1.5)

HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
X-Instance-ID: E3v8uoR8QQRQ8JsDc
Cache-Control: no-store
Pragma: no-cache
content-type: application/json
Vary: Accept-Encoding
Content-Encoding: gzip
Date: Wed, 17 Jan 2018 00:34:12 GMT
Connection: keep-alive
Transfer-Encoding: chunked

{"channels":[{"name":"channel-2","fname":"channel-2","t":"c","msgs":0,"u":{"_id":"EuGBBA3athDSt4Agt","username":"admin"},"customFields":{"field1":"aaa-2"},"ts":"2018-01-17T00:06:12.976Z","ro":false,"sysMes":true,"_updatedAt":"2018-01-17T00:06:12.977Z","_id":"8DvdRrByAh5bnSRTk"}],"offset":0,"count":1,"total":1,"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."}

…groups.listAll, im.list

Signed-off-by: Eugene Bolshakov <pub@relvarsoft.com>
Signed-off-by: Eugene Bolshakov <pub@relvarsoft.com>
@rodrigok
Copy link
Member

It looks interesting, @graywolf336 what do you think?

Signed-off-by: Eugene Bolshakov <pub@relvarsoft.com>
@xbolshe
Copy link
Contributor Author

xbolshe commented Jan 17, 2018

The last change allows to add customFields for IM (Direct Messages). The previously changed im.list now allows to search using 'query' like { "customFields.field1": { "$regex": "^org"}} as shown below:

GET http://192.168.2.71:3000/api/v1/im.list?query=%7B%20%22customFields.field1%22%3A%20%7B%20%22%24regex%22%3A%20%22%5Eorg%22%7D%7D HTTP/1.1
X-Auth-Token: XQXeGgIxrIrC4woJRQkqA3exK_guLdKTBkkRhtaDI3G
X-User-Id: hkH4KBTqACQexyJsM
Host: 192.168.2.71:3000
Connection: Keep-Alive
User-Agent: Apache-HttpClient/4.1.1 (java 1.5)

HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
X-Instance-ID: DZgju92h5LLb5DBAE
Cache-Control: no-store
Pragma: no-cache
content-type: application/json
Vary: Accept-Encoding
Date: Wed, 17 Jan 2018 21:45:41 GMT
Connection: keep-alive
Transfer-Encoding: chunked

{"ims":[{"_id":"hkH4KBTqACQexyJsMrocket.cat","_updatedAt":"2018-01-17T21:42:17.884Z","t":"d","msgs":0,"ts":"2018-01-17T21:42:17.884Z","username":"user2"}],"offset":0,"count":1,"total":1,"success":true}

Need to note that:

  • customFields value for IM/DM Room is taken from user's customFields
  • customFields value for Channel/Private Group is taken from Channel/Group's customFields

@xbolshe
Copy link
Contributor Author

xbolshe commented Jan 17, 2018

For Direct Messages a database record looks like shown below:
default

Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

Looks good. If you don't mind, when this gets merged, can you submit a pull request to our Documentation repo to update the docs for these methods?

@rodrigok
Copy link
Member

@xbolshe what happens when the value of the user's custom fields change?

@xbolshe
Copy link
Contributor Author

xbolshe commented Jan 18, 2018

@graywolf336 Sure. I can update Documentation repo with changes of this PR and usage examples.
@rodrigok I guess new custom fields' values will not be applied to existed IM Rooms. I'll try to prepare changes for 'update' methods.

Signed-off-by: Eugene Bolshakov <pub@relvarsoft.com>
@xbolshe
Copy link
Contributor Author

xbolshe commented Jan 22, 2018

@graywolf336 Docs PR is added (see a link above).
@rodrigok Fix for the user's custom fields change is added.

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.

@graywolf336 @rodrigok do we want to open this up to any query? Or should this be limited in some way?

'u._id': this.userId
});

let rooms = _.pluck(RocketChat.models.Subscriptions.find(ourQuery).fetch(), '_room');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to just blindly pass query through?

Copy link
Contributor

Choose a reason for hiding this comment

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

@geekgonecrazy it's not exactly passing the query blindly. The function parseJsonQuery handles several cases and line 410 ensures that the calling user is only able to query their own...although I say that without trying it. @xbolshe what happens whenever someone passes the query with a nested property like the one the string u._id is doing? For example, { "u": { "_id": "other_users_id" } }?

Copy link
Contributor Author

@xbolshe xbolshe Jan 23, 2018

Choose a reason for hiding this comment

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

@graywolf336 One string and nested string will have different actions as described here: https://stackoverflow.com/questions/16002659/how-to-query-nested-objects
Becase "u" has several fields inside, only nested "_id" will find no records at all:

default

But one string provides a records selection:

default

So, need to use all fields nested inside "u" like shown below:

default

Providing 2 different values for one field like "u._id" will cause a search of no records:

default

Answering on your question, one field cannot be equal at the same time to 2 different values.

If a value will be duplicated like shown below, a search will be performed:

default

So, only records belongs to this.userId will be searched for channels.list.joined.
I have tried to inherit constraints like this.userId applied before my changes.

Copy link
Contributor Author

@xbolshe xbolshe Jan 23, 2018

Choose a reason for hiding this comment

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

BTW: the last constraint's value overwrites the previous one:
default

default

So, 'u._id': this.userId shall be after 'query' as it's done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for looking into it in more depth! 👍

@engelgabriel engelgabriel added this to the 0.62.0 milestone Jan 30, 2018
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9424 February 15, 2018 14:49 Inactive
@rodrigok rodrigok merged commit 490fa2d into RocketChat:develop Feb 17, 2018
@rodrigok rodrigok mentioned this pull request Feb 28, 2018
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.

'Query' field does not work with channels.list.joined, groups.list, groups.listAll, im.list
5 participants