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

Discussion of switching GetContactPayload to SearchContactPayload #1397

Closed
windmemory opened this issue Jun 26, 2018 · 8 comments
Closed

Discussion of switching GetContactPayload to SearchContactPayload #1397

windmemory opened this issue Jun 26, 2018 · 8 comments
Labels

Comments

@windmemory
Copy link
Member

contactRawPayload in padchat-manager.ts is using WXGetContactPayload method to get the contact. Sometimes, the contactId passed to contactRawPayload might not be the friend of the user.

Per my investigation, when message.ts call ready method, it will also call ready method on the from and to Contact. If this message is from a room, the from might not be the friend of the bot, then ready in Contact will try to load the data for the contact, but since the contact is not a friend, with GetContactPayload, there will be no results for it. After that, this payload will be passed to contactRawPayloadParser, and the parser can not read out the user_name, thus cause breakage of the bot.

Do you think we should change the method in contactRawPayload method?

@windmemory
Copy link
Member Author

windmemory commented Jun 26, 2018

Related to #1358

@huan
Copy link
Member

huan commented Jun 27, 2018

Yes, Message needs the sender contact to be ready-ed, which means that Contact must be able to load the ContactPayload for itself.

Because in PuppetPadchat it will fail when getting a contact payload for a stranger in the room, so your discussion is very valuable for us to deal with the Pachat protocol well.

My suggestion would be:

  1. Yes, you can try SearchContact, and to see if it could solve this problem. But to my understanding of this API(of the name search), I don't think it would be the solution for us;
  2. Consider to load the sender payload from roomMemberPayload, which I feels should be the best data source for the room members.

https://github.com/Chatie/wechaty/blob/1de61f430f381905fc486750be6ac23c429b4eed/src/puppet-padchat/padchat-manager.ts#L750-L758

@huan huan added the question label Jun 27, 2018
@lijiarui
Copy link
Member

@windmemory I change my code as follows here in padchat-rpc.ts, when get undefined user_name then change to WXSearchContact()

  /**
   * Get contact by contact id
   * @param {any} id        user_name
   */
  public async WXGetContactPayload(id: string): Promise<PadchatContactPayload> {
    if (!isContactId(id)) { // /@chatroom$/.test(id)) {
      throw Error(`should use WXGetRoomPayload because get a room id :${id}`)
    }
    let result = await this.WXGetContact(id) as PadchatContactPayload

    if (!result.user_name) {
      result = await this.WXSearchContact(id) as PadchatContactPayload
    }

    return result
  }

Well, the API is blocked... Related log as follows:

