Update the geofencing subscription models to align on CAMARA commonalities

[maxl2287]

What type of PR is this?

• correction
• enhancement/feature

What this PR does / why we need it:

Update of the subscription-models based on:

• https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#12-subscription-notification--event
• https://github.com/camaraproject/Commonalities/blob/main/artifacts/camara-cloudevents/event-subscription-template.yaml

Which issue(s) this PR fixes:

Fixed Issues

• Fixes Geofencing API - Defining a standard behavior for first event #124
• Fixes Geofencing API - Add "format: uri" to notificationUrl  #118
• Fixes Geofencing - Align with new model #200

[maxl2287]

@bigludo7 @jlurien

What are we doing about this here?
Are we setting here a maximum of 1 for now?
Because we are not yet supporting multiple types, as this would be maybe an own PR / issue.

Wdyt?

        types:
          description: |
            Camara Event types eligible to be delivered by this subscription.
            Note: for the Commonalities meta-release v0.4 we enforce to have only event type per subscription then for following meta-release use of array MUST be decided
             at API project level.
          type: array
          items:
            $ref: "#/components/schemas/SubscriptionEventType"

[bigludo7]

@bigludo7 @jlurien

What are we doing about this here? Are we setting here a maximum of 1 for now? Because we are not yet supporting multiple types, as this would be maybe an own PR / issue.

Wdyt?

        types:
          description: |
            Camara Event types eligible to be delivered by this subscription.
            Note: for the Commonalities meta-release v0.4 we enforce to have only event type per subscription then for following meta-release use of array MUST be decided
             at API project level.
          type: array
          items:
            $ref: "#/components/schemas/SubscriptionEventType"

I think we can add this maximum:1 in our yaml. The template in commonalities is generic and should live for more than one release. Here we're crafting an yaml for implementation so better to be straightforward.
@jlurien @alpaycetin74 different view?

[bigludo7]

LGFM !
Thanks Max

[jlurien]

@bigludo7 @jlurien
What are we doing about this here? Are we setting here a maximum of 1 for now? Because we are not yet supporting multiple types, as this would be maybe an own PR / issue.
Wdyt?

        types:
          description: |
            Camara Event types eligible to be delivered by this subscription.
            Note: for the Commonalities meta-release v0.4 we enforce to have only event type per subscription then for following meta-release use of array MUST be decided
             at API project level.
          type: array
          items:
            $ref: "#/components/schemas/SubscriptionEventType"

I think we can add this maximum:1 in our yaml. The template in commonalities is generic and should live for more than one release. Here we're crafting an yaml for implementation so better to be straightforward. @jlurien @alpaycetin74 different view?

I agree to setting the maximum to 1 in this version as that will impose implementations to follow the agreed current limit.

[jlurien]

I think we should include in our spec for this version the values that are allowed: protocol=HTTP, credentialType=ACCESSTOKEN and remove the content for all other options which are not applicable.

[jlurien]

FYI, @PedroDiez, as you have been more involved than me in this track in Commonalities

[alpaycetin74]

@bigludo7 @jlurien
What are we doing about this here? Are we setting here a maximum of 1 for now? Because we are not yet supporting multiple types, as this would be maybe an own PR / issue.
Wdyt?

        types:
          description: |
            Camara Event types eligible to be delivered by this subscription.
            Note: for the Commonalities meta-release v0.4 we enforce to have only event type per subscription then for following meta-release use of array MUST be decided
             at API project level.
          type: array
          items:
            $ref: "#/components/schemas/SubscriptionEventType"

I think we can add this maximum:1 in our yaml. The template in commonalities is generic and should live for more than one release. Here we're crafting an yaml for implementation so better to be straightforward. @jlurien @alpaycetin74 different view?

Yes, I find maximum=1 reasonable. Thank you.

[jlurien]

@bigludo7, as you are also adopting the subscription model to other APIs, what is the agreement in Commonalities? Are we supposed to use the common artifact in Commonalities, as it is, even with the options which are not applicable to the current version, or APIs can make adjustments? I suggested to remove protocols and credentialTypes which are not supported but Pedro told me that the agreement was to include all options from the beginning and specify errors for options not applicable.

[PedroDiez]

Hi all,

