camaraproject / Commonalities

Repository to describe, develop, document and test the common guidelines and assets for CAMARA APIs
Apache License 2.0
11 stars 25 forks source link

Request body is required but all properties are optional #247

Open bigludo7 opened 2 months ago

bigludo7 commented 2 months ago

Problem description A small one but need global alignement in all our API. This one was raised by @fernandopradocabrillo in SIM Swap API and we got the same question with @FabrizioMoggio in Call Forwarding API.

The request body is required for an api, but all attributes are optional. What should be the body when the client doesn't need to send any attributes?

We have 2 options:

  1. Leave it as required and force the client to send an empty body -> {}
  2. Remove the requirement (required=true for the requestBody) and let the client send a POST without body

Expected action Define the behaviour when the client doen't need to send any request body param. It could a sentence in the design guideline. As of now with @fernandopradocabrillo we have a preference for option 1 in sim-swap.

Additional context It is important to define this behavior to include the scenarios in the test plan.

uwerauschenbach commented 2 months ago

My preference is option 1. Empty map, if all entries are optional.

Kind regards, Uwe

From: Ludovic Robert @.> Sent: Wednesday, July 10, 2024 5:44 PM To: camaraproject/Commonalities @.> Cc: Subscribed @.***> Subject: [camaraproject/Commonalities] Request body is required but all properties are optional (Issue #247)

Problem description A small one but need global alignement in all our API. This one was raised by @fernandopradocabrillohttps://github.com/fernandopradocabrillo in SIM Swap APIhttps://github.com/camaraproject/SimSwap/issues/118 and we got the same question with @FabrizioMoggiohttps://github.com/FabrizioMoggio in Call Forwarding API.

The request body is required for an api, but all attributes are optional. What should be the body when the client doesn't need to send any attributes?

We have 2 options:

  1. Leave it as required and force the client to send an empty body -> {}
  2. Remove the requirement (required=true for the requestBody) and let the client send a POST without body

Expected action Define the behaviour when the client doen't need to send any request body param. It could a sentence in the design guideline. As of now with @fernandopradocabrillohttps://github.com/fernandopradocabrillo we have a preference for option 1 in sim-swap.

Additional context It is important to define this behavior to include the scenarios in the test plan.

— Reply to this email directly, view it on GitHubhttps://github.com/camaraproject/Commonalities/issues/247, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAKRTHIZMU7UVTXJQC74GLDZLVJCLAVCNFSM6AAAAABKVF3CCOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQYDCMJQGI3TANQ. You are receiving this because you are subscribed to this thread.Message ID: @.**@.>>

FabrizioMoggio commented 2 months ago

In the Call Forwarding Signal API we are adopting option 1

PedroDiez commented 2 months ago

We also prefer option 1

eric-murray commented 2 months ago

In Device Identifier, we had a subtly different problem because the request body is the Device object, and so the optional fields of the body are the fields of the Device object. Because of the minProperties: 1 directive, this means an empty body is not a valid option. Hence the only option for the API consumer if they don't want to send a device subscription parameter (because they are using a 3-legged token) is not to send any body at all.

I decided this was a better solution because it saves on a couple of request headers (Content-Length and Content-Type if I remember correctly), and so should be more efficient. And this will be the normal way of calling this API, given that the Device object contains PII, and so 3-legged tokens should be used.

I'm happy to revisit this decision if there are good arguments in favour of empty body over no body, but otherwise any text that goes into the design guidelines has to be careful not to effectively mandate that POST requests always have a body, even if empty.

bigludo7 commented 2 months ago

I think @eric-murray you trigger another point about inconsistency in our pattern.

In device identifier and in device status (for device-roaming-status for example) we have only the device structure in request but not same model.

Device identifier:

  "phoneNumber": "123456789",
  "networkAccessIdentifier": "123456789@domain.com",
  "ipv4Address": {
    "publicAddress": "84.125.93.10",
    "publicPort": 59765
  },
  "ipv6Address": "2001:db8:85a3:8d3:1319:8a2e:370:7344"
}

