Update the geofencing subscription models to align on CAMARA commonalities

maxl2287 commented 3 months ago

What type of PR is this?

correction
enhancement/feature

What this PR does / why we need it:

Update of the subscription-models based on:

Which issue(s) this PR fixes:

Fixed Issues

Fixes #124
Fixes #118
Fixes #200

maxl2287 commented 3 months ago

@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 commented 3 months ago

@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?

jlurien commented 3 months ago

@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 commented 3 months ago

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

alpaycetin74 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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: https://github.com/camaraproject/Commonalities/issues/235

jlurien commented 3 months ago

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 commented 3 months ago

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

maxl2287 commented 3 months ago

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

bigludo7 commented 3 months ago

@maxl2287 We can merge this one, correct?

maxl2287 commented 3 months ago

@bigludo7 I am finished. So yes. :)

camaraproject / DeviceLocation