You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2023/01/10 10:56:22 UTC
[GitHub] [james-project] quantranhong1999 opened a new pull request, #1386: JAMES-3756 JMAP pushes should support delegation
quantranhong1999 opened a new pull request, #1386:
URL: https://github.com/apache/james-project/pull/1386
TODO:
- [x] web push
- [x] websocket
- [ ] sse
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1067652828
##########
event-bus/api/src/main/java/org/apache/james/events/EventBus.java:
##########
@@ -54,6 +55,13 @@ default Publisher<Registration> register(EventListener listener, RegistrationKey
Publisher<Registration> register(EventListener.ReactiveEventListener listener, RegistrationKey key);
+ default Publisher<Registration> register(EventListener.ReactiveEventListener listener, Collection<RegistrationKey> keys) {
+ return Flux.fromIterable(keys)
+ .concatMap(key -> register(listener, key))
+ .reduce((reg1, reg2) -> () -> Mono.from(reg1.unregister())
+ .then(Mono.from(reg2.unregister())));
+ }
Review Comment:
IMO we could / should use Flux.merge.
There's no need to delay `reg2.unregister()` subscription.
```
return Flux.fromIterable(keys)
.concatMap(key -> register(listener, key))
.collectList()
.flatMap(regs -> Flux.merge(regs).then());
```
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1065631209
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/WebSocketRoutes.scala:
##########
@@ -134,6 +136,12 @@ class WebSocketRoutes @Inject() (@Named(InjectionKeys.RFC_8621) val authenticato
StateChangeListener(pushEnable.dataTypes.getOrElse(typeStateFactory.all.toSet), clientContext.outbound),
AccountIdRegistrationKey.of(clientContext.session.getUser)))
.doOnNext(newRegistration => clientContext.withRegistration(newRegistration))
+ .`then`(SFlux.fromPublisher(delegationStore.delegatedUsers(clientContext.session.getUser))
+ .flatMap(delegatedUser => eventBus.register(
+ StateChangeListener(pushEnable.dataTypes.getOrElse(typeStateFactory.all.toSet), clientContext.outbound),
+ AccountIdRegistrationKey.of(delegatedUser)))
+ .doOnNext(newRegistration => clientContext.withRegistration(newRegistration))
Review Comment:
I am unsure about unregistering the old registration here, could need a bit more attention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
vttranlina commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1070752381
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/EventSourceRoutes.scala:
##########
@@ -188,10 +190,11 @@ class EventSourceRoutes@Inject() (@Named(InjectionKeys.RFC_8621) val authenticat
.asFlux()
.subscribe(ping => context.outbound.emitNext(ping, FAIL_FAST))
- SMono(
- eventBus.register(
- StateChangeListener(options.types, context.outbound),
- AccountIdRegistrationKey.of(session.getUser)))
+ SMono.just(session.getUser)
+ .concatWith(SFlux.fromPublisher(delegationStore.delegatedUsers(session.getUser)))
Review Comment:
`SFlux(delegationStore.delegatedUsers(session.getUser))`
is better?
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/pushsubscription/PushListener.scala:
##########
@@ -58,15 +59,18 @@ object PushListener {
}
class PushListener @Inject()(pushRepository: PushSubscriptionRepository,
- webPushClient: WebPushClient,
- pushSerializer: PushSerializer) extends ReactiveGroupEventListener {
+ webPushClient: WebPushClient,
+ pushSerializer: PushSerializer,
+ delegationStore: DelegationStore) extends ReactiveGroupEventListener {
override def getDefaultGroup: Group = PushListenerGroup()
override def reactiveEvent(event: Event): Publisher[Void] =
event match {
case event: StateChangeEvent =>
- SFlux(pushRepository.list(event.username))
+ SMono.just(event.username)
+ .concatWith(delegationStore.authorizedUsers(event.username))
Review Comment:
why we don't use
`SFlux(delegationStore.authorizedUsers(event.username)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
chibenwa commented on PR #1386:
URL: https://github.com/apache/james-project/pull/1386#issuecomment-1378195100
Don't forget to rebase
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa merged pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
chibenwa merged PR #1386:
URL: https://github.com/apache/james-project/pull/1386
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1070761070
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/pushsubscription/PushListener.scala:
##########
@@ -58,15 +59,18 @@ object PushListener {
}
class PushListener @Inject()(pushRepository: PushSubscriptionRepository,
- webPushClient: WebPushClient,
- pushSerializer: PushSerializer) extends ReactiveGroupEventListener {
+ webPushClient: WebPushClient,
+ pushSerializer: PushSerializer,
+ delegationStore: DelegationStore) extends ReactiveGroupEventListener {
override def getDefaultGroup: Group = PushListenerGroup()
override def reactiveEvent(event: Event): Publisher[Void] =
event match {
case event: StateChangeEvent =>
- SFlux(pushRepository.list(event.username))
+ SMono.just(event.username)
+ .concatWith(delegationStore.authorizedUsers(event.username))
Review Comment:
your solution does include the owner of the event himself
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1065957779
##########
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/WebSocketContract.scala:
##########
@@ -486,6 +488,51 @@ trait WebSocketContract {
.contains(stateChange)
}
+ @Test
+ @Timeout(180)
+ def shouldPushChangesToDelegatedUser(server: GuiceJamesServer): Unit = {
Review Comment:
Please add a test ensuring that if alice delegates her mailbox to bob then alice can still receive her notifications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
chibenwa commented on PR #1386:
URL: https://github.com/apache/james-project/pull/1386#issuecomment-1378147640
Alternative design idea:
Allow the event bus to register several key for a listener:
```
default Publisher<Registration> register(EventListener.ReactiveEventListener listener, Collection<RegistrationKey> keys) {
return Flux.fromIterable(keys)
.concatMap(key -> register(listener, key))
.reduce((r1, r2) -> () -> Mono.from(r1.unregister())
.then(Mono.from(r2.unregister())));
}
```
Use this to keep several registrations at once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1068914331
##########
event-bus/api/src/main/java/org/apache/james/events/EventBus.java:
##########
@@ -54,6 +55,13 @@ default Publisher<Registration> register(EventListener listener, RegistrationKey
Publisher<Registration> register(EventListener.ReactiveEventListener listener, RegistrationKey key);
+ default Publisher<Registration> register(EventListener.ReactiveEventListener listener, Collection<RegistrationKey> keys) {
+ return Flux.fromIterable(keys)
+ .concatMap(key -> register(listener, key))
+ .reduce((reg1, reg2) -> () -> Mono.from(reg1.unregister())
+ .then(Mono.from(reg2.unregister())));
+ }
Review Comment:
Your proposal does not work exactly but I take your idea and mutate it a bit. Please check my latest fixup commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
vttranlina commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1070752381
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/EventSourceRoutes.scala:
##########
@@ -188,10 +190,11 @@ class EventSourceRoutes@Inject() (@Named(InjectionKeys.RFC_8621) val authenticat
.asFlux()
.subscribe(ping => context.outbound.emitNext(ping, FAIL_FAST))
- SMono(
- eventBus.register(
- StateChangeListener(options.types, context.outbound),
- AccountIdRegistrationKey.of(session.getUser)))
+ SMono.just(session.getUser)
+ .concatWith(SFlux.fromPublisher(delegationStore.delegatedUsers(session.getUser)))
Review Comment:
`SFlux(delegationStore.delegatedUsers(session.getUser))`
is better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1065956696
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/routes/WebSocketRoutes.scala:
##########
@@ -134,6 +136,12 @@ class WebSocketRoutes @Inject() (@Named(InjectionKeys.RFC_8621) val authenticato
StateChangeListener(pushEnable.dataTypes.getOrElse(typeStateFactory.all.toSet), clientContext.outbound),
AccountIdRegistrationKey.of(clientContext.session.getUser)))
.doOnNext(newRegistration => clientContext.withRegistration(newRegistration))
+ .`then`(SFlux.fromPublisher(delegationStore.delegatedUsers(clientContext.session.getUser))
+ .flatMap(delegatedUser => eventBus.register(
+ StateChangeListener(pushEnable.dataTypes.getOrElse(typeStateFactory.all.toSet), clientContext.outbound),
+ AccountIdRegistrationKey.of(delegatedUser)))
+ .doOnNext(newRegistration => clientContext.withRegistration(newRegistration))
Review Comment:
It get's tricky because If bob delegates account for alice then alice needs to have 2 registration: alice one and bob one.
We need client context to hold a list of registration, somehow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
chibenwa commented on PR #1386:
URL: https://github.com/apache/james-project/pull/1386#issuecomment-1377475151
SSE ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] quantranhong1999 commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1070761070
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/pushsubscription/PushListener.scala:
##########
@@ -58,15 +59,18 @@ object PushListener {
}
class PushListener @Inject()(pushRepository: PushSubscriptionRepository,
- webPushClient: WebPushClient,
- pushSerializer: PushSerializer) extends ReactiveGroupEventListener {
+ webPushClient: WebPushClient,
+ pushSerializer: PushSerializer,
+ delegationStore: DelegationStore) extends ReactiveGroupEventListener {
override def getDefaultGroup: Group = PushListenerGroup()
override def reactiveEvent(event: Event): Publisher[Void] =
event match {
case event: StateChangeEvent =>
- SFlux(pushRepository.list(event.username))
+ SMono.just(event.username)
+ .concatWith(delegationStore.authorizedUsers(event.username))
Review Comment:
your solution does not include the owner of the event himself
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] vttranlina commented on a diff in pull request #1386: JAMES-3756 JMAP pushes should support delegation
Posted by GitBox <gi...@apache.org>.
vttranlina commented on code in PR #1386:
URL: https://github.com/apache/james-project/pull/1386#discussion_r1070763285
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/pushsubscription/PushListener.scala:
##########
@@ -58,15 +59,18 @@ object PushListener {
}
class PushListener @Inject()(pushRepository: PushSubscriptionRepository,
- webPushClient: WebPushClient,
- pushSerializer: PushSerializer) extends ReactiveGroupEventListener {
+ webPushClient: WebPushClient,
+ pushSerializer: PushSerializer,
+ delegationStore: DelegationStore) extends ReactiveGroupEventListener {
override def getDefaultGroup: Group = PushListenerGroup()
override def reactiveEvent(event: Event): Publisher[Void] =
event match {
case event: StateChangeEvent =>
- SFlux(pushRepository.list(event.username))
+ SMono.just(event.username)
+ .concatWith(delegationStore.authorizedUsers(event.username))
Review Comment:
ah, i got it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org