No contactPayloadDirty method in puppet-implementation.

[su-chang]

When wechaty call method contactPayloadDirty, the wechaty-puppet-hostie can not match this method to wechaty-puppet.

Related issue: wechaty/wechaty#1964

[huan]

Could you explain it in details with some code snip example, so that it can help me to understand it well?

[su-chang]

public async alias (newAlias?: null | string): Promise<null | string | void> {
  ...
  await this.puppet.contactAlias(this.id, newAlias)
  await this.puppet.contactPayloadDirty(this.id)
  this.payload = await this.puppet.contactPayload(this.id)
  ...
}

wechaty-puppet-hostie need to call method contactPayloadDirty(), but maybe this method missed.

[huan]

1. Can we call this method in other puppet implementations, like padplus?
2. Can you confirm whether we have this method contactPayloadDirty in our abstract class?

[su-chang]

This method is not an abstract method in wechaty-puppet.

public async contactPayloadDirty (contactId: string): Promise<void> {
  log.verbose('Puppet', 'contactPayloadDirty(%s)', contactId)
  this.cacheContactPayload.delete(contactId)
}

I override it in wechaty-puppet-donut.

public async contactPayloadDirty (contactId: string): Promise<void> {
  log.verbose(PRE, 'contactPayloadDirty(%s)', contactId)

  if (this.bridge && this.bridge.cacheManager) {
    await this.bridge.cacheManager.deleteContact(contactId)
  }

  await super.contactPayloadDirty(contactId)
}

I can not find this method been called in my log. It is the reason of this issue.

I want to know does there need to add something in wechaty-puppet-hostie for this method.

[huan]

contactPayloadDirty() is a method instead of an abstract method in our wechaty-puppet because it needs to be called as a support method for others.

I noticed that you said you have overridden it in wechaty-puppet-donut, could you please explain the reason of why you did that?

I don't think you should do that because the following two reasons:

1. contactPayloadDirty() is in charge of update the cache in puppet cache. If you override it, it will not be able to update the cache in the puppet anymore, which I believe will cause problems.
2. If you need to update your cache in your bridge.cacheManager, it is your duty to do that in other places, but not override the contactPayloadDirty in puppet.

So my suggestion would be:

1. Do not override the contactPayloadDirty() anymore, just call it when you need to update the cache in puppet.
2. You do not need to add anything in wechaty-puppet-hostie for this method, just call it as you need.

[su-chang]

Thanks for you response. I understand, there's no need to override the contactPayloadDirty() anymore.

[huan]

You are welcome.

And one more thing: we should keep in mind that the existing methods in the wechaty-puppet are required to work for internal usage, so it will be better to never override them, except that we have a very strong reason to do that.

TL;DR: only implement the required methods for a wechaty-puppet, and put all other logics in other classes: Always keep the wechaty-puppet-xxx clean, simple, and stupid.

[su-chang]

alias() and sync() are the same to call these two methods in Contact:

await this.puppet.contactPayloadDirty(this.id)
this.payload = await this.puppet.contactPayload(this.id)

For sync the updated contact payload, it seems failed when I want to sync() or alias() one contact. I get the same contact payload of contact each time until io-client restart.

After I add method await this.puppet.contactPayloadDirty(id) to contactPayload() method in puppet-implementation.ts, I can get the updated contact payload each time.

contactPayload: async (call, callback) => {
  log.verbose('PuppetServiceImpl', 'contactPayload()')

  const id = call.request.getId()

  try {
    await this.puppet.contactPayloadDirty(id) // here
    const payload = await puppet.contactPayload(id)

    const response = new ContactPayloadResponse()
    response.setAddress(payload.address || '')
    response.setAlias(payload.alias || '')
    response.setAvatar(payload.avatar)
    response.setCity(payload.city || '')
    response.setFriend(payload.friend || false)
    response.setGender(payload.gender)
    response.setId(payload.id)
    response.setName(payload.name)
    response.setProvince(payload.province || '')
    response.setSignature(payload.signature || '')
    response.setStar(payload.star || false)
    response.setType(payload.type)
    response.setWeixin(payload.weixin || '')

    return callback(null, response)
  } catch (e) {
    return grpcError('contactPayload', e, callback)
  }
},

Question
I do not know why the method await this.puppet.contactPayloadDirty(this.id) does not work when I call contact.alias() or contact.sync().

Reproduce Step

