Add documentation

[bigludo7]

Add documentation for the API
Solve issue #57

What type of PR is this?

• documentation

What this PR does / why we need it:

This PR add the documentation in the API swagger.

Which issue(s) this PR fixes:

Fixes #57

Special notes for reviewers:

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

[fernandopradocabrillo]

I think we should include documentation regarding the use of the sync/async use of the API. Something along the lines of indicating that the response code will be implementation-dependent.
And I would complete the async behaviour a bit more by pointing out that a GET request to /event-subscriptions/{eventSubscriptionId} should be made after the creation/deletion of the subscription to check if the operation was successful.

[eric-murray]

@fernandopradocabrillo
Can you propose some additional text? I don't think we need to document optional behaviour on the API consumer's part.

[bigludo7]

@fernandopradocabrillo (additionnally to @eric-murray comment)
Fair point but one question for you before to update this doc: this point shouldn't be documented in the design guideline API ? As it should be common for all subscriptions it seems to me that is should be there. Thoughts?

[JoachimDahlgren]

The guideline describes multiple subscription models. Maybe it would be easier for the user of this API if relevant information was written here instead,

[eric-murray]

I updated the links to be hash-links to the most appropriate numbered section in the design guidelines. Unfortunately the sections on instance-based and resource-based subscriptions are not numbered, so cannot be directly hash-linked. This could be changed.

My preference is not to copy over information from the design guidelines, because then we have a maintenance issue whenever the design guidelines are updated. Of course, by hash-linking there is still an issue if the section names or numbers change, but probably more manageable.

Anyway, that's my proposal, but happy to discuss.

[JoachimDahlgren]

Small typo missing a "w" in networks.

[eric-murray]

Fixed

[fernandopradocabrillo]

@fernandopradocabrillo (additionnally to @eric-murray comment) Fair point but one question for you before to update this doc: this point shouldn't be documented in the design guideline API ? As it should be common for all subscriptions it seems to me that is should be there. Thoughts?

@bigludo7 Yes! I think it definitely should be well documented in the guidelines since it is going to be the same behaviour for all the APIs. But, don't you think we should mention it briefly here too? To make it easier for the implementer to understand how it works.

@fernandopradocabrillo Can you propose some additional text? I don't think we need to document optional behaviour on the API consumer's part.

@eric-murray I was considering that it would be better if the documentation reflects the full behaviour of the API and what should be expected in each scenario. In this case indicating that a GET must be done to ensure the existence or successful deletion of the subscription is to point out that for asynchronous scenarios, there is no other way to check that the operation has been performed successfully. What do you think?

[eric-murray]

@fernandopradocabrillo
I agree with @bigludo7 that details around the operation of the subscription / notification mechanism are best put in the API design guidelines themselves and linked to from the documentation here. So I'd suggest to modify that document, as the behaviour applies to all resource-based subscriptions, and the Device Status API will pick up on the documentation through the hash-links.

[akoshunyadi]

Is /notifications mandatory?
Doesn't the webhook/notificationUrl define the complete url?

[eric-murray]

Hi @akoshunyadi

Good question. I had assumed that it was mandatory for the notification endpoint name to match the name defined in the API definition. So either the API consumer includes that in notificationUrl, or it gets added on automatically to whatever is specified.

However, looking at the example in the CAMARA Design Guidelines, I see that the API consumer specified "notificationUrl": "https://application-server.com"but the notification was sent to https://application-server.com/v0/notifications.

So now I'm asking "where did the v0 come from?". Rules around constructing the full notification URL need to be clarified.

But, despite that, my assumption was that if the API consumer had instead specified "notificationUrl": "https://application-server.com/notification", then that is exactly where the notification would be sent (rather than "https://application-server.com/notification/notification").

[Sachinsiso]

As per my understanding, when the API consumer registers its webhook/notification URL with the API provider, the provider should perform validation on the URL to ensure it meets the required format that should be properly documented at our end. If there are any issues, such as missing components or incorrect structure (missing “/notification”), the API provider should respond with an appropriate error message indicating what went wrong and how the API consumer can correct the URL.

Also it may create some confusion to have extra endpoint in our swagger file as if client try to access “Try it out” feature then this extra endpoint will take him/her to http://localhost:9091//device-status/v0/notifications) that can confuse him/her.

[akoshunyadi]

In QoD we decided not to add or expect "/notifications", just use the provided callback url
notifications:
"{$request.body#/webhook/notificationUrl}":

[eric-murray]

Given that we have an explicit /notifications endpoint defined here, and given the resource-based subscription example used in the design guidelines, I think we have to assume that the notification endpoint must end with /notifications.

The question is what we should do if the notificationUrl provided by the API consumer does not end with /notifications? The example in the design guidelines suggests (more or less) that we should just add /notifications to the endpoint provided by the API consumer.

But I agree with @Sachinsiso that a better option might be to reject it and get the API consumer to try again to avoid the risk that the API consumer has misunderstood how to implement this endpoint. This interpretation would require a change in the design guidelines.

I'd suggest to wait until @bigludo7 returns before discussing further, and keep this PR open in the meantime.

[PedroDiez]

Hi,

will also align this with @fernandopradocabrillo, anycase I think it is good to wait until Ludovic returns.

I consider the comment is valid, and probably this can generate misunderstanding. When sending notification from Telco Operator to client, the endpoint is settled by {$request.body#/webhook/notificationUrl}, so really not required any append like "/notifications". The notification is sent to the endpoint provided by the client.

Think it is good time to have this discussion given there is an issue where a "traversal" API is being discussed/defined to cover this flow: camaraproject/Commonalities#8

[bigludo7]

I've identified here 2 topics all related to events:

1. use of callbacks: OAS definition (like in QoD API) or add a specific /notification endpoint (like we have there). I guess the decision should be at CAMARA level and not per API. @PedroDiez raised issue camaraproject/Commonalities#44 to track this.
2. definition of notification url - I tend to think that we need to fix this and proposal by @PedroDiez to use {$request.body#/webhook/notificationUrl} as it is defined in QoD v0.0.9 seems to be a good approach.

Additionally there is a third point (3.) raised in commonalities that could impact this API: should we keep current notification model (https://github./camaraproject/Commonalities/pull/41) or move to cloud event notification (camaraproject/Commonalities#43).

I suggest to wait until commonalities outputs for 1. & 3. before to move forward for this PR.

[bigludo7]

To provide clarification about Notification update
Aligned with camaraproject/Commonalities#56

[bigludo7]

I propose to close this issue as the discussion is not about the documentation but to the shift to CloudEvent. I will open another PR with the new yaml proposal.

[bigludo7]

New PR will be created for #74