camaraproject / DeviceLocation

Repository to describe, develop, document and test the DeviceLocation API family
Apache License 2.0
21 stars 33 forks source link

Define new geofencing subscriptions API with event subscription endpoint #98

Closed maxl2287 closed 10 months ago

maxl2287 commented 10 months ago

What type of PR is this?

Add one of the following kinds:

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.

maxl2287 commented 10 months ago

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

maxl2287 commented 10 months ago

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

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

bigludo7 commented 10 months ago

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

maxl2287 commented 10 months ago

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

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

@bigludo7 ready now to be rereviewed 🚀