Skip to content

Define new geofencing subscriptions API with event subscription endpoint#98

Merged
jlurien merged 29 commits intocamaraproject:mainfrom
maxl2287:define-new-location-subscriptions-api-with-event-subscription-endpoint
Oct 25, 2023
Merged

Define new geofencing subscriptions API with event subscription endpoint#98
jlurien merged 29 commits intocamaraproject:mainfrom
maxl2287:define-new-location-subscriptions-api-with-event-subscription-endpoint

Conversation

@maxl2287
Copy link
Contributor

@maxl2287 maxl2287 commented Oct 9, 2023

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

Add CloudEvent subscription endpoints.

Which issue(s) this PR fixes:

Fixes #18

Note to reviewers

This PR is a clone to #74
In the fact that I am not allowed to add commits to Chia's repository and he is not part of the CAMARA project anymore.

chia2017 and others added 17 commits October 9, 2023 23:39
resolved review comments
removed extra boolean variables
Changed yaml file based on last review comments
Resolved review comments
Please check this last commit. All the review comments applied.
Deleted EventBase object, now all other events inherit from Event object and there is no "anyOf" in the Event anymore
Create EventTypeNotification which inherit from EventType, now in create subscription we do not have SUBSCRIPTION_ENDS and only in notification we use that.
Created NotifiedEventType enum with 3 values
change the api name to geofencing
changed the title and description of API based on new name
@maxl2287 maxl2287 changed the title Define new location subscriptions api with event subscription endpoint Define new geofencing subscriptions API with event subscription endpoint Oct 9, 2023
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @maxl2287 for the subscription.
Please see some comments for your consideration in my review.

The most significant is about the event type granularity. I was expecting to have distinct event for entered and exit. Let's discuss this.

@maxl2287
Copy link
Contributor Author

Thanks for your review @bigludo7!
I will update the PR asap. 🚀

@maxl2287
Copy link
Contributor Author

maxl2287 commented Oct 12, 2023

@jlurien I am using, e.g., a swagger editor to validate my written schemas.
For the discriminator used in the "Area" it seams to not work.
My expected behaviour is here that there should be an example for "area" in the subscription-POST-request.
But "area" ist here an empty object:

"area" : {}
image

Could you please check if this is at least it is specified good or not here:

    Area:
      type: object
      properties:
        areaType:
          $ref: '#/components/schemas/AreaType'
      required:
        - areaType
      discriminator:
        propertyName: 'areaType'
        mapping:
          CIRCLE:  "#/components/schemas/Circle"

    AreaType:
      type: string
      description: |
        Type of this area.
        CIRCLE - The area is defined as a circle.
      enum:
        - CIRCLE

    Circle:
      description: Circular area
      allOf:
        - $ref: '#/components/schemas/Area'
        - type: object
          properties:
            center:
              $ref: '#/components/schemas/Point'
            radius:
              type: integer
              description: Expected accuracy for the subscription event of device location in m, from location (radius)
              minimum: 2000
              maximum: 200000
          required:
            - center
            - radius
      example:
        areaType: CIRCLE
        center:
          latitude: 50.735851
          longitude: 7.10066
        radius: 50000

Thanks in advance

@maxl2287 maxl2287 requested a review from jlurien October 13, 2023 08:28
@maxl2287 maxl2287 requested a review from bigludo7 October 18, 2023 08:58
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution;
Some very small corrections to fix the tag.

One question: Do you plan to provide the documentation in this PR or prefer to merge this one only with the yaml and have another for the documentation?

@maxl2287
Copy link
Contributor Author

maxl2287 commented Oct 19, 2023

Thanks for the contribution; Some very small corrections to fix the tag.

One question: Do you plan to provide the documentation in this PR or prefer to merge this one only with the yaml and have another for the documentation?

@bigludo7 Thanks for the review.

Yes I will add the documentation asap in this PR

@maxl2287 maxl2287 requested a review from bigludo7 October 19, 2023 08:42
@bigludo7
Copy link
Collaborator

@maxl2287 Thanks for the corrections.
I will make another review once the documentation added but no hurry :)

@maxl2287
Copy link
Contributor Author

@bigludo7 short question about "CreateSubscription".
I have here just the webhook required.
I saw it as well in your DeviceStatus PR.

But does it make sense?
I think SubscriptionDetail should be required too, shouldn't it?

@bigludo7
Copy link
Collaborator

bigludo7 commented Oct 19, 2023

@bigludo7 short question about "CreateSubscription". I have here just the webhook required. I saw it as well in your DeviceStatus PR.

But does it make sense? I think SubscriptionDetail should be required too, shouldn't it?

@maxl2287 That's a fair question.
From a generic pattern the subscriptionDetail is not mandatory as nothing prevent to have one specific path for one type and the device could be identified by the network but this is a very specific case.

In the context of geofencing API, as it the device status API, I think that making createSubscription mandatory makes a lot of sense !
Let's do this way in both API and check team feedback.

@maxl2287
Copy link
Contributor Author

@bigludo7 ready now to be rereviewed 🚀

Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some final comments. I think it's quite mature to be merged as -wip after some few adjustments.

@maxl2287 maxl2287 requested a review from jlurien October 24, 2023 09:33
Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave the default for basePath as you propose until it is fully aligned in Commonalities and Release Management, so we can progress with this PR.

@maxl2287 maxl2287 requested a review from jlurien October 24, 2023 20:36
Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo

@maxl2287 maxl2287 requested a review from jlurien October 25, 2023 09:46
Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !!!

@jlurien jlurien merged commit 51c8f26 into camaraproject:main Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Subscription for Location Area

4 participants