01:45:53 INFO PadchatRpc WXSearchContact wxid: wxid_pmct091suwyt12 cannot be searched
01:45:53 SILL PuppetPadchat contactRawPayloadParser({user_name=""})
01:45:53 ERR Contact ready() this.puppet.contactPayload(Contact) exception: cannot get user_name from raw payload: {"big_head":"","city":"","country":"","message":"\n�\u0002<e>\n<ShowType>1</ShowType>\n<Content><![CDATA[找不到相关帐号或内容]]></Content>\n<Url><![CDATA[]]></Url>\n<DispSec>30</DispSec>\n<Title><![CDATA[]]></Title>\n<Action>2</Action>\n<DelayConnSec>0</DelayConnSec>\n<Countdown>0</Countdown>\n<Ok><![CDATA[]]></Ok>\n<Cancel><![CDATA[]]></Cancel>\n</e>\n","nick_name":"","provincia":"","py_initial":"","quan_pin":"","sex":0,"signature":"","small_head":"","status":-24,"stranger":"","user_name":""}
01:45:53 SILL PadchatRpc WXSearchContact result: {"big_head":"","city":"","country":"","message":"\n�\u0002<e>\n<ShowType>1</ShowType>\n<Content><![CDATA[找不到相关帐号或内容]]></Content>\n<Url><![CDATA[]]></Url>\n<DispSec>30</DispSec>\n<Title><![CDATA[]]></Title>\n<Action>2</Action>\n<DelayConnSec>0</DelayConnSec>\n<Countdown>0</Countdown>\n<Ok><![CDATA[]]></Ok>\n<Cancel><![CDATA[]]></Cancel>\n</e>\n","nick_name":"","provincia":"","py_initial":"","quan_pin":"","sex":0,"signature":"","small_head":"","status":-24,"stranger":"","user_name":""}
01:45:53 INFO PadchatRpc WXSearchContact wxid: wxid_o9yzflim2f0n12 cannot be searched
01:45:53 SILL PuppetPadchat contactRawPayloadParser({user_name=""})
01:45:53 ERR Contact ready() this.puppet.contactPayload(Contact) exception: cannot get user_name from raw payload: {"big_head":"","city":"","country":"","message":"\n�\u0002<e>\n<ShowType>1</ShowType>\n<Content><![CDATA[找不到相关帐号或内容]]></Content>\n<Url><![CDATA[]]></Url>\n<DispSec>30</DispSec>\n<Title><![CDATA[]]></Title>\n<Action>2</Action>\n<DelayConnSec>0</DelayConnSec>\n<Countdown>0</Countdown>\n<Ok><![CDATA[]]></Ok>\n<Cancel><![CDATA[]]></Cancel>\n</e>\n","nick_name":"","provincia":"","py_initial":"","quan_pin":"","sex":0,"signature":"","small_head":"","status":-24,"stranger":"","user_name":""}
01:45:53 SILL PadchatRpc WXSearchContact result: {"big_head":"","city":"","country":"","message":"\n�\u0002<e>\n<ShowType>1</ShowType>\n<Content><![CDATA[操作过于频繁,请稍后再试]]></Content>\n<Url><![CDATA[]]></Url>\n<DispSec>30</DispSec>\n<Title><![CDATA[]]></Title>\n<Action>2</Action>\n<DelayConnSec>0</DelayConnSec>\n<Countdown>0</Countdown>\n<Ok><![CDATA[]]></Ok>\n<Cancel><![CDATA[]]></Cancel>\n</e>\n","nick_name":"","provincia":"","py_initial":"","quan_pin":"","sex":0,"signature":"","small_head":"","status":-24,"stranger":"","user_name":""}
01:45:53 INFO PadchatRpc WXSearchContact wxid: yyf2010555 cannot be searched
01:45:53 SILL PuppetPadchat contactRawPayloadParser({user_name=""})
01:45:53 ERR Contact ready() this.puppet.contactPayload(Contact) exception: cannot get user_name from raw payload: {"big_head":"","city":"","country":"","message":"\n�\u0002<e>\n<ShowType>1</ShowType>\n<Content><![CDATA[操作过于频繁,请稍后再试]]></Content>\n<Url><![CDATA[]]></Url>\n<DispSec>30</DispSec>\n<Title><![CDATA[]]></Title>\n<Action>2</Action>\n<DelayConnSec>0</DelayConnSec>\n<Countdown>0</Countdown>\n<Ok><![CDATA[]]></Ok>\n<Cancel><![CDATA[]]></Cancel>\n</e>\n","nick_name":"","provincia":"","py_initial":"","quan_pin":"","sex":0,"signature":"","small_head":"","status":-24,"stranger":"","user_name":""}
01:45:53 SILL PadchatRpc WXSearchContact result: {"big_head":"","city":"","country":"","message":"\n�\u0002<e>\n<ShowType>1</ShowType>\n<Content><![CDATA[操作过于频繁,请稍后再试]]></Content>\n<Url><![CDATA[]]></Url>\n<DispSec>30</DispSec>\n<Title><![CDATA[]]></Title>\n<Action>2</Action>\n<DelayConnSec>0</DelayConnSec>\n<Countdown>0</Countdown>\n<Ok><![CDATA[]]></Ok>\n<Cancel><![CDATA[]]></Cancel>\n</e>\n","nick_name":"","provincia":"","py_initial":"","quan_pin":"","sex":0,"signature":"","small_head":"","status":-24,"stranger":"","user_name":""}
01:45:53 INFO PadchatRpc WXSearchContact wxid: wxid_hjjqix6dojdr12 cannot be searched
01:45:53 SILL PuppetPadchat contactRawPayloadParser({user_name=""})
01:45:53 ERR Contact ready() this.puppet.contactPayload(Contact) exception: cannot get user_name from raw payload: {"big_head":"","city":"","country":"","message":"\n�\u0002<e>\n<ShowType>1</ShowType>\n<Content><![CDATA[操作过于频繁,请稍后再试]]></Content>\n<Url><![CDATA[]]></Url>\n<DispSec>30</DispSec>\n<Title><![CDATA[]]></Title>\n<Action>2</Action>\n<DelayConnSec>0</DelayConnSec>\n<Countdown>0</Countdown>\n<Ok><![CDATA[]]></Ok>\n<Cancel><![CDATA[]]></Cancel>\n</e>\n","nick_name":"","provincia":"","py_initial":"","quan_pin":"","sex":0,"signature":"","small_head":"","status":-24,"stranger":"","user_name":""}
01:45:53 SILL PadchatRpc WXSearchContact result: {"big_head":"","city":"","country":"","message":"\n�\u0002<e>\n<ShowType>1</ShowType>\n<Content><![CDATA[操作过于频繁,请稍后再试]]></Content>\n<Url><![CDATA[]]></Url>\n<DispSec>30</DispSec>\n<Title><![CDATA[]]></Title>\n<Action>2</Action>\n<DelayConnSec>0</DelayConnSec>\n<Countdown>0</Countdown>\n<Ok><![CDATA[]]></Ok>\n<Cancel><![CDATA[]]></Cancel>\n</e>\n","nick_name":"","provincia":"","py_initial":"","quan_pin":"","sex":0,"signature":"","small_head":"","status":-24,"stranger":"","user_name":""}
01:45:53 INFO PadchatRpc WXSearchContact wxid: wxid_v6hwv4tc5na921 cannot be searched
01:45:53 SILL PuppetPadchat contactRawPayloadParser({user_name=""})