This is my understading about the agreement in commonalities: @bigludo7, @shilpa-padgaonkar, @rartych

• Artifact, is a common baseline to be reused as-is in any API (in APIs with Instance-based (implicit) subscription, only applies sink and sinkCredential Model). That was the reason to model all the variants of Cloudevents and to add specific exceptions to address current simplifications. Benefit is that we will not need to update each time the model and avoid breaking-changes in the future.
• If the template is not followed, then it is not a template and probably this will generate confusion and misalignments between Subproyects

cc @maxl2287

[bigludo7]

Hi all,

I'm globally aligned with https://github.com/PedroDiez statement... but I should confess one contradiction (with myself ...ouch) because I was also fine to add this max set to 1 for the attribute types to be perfectly explicit to have only one event type per subscription for this release.

For the protocol & tokenType I prefer to keep this in this yaml as defined in the template.

[jlurien]

Hi all,

I'm globally aligned with https://github.com/PedroDiez statement... but I should confess one contradiction (with myself ...ouch) because I was also fine to add this max set to 1 for the attribute types to be perfectly explicit to have only one event type per subscription for this release.

For the protocol & tokenType I prefer to keep this in this yaml as defined in the template.

Indeed I read first the issue about adding the maximum even if it was not in the artifact, and I thought that the same criteria would apply to the rest of non-applicable stuff for this version :-) I think we should have a consistent approach for this, as other APIs may choose to not add the maximum, and that would create inconsistency.

IMO, commonalities versions should reflect what is applicable to each version, and they will evolve as we support more options, but if the agreement is to model the operations as in the template, we should stick to whatever it is in the artifact for this version.

[maxl2287]

Indeed I read first the issue about adding the maximum even if it was not in the artifact, and I thought that the same criteria would apply to the rest of non-applicable stuff for this version :-) I think we should have a consistent approach for this, as other APIs may choose to not add the maximum, and that would create inconsistency.

IMO, commonalities versions should reflect what is applicable to each version, and they will evolve as we support more options, but if the agreement is to model the operations as in the template, we should stick to whatever it is in the artifact for this version.

@jlurien - about maximum of types,
As it is written in the Commonalities-template:

        types:
          description: |
            Camara Event types eligible to be delivered by this subscription.
            Note: for the Commonalities meta-release v0.4 we enforce to have only event type per subscription then for following meta-release use of array MUST be decided
             at API project level.
          type: array
          items:
            type: string

(https://github.com/camaraproject/Commonalities/blob/main/artifacts/camara-cloudevents/event-subscription-template.yaml#L305C1-L312C25)

Or in API design guidelines:

Note: An array of types could be passed but as of now only one value MUST passed. Use of multiple value will be open later at API level.

Based on this notes it seems to be limited to a maximum of one item in the array (as of now).

(see https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#resource-based-explicit-subscription)

So in my PoV we could set here a limit of 1.

And when we want to support multiple Types, we can still create an own issue for it.

Wdyt @jlurien @bigludo7
cc @PedroDiez @alpaycetin74

[maxl2287]

@PedroDiez @jlurien @bigludo7
What about setting a minimum of Items to 1?
minItems: 1

Because otherwise it would also be possible to provide an empty array (what is I guess not the expectation, when creating subscriptions)

I have created an issue for that: camaraproject/Commonalities#235

[jlurien]

If that was the decision, I'm OK with keeping all unused values for protocol and credentialType. There are specific examples for errors INVALID_PROTOCOL and INVALID_CREDENTIAL for those. There is no specific code for the case when more that one event, or zero items, are sent, so it makes even more sense to add the limits minItems, maxItems in the schema, and rely on the generic 400 INVALID_ARGUMENT when a request body is not compliant.

[bigludo7]

Agreed also with @maxl2287 proposal for the min. I will take care of this one in Commonalities !

[maxl2287]

@jlurien & @bigludo7 ready to be reviewed again :)

[bigludo7]

LGTM/
For simplicity probably good to manage device identifier update defined in PR213 in another issue/PR.

[bigludo7]

LGTM/

[jlurien]

LGTM

[bigludo7]

@maxl2287 We can merge this one, correct?

[maxl2287]

@bigludo7 I am finished. So yes. :)