1. contact.sync()
2. modify contact alias in WeChat App or call API do that
3. contact.sync() again

[huan]

I believe your contactPayloadDirty() does not work is because you have overridden it?

Try to remove your override method, rebuild and try again, I guess it might be able to work.

[su-chang]

Nope, it is not due to this problem, I have checked it just now. It still does not work.

[windmemory]

I went through the code, and I think I understand what happened here.

I believe this is our current structure of wechaty-puppet-hostie. And @su-chang is working on the wechaty-puppet-xx. Now with his code in wechaty-puppet-xx, he could dirty the cache1, but when using Wechaty with hostie, it will read cache2 first, then read the cache1. However in some cases, like alias change event, he cleared the cache1, the cache2 still can not be cleared, which causing his problem now. Is there a way for underlying puppet to clear hostie cache?

[huan]

@windmemory thank you very much for your great explaining with the clear picture, I agree with your analytics that the double layer of the cache is the reason is this problem.

Let's think about how to design a way for underlying puppet to clear hostie cache.

[su-chang]

After I add method await this.puppet.contactPayloadDirty(id) to contactPayload() method in puppet-implementation.ts, I can get the updated contact payload each time.

Shall use this way as work around before the new design for underlying puppet to clear hostie cache? Willing to hear your suggestions. @huan @windmemory

[huan]

I have a very naive design to solve this problem, draft it as follows:

1. we add a dirty event for the puppet
2. if a puppet receives the dirty event, it should:
   1. call onDirtyXXX() to clean the cache according to the event payload

To explain how it solves our problem:

1. wechaty-puppet-donut want to update the contact id XXX
2. wechaty-puppet-donut emit a dirty event, and the wechaty-puppet-donut cache has been updated
3. hostie server received the dirty event, and emit this event over GRPC
4. wechaty-puppet-hostie received a dirty event, then it updates its cache as well.

Then they are all good.

The above is just a scratch from my head today, please feel free to share your thoughts, thank you very much.

[windmemory]

1. we add a dirty event for the puppet
2. if a puppet receives the dirty event, it should:
   i. call onDirtyXXX() to clean the cache according to the event payload

I think this design would be able to resolve the problem we have here.

[su-chang]

So all update info will pop from wechaty-puppet-donut.

For contact.sync()
this method from initiative change to passive.

Case
Modify user contact in WeChat App
contact.sync() could not refresh the donut cache.

Due to donut can not know when the contact has been modified. We could emit a dirty event when the operation by API only.

[su-chang]

@huan Do you have some idea about re-design this feature? What can I do for it?

[huan]

Yes, I think we should implement the #43 (comment) .

I have run into the bug caused by this issue lots of times, so I believe we should go-ahead to fix it recently.

Could you please draft a design for my proposal above, so that we can make sure we are talking about the same solution?

After I confirmed that, I'd like to upgrade the abstract puppet to add those features.

[windmemory]

To clear the cache in hostie server side, here is a workaround:

// wechaty-puppet-xx
public contactRawPayload (id: string) {
  // Any code that you do your stuff
}

public contactRawPayloadParser (id: string) {
  // Any other code that you do the parse
}

// Code that act as a workaround
// This will override the implementation in wechaty-puppet
public contactPayload (id: string) {
  await this.contactPayloadDirty(id)
  return super.contactPayload(id: string)
}

If you add the contactPayload function in your wechaty-puppet-xx, then you will lose the cache mechanism in wechaty-puppet layer, but this will partially solve the issue that we are facing here.

The reason that this alternative will partially solve the problem is that the wechaty-puppet-hostie client could only call the contactPayload method to server, even if we do a sync() on the contact. So if the wechaty-puppet-hostie server has the cache in wechaty-puppet layer, then the data in wechaty-puppet-xx will never be used. With the code above, user can clear the cache and load data directly from wechaty-puppet-xx by calling sync(). So I think this could partially solve the problem.

Here is the PR that I created to add the method definition so the client could be able to clear the cache in server side in a more elegant way.

[huan]

Let's link all related Issues & PRs to this issue so that we can find everything from here.

[windmemory]

Feels like we can close this issue since we've implemented the dirty logic in wechaty-puppet-hostie.

[huan]

Yes, I think do.

@su-chang please help us confirm this issue had been resolved and close it if so.

[su-chang]

It works well as expect, let's close it now.

Thank you for your great design, @huan @windmemory

[huan]

You are welcome!