Maybe this is because WXSearchContact() is related to add a friend, this should not be used to frequently. But WXGetContact() can be called frequently more than WXSearchContact().

So maybe we shouldn't change WXGetContact to WXSearchContact...

@windmemory
Copy link
Member Author

Thanks @lijiarui for your investigation. You make it easy to answer @zixia 's comment. We need to load the data from roomMember instead of contact.

Then I got another question. I think @zixia could answer this well.

I think it's better to keep align with the original design so we don't break anyone.

What's the original design for room member? Are these information also supposed to be stored in Contact? If so, we need to fill the member data into Contact when we sync the room member, otherwise, we should change the find logic here for the room member.

@huan
Copy link
Member

huan commented Jun 29, 2018

In Wechaty, the original design for room member is to get the room related information for the contact, for example, the room alias.

So that information is all stored in the Contact, exception the room alias.

Please see the related commits and I had made today, and feel free to let me know if you have any suggestions/questions, and any further improvements are welcome.

@windmemory
Copy link
Member Author

windmemory commented Jun 30, 2018

After reading code carefully, I find that there are two places that writes the contact information to cache.
One is here in SyncContactsAndRooms
https://github.com/Chatie/wechaty/blob/d89f77f4bb98c83f4b91c8c419d4a154014a3f41/src/puppet-padchat/padchat-manager.ts#L883-L895
The other one is in contactRawPayload
https://github.com/Chatie/wechaty/blob/d89f77f4bb98c83f4b91c8c419d4a154014a3f41/src/puppet-padchat/padchat-manager.ts#L947-L954

Code in SyncContactsAndRooms seems to be safe and will always write valid data into cache, but the code in contactRawPayload is not safe. There was a check before we writes data into cache, but got commented out after. I think we should always check the payload before we set the payload into the cache. At least check the payload is not empty.

Besides, seems like the contactRawPayload is the deepest datasource in padchat for contact, so I wonder whether it makes more sense to move the logic of checking roomMemberList from puppet-padchat to this method?

If this makes sense to you @zixia , I will submit a PR later. Let me know what do you think.

@huan
Copy link
Member

huan commented Jul 1, 2018

Thank you for reading code carefully.

I believe we should create a new issue because the discussion right now is not related to the current title of SearchContactPayload.

And also please notice that I had moved all the code base related to the wechaty-puppet-padchat to it's new home at https://github.com/lijiarui/wechaty-puppet-padchat, all the future development and discussion should be moved over there.

About the PR: Please feel free to submit PR at https://github.com/lijiarui/wechaty-puppet-padchat instead here, because all the code related to the padchat had been frozen in this repository to prevent source code inconsistent. Make sure you had tested it locally, and submit with unit tests will be recommend.

@windmemory
Copy link
Member Author

Cool, I will close this issue.

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

No branches or pull requests

3 participants