-
Notifications
You must be signed in to change notification settings - Fork 37
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
Make extension scoping work with only one field #149
Conversation
869d707
to
bb0d653
Compare
honestly I'm not thrilled with this
12c5824
to
d17737d
Compare
d17737d
to
da544d9
Compare
// InterpretAsInitial interprets this as an initial request rather than a delta, and | ||
// overwrites fields accordingly. This can be useful when fields have default | ||
// values, but is a little ugly. Use sparingly. | ||
InterpretAsInitial() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some of this ugliness comes from the fact that we use the same struct type to represent a blob of data D and an optional change to that blob of data.
@@ -202,6 +226,24 @@ func (r Request) ApplyDelta(next *Request) Request { | |||
return r | |||
} | |||
|
|||
func (r *Request) InterpretAsInitial() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this is supposed to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment above the declaration of this in the interface. https://github.com/matrix-org/sliding-sync/pull/149/files#r1232127907
To illustrate: suppose I receive a request whose account data extension is configured with
{
"enabled": "true"
}
If this is a brand-new request, the proxy should process this in two steps:
- Use the initial sticky parameters for
lists
androoms
(i.e.["*"]
). - Set
enabled
totrue
If this is a follow-up to a previous request, the proxy should interpret this as follows
- Use the previous sticky values for
lists
androoms
. - Set
enabled
totrue
.
The way I wanted to handle this was something like:
- On GenericRequest, define a Default() method that returns a new GenericRequest of the same type, filled with initial sticky values.
- Use this in Request.ApplyDelta to generate
curr
in the brand-new case whenisNil(curr)
. Then callcurr.ApplyDelta(next)
, just like we already do in the follow-up case.
The problem is that I couldn't figure out a way to do the first bullet. It felt like I was fighting the language. So instead, InterpretAsInitial
has some extra logic to mutate next
in place. The intention is that
Default().ApplyDelta(next)
would be equal to next
after running next.InterpretAsInitial()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTAOD I'm not very happy with InterpretAsInitial. If there is some nice way to implement Default() or similar I'm all ears.
@@ -288,6 +289,159 @@ func TestTypingRespectsExtensionScope(t *testing.T) { | |||
}) | |||
} | |||
|
|||
// Similar to TestTypingRespectsExtensionScope, but here we check what happens if | |||
// the extension is configured with only one of the `lists` and `rooms` fields. | |||
func TestTypingRespectsExtensionScopeWithOmittedFields(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we actually test the EX use case as well please as another test? I.e rooms: [*], lists: []
on the receipts extension -> only see receipts for subscribed rooms.
Co-authored-by: kegsay <[email protected]>
Fixes #148.