openid / sharedsignals

OpenID Shared Signals Working Group Repository
45 stars 11 forks source link

Do not allow create, update of stream without `events_requested` #100

Closed appsdesh closed 9 months ago

appsdesh commented 11 months ago

Stream does not have any meaning if the events_requested is created, updated with an empty array. Fixing the text to advise against such update.

Resolves #99

appsdesh commented 11 months ago

Why can't a Receiver set up a stream without events? What if you wanted to create a stream that was dynamically populated with event types at a later point?

After the stream is successfully created with relevant events, it makes sense that the requirements change and receiver updates existing events_requested with a new array.

What's the use case of stream creation for the empty events_requested, what would be the status of such a stream.

Maybe we should only prevent updating the stream with empty events_requested, if stream creation with empty array is a special case? The receiver can dynamically update the existing stream with the new array of events, there is no need to allow setting it to empty. If the receiver wants to decommission the stream it could do that by the status APIs, rather than making events_requested as empty.

Thoughts @FragLegs ?

independentid commented 11 months ago

For an update I would assume a missing value means don’t change.A provided value means may be changed. PhilOn Aug 9, 2023, at 10:23 AM, Apoorva Deshpande @.***> wrote:

Why can't a Receiver set up a stream without events? What if you wanted to create a stream that was dynamically populated with event types at a later point?

After the stream is successfully created with relevant events, it makes sense that the requirements change and receiver updates existing events_requested with a new array. What's the use case of stream creation for the empty events_requested, what would be the status of such a stream. Maybe we should only prevent updating the stream with empty events_requested, if stream creation with empty array is a special case? The receiver can dynamically update the existing stream with the new array of events, there is no need to allow setting it to empty. If the receiver wants to decommission the stream it could do that by the status APIs, rather than making events_requested as empty. Thoughts @FragLegs ?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

appsdesh commented 11 months ago

@independentid in the current shape and form, a replace config call PUT with empty values will reset the config. ( it will not ignore empty values )

FragLegs commented 10 months ago

I don't understand what bad thing happens if the Receiver decides to set events_requested to an empty array. True, they won't get any events. But is it our job to police that? They also won't get any events if they never add a subject to the stream. Should we raise alarms in that case too?

I think that the existing behavior is fine as is.

appsdesh commented 10 months ago

I don't understand what bad thing happens if the Receiver decides to set events_requested to an empty array. True, they won't get any events. But is it our job to police that? They also won't get any events if they never add a subject to the stream. Should we raise alarms in that case too?

I think that the existing behavior is fine as is.

+1 to keep the spec flexible but we should try to answer following things -

  1. Operationally, a transmitter may decide to cap max number of streams available, a non functional stream like this (one without events subscription) would use up that quota.
  2. If a stream is without any event subscriptions then would it be considered enabled or paused from the status perspective?
  3. If we already have a mechanism to pause the stream using update stream then why would we need to support a pseudo pause state by allowing empty subscription?
appsdesh commented 9 months ago

Following up on https://github.com/openid/sharedsignals/pull/117