search

camaraproject / CallForwardingSignal

Repository to describe, develop, document and test the Call Forwarding Signal API
Apache License 2.0
2 stars 3 forks source link

Optional operations #37

Open bigludo7 opened 1 month ago

bigludo7 commented 1 month ago

Problem description As we have 2 operations defined we can assume that some implementations will not provide both. In order to allow this we must add error code 501 Not Implemented as an error for both endpoint. This is defined in the guideline in the table here.

Possible evolution Add 501 error code

Alternative solution

Additional context

FabrizioMoggio commented 1 month ago

Similar discussion in the Edge Cloud WG for the TI API:

https://github.com/camaraproject/EdgeCloud/discussions/218#discussioncomment-9578300

bigludo7 commented 1 month ago

@FabrizioMoggio Reading your comment on this discussion for TI API and I share your understanding. Perhaps we took for granted something which is not yet.

I found this discussion in commonalities: https://github.com/camaraproject/Commonalities/discussions/160 This is quite interesting. As far as I understood, if we use 3-legs we'll not have the problem because it will be handle thru the token. We can rebound on this discussion to get more details and keep this issue open for now without changing the yaml.

FabrizioMoggio commented 1 week ago

According to : https://github.com/camaraproject/Commonalities/discussions/160#discussioncomment-9619925

Possible solution: is to use 404 (resource not found). The resource not found is: CallForwardingSignal

FabrizioMoggio commented 4 days ago

according to: https://wiki.camaraproject.org/display/CAM/2024-07-02+CFS+Minutes

We will adopt a solution for the Alpha release. Even if the discussion in Commonalities is not closed.

FabrizioMoggio commented 2 days ago

Considering: https://github.com/camaraproject/Commonalities/discussions/160#discussioncomment-9945260

are we really sure we need endpoint two (unconditional) as optional?

In terms of implementation in the Transformation Function I don't see it technical reasons for not having it

bigludo7 commented 2 days ago

Why not re-opening the 2 yaml option in this case? I solves the problem, we have UC for both, and it did no slow dow api adoption?

As we discussed in the past the UC are distinct for the use of this 2 endpoints. As such it is perfectly valid for a company to only need to solve one UC and not the other.

FabrizioMoggio commented 2 days ago

we need to maintain two YAMLs :-). Is this worthen?

This is the current situation:

First of all we need to be sure if we want to have some operation as optional. This can be for technical or business reasons.

Second if we go for "optional" we have the following options (under discussion here: https://github.com/camaraproject/Commonalities/discussions/160#). I summarize them here, with no comment, just for alignment:

  1. optional endpoint not implemented, access denial to the operation in the authentication in the authorization flow
  2. optional endpoint implemented but void, just returning an error code ("operation not supported")
  3. One YAML for each operation
FabrizioMoggio commented 2 days ago

My personal position:

I don't see any technical or business reason strong enough to justify the burden.

If we decide to have and endpoint as optional we must follow what it will be defined in Commonalities.

Anyway my personal preference goes to option 2. I see it easier to document and to explain to the Developer with respect to option 1 and it is easier to maintain with respect to option 3.

bigludo7 commented 2 days ago

For me as we have UC that work only with one of them they should be optional. As as as I know, our anti fraud UC that was btw the one initially triggering our work, requires only unconditional -- right?

Regarding the option we circle back & forth for month in device status before to move to distinct yaml. Same issue but distinct outcome in SIM Swap (one yaml - 2 endpoints). We did not have a clear answer in Commonalities - Why not getting TSC view on this ? @hdamker do you think we can get have opinion in TSC?

FabrizioMoggio commented 2 days ago

@bigludo7 I see your point on linking the endpoints to the Use Cases. So it is a business reason driving us to go for the "optional" approach. I also support to your proposal to escalate to TSC.

hdamker commented 2 days ago

@bigludo7 @FabrizioMoggio I support the proposal to bring the topic - in a generic form - into the TSC. It's about "atomic" versus "combined" APIs with optional supported endpoints. Would be good to have an issue with a problem statement and a solution proposal (or multiple options) within https://github.com/camaraproject/Governance. If possible also with a proposal where and how to document the result (ProjectCharter? Guideline in Commonalities?).

My personal preference are atomic APIs which are completely implemented. The main reasons is for me that otherwise an aggregation across operators is hard to achieve. The effort to maintain multiple YAML files can step by step be reduced by more automation.

FabrizioMoggio commented 2 days ago

Considering our time plan for the API development, to join Fall24 (https://github.com/camaraproject/CallForwardingSignal/discussions/38#discussioncomment-9956183)

And considering the agreement in the last call (https://wiki.camaraproject.org/display/CAM/2024-07-02+CFS+Minutes): "Optional Operation: the discussion Commonalities is not yet closed, anyway we will adopt a solution for Alpha with the proper documentation"

We should anyway move forward with a temporary solution. Considering the current feedbacks in this discussion and in https://github.com/camaraproject/Commonalities/discussions/160#, the options are:

  1. 501 error code returned by the endpoint as defined in CAMARA
  2. two YAMLs
  3. authorization denial

I suggest to choose, at this stage, the one minimizing the effort, and already in the Guidelines

PROPOSAL: So I suggest to temporarily adopt option 1 (501).

Editor note: I have modified this message after a further internal discussion in the sub project team.

bigludo7 commented 2 days ago

@hdamker I can do it at least to illustrate the issue & list proposal to solve it. I guess the result could be also impact the API backlog (when we deide for a new API we can discuss this)

@FabrizioMoggio I support your proposal... as it was mi initial proposal to add this 501 :)

FabrizioMoggio commented 2 days ago

I'm wondering: because 501 is not something specific in our API but it is foreseen by the CAMARA Guidelines, do we need to explain in the documentation section (at the beginning of the YAML) that some endpoints are mandatory while others are optional? I mean, the Developer is totally aware of that if an endpoint returns 501.

endpoint returning 501: optional endpoint not returning 501: mandatory

So I propose not to give too much emphasis on this in the documentation, also considering that maybe we need to change the approach when #160 is closed.

FabrizioMoggio commented 1 day ago

Implemented with: https://github.com/camaraproject/CallForwardingSignal/pull/48