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

Don't ignore set_tweak actions with no explicit value. #7766

Merged
merged 4 commits into from
Jul 6, 2020
Merged

Don't ignore set_tweak actions with no explicit value. #7766

merged 4 commits into from
Jul 6, 2020

Conversation

reivilibre
Copy link
Contributor

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

@reivilibre
Fix spec compliance; tweaks without values are valid
2209621 
(default to True, which is only concretely specified for
`highlight`, but it seems only reasonable to generalise)
@reivilibre
Changelog for 7766.
3920a92
@reivilibre reivilibre requested a review from a team July 1, 2020 10:06
@richvdh
Copy link
Member

richvdh commented Jul 1, 2020

Are you sure this is the correct interpretation of the spec?

See also: https://github.com/matrix-org/matrix-doc/issues/2643

@reivilibre
Copy link
Contributor Author

reivilibre commented Jul 1, 2020 •
edited
Loading

The spec is a bit yucky in this regard, but it says that:

  • set_tweak without a value is a valid action.
  • for highlight tweaks, True is to be assumed if value is absent. (see 1.)

However, by omission:

  • other tweaks (sound, plus any other tweaks that clients and push gateways may agree on, as is their permission to do according to spec) don't have a spec-defined value in the case of an absent value.

Ideally (imo) the spec should have had the guts to fully define this or not made value optional at all... Seems like a silly place to introduce needless ambiguity. But it is what it is, and this solution solves the case of highlight and at least sets something reasonable for other arbitrary tweaks.

  1. highlight
    A boolean representing whether or not this message should be highlighted in the UI. This will normally take the form of presenting the message in a different colour and/or style. The UI might also be adjusted to draw particular attention to the room in which the event occurred. If a highlight tweak is given with no value, its value is defined to be true. If no highlight tweak is given at all then the value of highlight is defined to be false.

@richvdh
Copy link
Member

richvdh commented Jul 1, 2020

sorry I'm not entirely following you. Agreed that the spec says highlight with no value is the same as highlight with value=true. Are you telling me that synapse does not correctly implement that? (Not that it makes much difference in practice, since the neither of the pushkins care about highlight)?

I'm obviously missing something else: my understanding was that synapse passes the tweaks dictionary straight through to the push gateway. What is this tweaks_for_actions trying to do?

@reivilibre
Copy link
Contributor Author

sorry I'm not entirely following you. Agreed that the spec says highlight with no value is the same as highlight with value=true. Are you telling me that synapse does not correctly implement that? (Not that it makes much difference in practice, since the neither of the pushkins care about highlight)?

Yes, a tweak with no value is currently discarded.

I'm obviously missing something else: my understanding was that synapse passes the tweaks dictionary straight through to the push gateway. What is this tweaks_for_actions trying to do?

Converts a list of actions e.g. [{"set_tweak": "a", "value": "AAA"}, {"set_tweak": "b", "value": "BBB"}, "notify"]
into a tweaks dict e.g. {"a": "AAA", "b": "BBB"} which is then passed to Sygnal (and used for #7765).

@richvdh
Copy link
Member

richvdh commented Jul 1, 2020

right, what a mess. thanks for explaining.

richvdh
richvdh reviewed Jul 1, 2020
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

looks fine, but a test would be nice.

@reivilibre reivilibre requested review from richvdh and a team and removed request for richvdh July 1, 2020 14:27
richvdh
richvdh approved these changes Jul 3, 2020
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

(of course: for even more bonus points we'd have a sytest, so that we didn't have to re-learn this decision for every future HS impl...)

@reivilibre reivilibre merged commit 57feeab into develop Jul 6, 2020
@reivilibre reivilibre deleted the rei/pushrule_valueless branch July 6, 2020 10:43
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@anoadragon453
Merge commit '43726783e' into dinsic
530c8a5 
* commit '43726783e': (22 commits)
  1.17.0rc1
  Fix some spelling mistakes / typos. (#7811)
  `update_membership` declaration: now always returns an event id. (#7809)
  Improve stacktraces from exceptions in background processes (#7808)
  Fix `can only concatenate list (not "tuple") to list` exception (#7810)
  Pass original request headers from workers to the main process. (#7797)
  Generate real events when we reject invites (#7804)
  Add `HomeServer.signing_key` property (#7805)
  Revert "Update the installation docs on apt-transport-https (#7801)"
  Do not use simplejson in Synapse. (#7800)
  Stop passing bytes when dumping JSON (#7799)
  Update the installation docs on apt-transport-https (#7801)
  shuffle changelog slightly
  Change Caddy links (old is deprecated) (#7789)
  Stop populating unused table `local_invites`. (#7793)
  Refactor getting replication updates from database v2. (#7740)
  Add libwebp dependency to Dockerfile (#7791)
  Add documentation for JWT login type and improve sample config. (#7776)
  Convert the appservice handler to async/await. (#7775)
  Don't ignore `set_tweak` actions with no explicit `value`. (#7766)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richvdh richvdh richvdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@reivilibre @richvdh