Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

type property missing for m.reference relations #7941

Closed
joepie91 opened this issue Jul 23, 2020 · 6 comments
Closed

type property missing for m.reference relations #7941

joepie91 opened this issue Jul 23, 2020 · 6 comments
Labels
good first issue Good for newcomers S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution z-p3 (Deprecated Label)

Comments

@joepie91
Copy link

I received the following event from Synapse for a bridged Telegram message:

  {
    type: 'm.room.message',
    room_id: '!redacted:pixie.town',
    sender: '@telegram_redacted:pixie.town',
    content: {
      msgtype: 'm.text',
      body: 'redacted',
      external_url: 'https://t.me/c/redacted'
    },
    origin_server_ts: 1595536311000,
    unsigned: {
      age: 710692,
      'm.relations': {
        'm.reference': {
          chunk: [
            {
              event_id: '$redacted1'
            }
          ]
        }
      }
    },
    event_id: '$redacted2',
    user_id: '@telegram_redacted:pixie.town',
    age: 710692
  },

However, MSC 1849 says:

m.reference list the event_id and event type of the events which reference that event.

... which implies that the server is required to provide the type property for the m.reference relation, but it is currently missing.

(I realize that the MSC hasn't been finalized and merged yet, but I'm filing an issue anyway to make sure it doesn't get overlooked later.)

@clokep
Copy link
Member

clokep commented Jul 23, 2020

Is this a bug with the Telegram bridge?

@joepie91
Copy link
Author

As I understand it, the m.relations field is generated by the homeserver based on the related events that it is aware of, so that should be totally separate from any buggy clients/appservices.

(Though even if it were a buggy appservice, that would just make this an input validation issue!)

Anyway, seeing something similar for origin_server_ts in m.annotation, which is also missing:

  {
    content: {
      body: 'redacted',
      msgtype: 'm.text'
    },
    origin_server_ts: 1595367512866,
    room_id: '!redacted:matrix.org',
    sender: '@redacted:example.com',
    type: 'm.room.message',
    unsigned: {
      age: 171403042,
      'm.relations': {
        'm.annotation': { chunk: [ { type: 'm.reaction', key: '💥', count: 1 } ] }
      }
    },
    event_id: '$redacted',
    user_id: '@redacted:example.com',
    age: 171403042
  },

The MSC is slightly less explicit about the requiredness of that one, though.

@anoadragon453 anoadragon453 added z-bug (Deprecated Label) good first issue Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution z-p3 (Deprecated Label) labels Jul 23, 2020
@jerinjtitus
Copy link
Contributor

Can anyone help me, I was trying to find where percisely m.relations field is generated. I am not completely sure which one it is.

@clokep
Copy link
Member

clokep commented Jan 11, 2021

Looks like the code around this is at

annotations = await self.store.get_aggregation_groups_for_event(event_id)
references = await self.store.get_relations_for_event(
event_id, RelationTypes.REFERENCE, direction="f"
)
if annotations.chunk:
r = serialized_event["unsigned"].setdefault("m.relations", {})
r[RelationTypes.ANNOTATION] = annotations.to_dict()
if references.chunk:
r = serialized_event["unsigned"].setdefault("m.relations", {})
r[RelationTypes.REFERENCE] = references.to_dict()

@richvdh richvdh added the good first issue Good for newcomers label Apr 6, 2021
@erikjohnston erikjohnston added S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. and removed z-bug (Deprecated Label) labels Jul 26, 2021
@lukasdenk
Copy link
Contributor

lukasdenk commented Jan 3, 2022

(I realize that the MSC hasn't been finalized and merged yet, but I'm filing an issue anyway to make sure it doesn't get overlooked later.)

As statet by @joepie91, the MSC specifying that an m.reference must contain a type is still a proposal. Furthermore, it seems like MSC1849 is split in several proposals (see e.g. here). The specification of event references seems to be handled in MSC3267. There, the specification is different from this PR's description.

@clokep Since MSC3267 is still ongoing work (more specifically, it has no proposal lifetime state label), I would suggest to make clear that this issue should only be resolved after MSC3267 is accepted.

@clokep
Copy link
Member

clokep commented Jan 4, 2022

Since MSC3267 is still ongoing work (more specifically, it has no proposal lifetime state label), I would suggest to make clear that this issue should only be resolved after MSC3267 is accepted.

Yeah, the state of those MSCs is quite confusing; MSC3267 specifically says it doesn't declare a bundled aggregation format, but MSC2675 also doesn't declare one. (Note that MSC1849 is obsolete as you mention.)

I'll make a note on MSC3267 about this inconsistency, but for now I'm going to close this issue since it isn't clear it is a Synapse issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution z-p3 (Deprecated Label)
Projects
None yet
Development

No branches or pull requests

7 participants