device-roaming-status:

  "device": {
    "phoneNumber": "+123456789",
    "networkAccessIdentifier": "123456789@domain.com",
    "ipv4Address": {
      "publicAddress": "84.125.93.10",
      "publicPort": 59765
    },
    "ipv6Address": "2001:db8:85a3:8d3:1319:8a2e:370:7344"
  }
}

Both implement Device as specified - probably we need to align?

uwerauschenbach commented 2 months ago

You write that you save the Content-Length header. But wouldn’t you have to send that header even if you have an empty request message (i.e. set it to 0)? Section 8.6 in RFC 9110 states that.

Kind regards, Uwe

From: Eric Murray @.> Sent: Thursday, July 18, 2024 6:10 PM To: camaraproject/Commonalities @.> Cc: Uwe Rauschenbach (Nokia) @.>; Comment @.> Subject: Re: [camaraproject/Commonalities] Request body is required but all properties are optional (Issue #247)

In Device Identifier, we had a subtly different problem because the request body is the Device object, and so the optional fields of the body are the fields of the Device object. Because of the minProperties: 1 directive, this means an empty body is not a valid option. Hence the only option for the API consumer if they don't want to send a device subscription parameter (because they are using a 3-legged token) is not to send any body at all.

I decided this was a better solution because it saves on a couple of request headers (Content-Length and Content-Type if I remember correctly), and so should be more efficient. And this will be the normal way of calling this API, given that the Device object contains PII, and so 3-legged tokens should be used.

I'm happy to revisit this decision if there are good arguments in favour of empty body over no body, but otherwise any text that goes into the design guidelines has to be careful not to effectively mandate that POST requests always have a body, even if empty.

— Reply to this email directly, view it on GitHubhttps://github.com/camaraproject/Commonalities/issues/247#issuecomment-2236965301, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAKRTHLEWFO4MPN2HQ5BRN3ZM7SFDAVCNFSM6AAAAABKVF3CCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZWHE3DKMZQGE. You are receiving this because you commented.Message ID: @.**@.>>

eric-murray commented 2 months ago

@uwerauschenbach An empty body requires a Content-Length : 0 header, whereas no body at all requires no Content-Length header. Indeed, it is the presence of the Content-Length : 0 header that defines the request has a body (albeit empty). Nothing else is sent as the empty body itself.

Try wiresharking the following commands and tell me what you see:

curl -X POST http://localhost       # No body
curl --data "" http://localhost     # Empty body
uwerauschenbach commented 2 months ago

Hi Eric, and all,

Thanks, that’s a precise statement.

I don’t understand all the details of device identifier, but here’s my general view on the possibilities to omit all information in a request body:

  1. If you have a required structure that has all fields optional, and you choose to send none of them, then you send an empty structure (in JSON: {})
  2. If you have an optional structure that requires at least one field, you may send an empty body when you choose to send no information.
  3. If you have an optional structure that has all fields optional, you may choose: empty structure or empty body.

Ideally, we agree on a common pattern across all APIs, and document this in Commonalities. Less ideally, we document the choices that are available, and leave it to the API to choose (but then, the chosen pattern needs to be consistent across the whole API). Among these, we might call out one pattern as the recommended one.

On the choice of the patterns themselves, I don’t care so much about header overhead when looking at conceptual clarity. In HTTP/2, headers can be compressed. So there are ways to optimize.

My preference, as indicated, is #1. An empty set of parameters is still an “empty set” and signalled as such, i.e. as {}.

2 treat the semantics “empty set” different from “non-empty set”. Here, an empty set is not signalled as a set, but as a message containing nothing. It could still be chosen if needed for legacy support.

3 Should be avoided. It allows two different representations for the same concept.

My 2 ct.

Kind regards, Uwe

From: Eric Murray @.> Sent: Friday, July 19, 2024 4:35 PM To: camaraproject/Commonalities @.> Cc: Uwe Rauschenbach (Nokia) @.>; Mention @.> Subject: Re: [camaraproject/Commonalities] Request body is required but all properties are optional (Issue #247)

@uwerauschenbachhttps://github.com/uwerauschenbach An empty body requires a Content-Length : 0 header, whereas no body at all requires no Content-Length header. Indeed, it is the presence of the Content-Length : 0 header that defines the request has a body (albeit empty). Nothing else is sent as the empty body itself.

Try wiresharking the following commands and tell me what you see:

curl -X POST http://localhost # No body

curl --data "" http://localhost # Empty body

— Reply to this email directly, view it on GitHubhttps://github.com/camaraproject/Commonalities/issues/247#issuecomment-2239326058, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAKRTHPG5MH62QO6UN6IZNLZNEPZNAVCNFSM6AAAAABKVF3CCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZZGMZDMMBVHA. You are receiving this because you were mentioned.Message ID: @.**@.>>

eric-murray commented 2 months ago

@uwerauschenbach @bigludo7

So I think the better pattern for "future-proofness" is that of device-roaming-status, as the current device-identifier definition cannot be extended with additional optional attributes without introducing breaking changes (i.e. introducing an explicit device field as well). However, it is more likely that device-identifier will drop identifiers other than phoneNumber (like sim-swap) as this is the major use case, and I think it cleaner to have:

{ "phoneNumber": "+123456789" }

rather than:

{ device: {"phoneNumber": "+123456789" }}

Currently sim-swap is required to pass a phoneNumber, but I guess that will be changed and they will have the same discussion as to whether to pass an empty JSON object or no body at all. My preference remains no body at all as this gives a minor performance advantage, but I agree this is a marginal consideration. My tests suggest sending no body saves 53 bytes over sending an empty JSON object.

So I am happy for Commonalties to align on a pattern for passing the Device object, but device-identifier may not use this object in future.

Also, I realise that your option 1 is actually "empty JSON object" (i.e. {}) rather than "empty body". An empty body itself is not a valid JSON object.

uwerauschenbach commented 2 months ago

Indeed.

From: Eric Murray @.> Sent: Monday, July 22, 2024 1:15 PM To: camaraproject/Commonalities @.> Cc: Uwe Rauschenbach (Nokia) @.>; Mention @.> Subject: Re: [camaraproject/Commonalities] Request body is required but all properties are optional (Issue #247)

@uwerauschenbachhttps://github.com/uwerauschenbach @bigludo7https://github.com/bigludo7

So I think the better pattern for "future-proofness" is that of device-roaming-status, as the current device-identifier definition cannot be extended with additional optional attributes without introducing breaking changes (i.e. introducing an explicit device field as well). However, it is more likely that device-identifier will drop identifiers other than phoneNumber (like sim-swap) as this is the major use case, and I think it cleaner to have:

{ "phoneNumber": "+123456789" }

rather than:

{ device: {"phoneNumber": "+123456789" }}

Currently sim-swap is required to pass a phoneNumber, but I guess that will be changed and they will have the same discussion as to whether to pass an empty JSON object or no body at all. My preference remains no body at all as this gives a minor performance advantage, but I agree this is a marginal consideration. My tests suggest sending no body saves 53 bytes over sending an empty JSON object.

So I am happy for Commonalties to align on a pattern for passing the Device object, but device-identifier may not use this object in future.

Also, I realise that your option 1 is actually "empty JSON object" (i.e. {}) rather than "empty body". An empty body itself is not a valid JSON object.

— Reply to this email directly, view it on GitHubhttps://github.com/camaraproject/Commonalities/issues/247#issuecomment-2242706086, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAKRTHIUUFVRUQO3UXWIXQDZNTSSPAVCNFSM6AAAAABKVF3CCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBSG4YDMMBYGY. You are receiving this because you were mentioned.Message ID: @.**@.>>