Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CAMARA_common.yaml: add Device headers, fix address #120

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Kevsy
Copy link
Collaborator

@Kevsy Kevsy commented Jan 10, 2024

Fixes #118 #119

What type of PR is this?

Add one of the following kinds:

  • correction
  • enhancement/feature

What this PR does / why we need it:

  • Adds HTTP Headers for Device Identifiers, for reuse in APIs with GET methods requiring protection for Device Identifiers
  • Changes instances of Addr to Address in object names.

Which issue(s) this PR fixes:

Fixes #118
Fixes #119

Special notes for reviewers:

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

Copy link
Collaborator

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

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

LGTM
But as this will impact many existing APIs (as IP addresses are often used) we need to ask other companies for their opinions.

@rartych
Copy link
Collaborator

rartych commented Jan 24, 2024

Thank you @patrice-conil for pointing this out. As this small change: Addr to Address impacts subprojects like Device Identifier, Device Status and Device Location, QoD I am adding @eric-murray @bigludo7 @jlurien and @hdamker as reviewers.

Note: for IPv6 the DeviceIpv6Address name is used.

Edits to be consistent with the current indentation
@@ -10,6 +10,31 @@ info:
version: 0.4.0
paths: {}
components:
parameters:
IPv4-Address:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using same format for all schemas? If we use UpperCamelCase for schema names, it is more coherent to use them as well here, e.g. Ipv4Address

Copy link
Collaborator Author

@Kevsy Kevsy Jan 29, 2024

Choose a reason for hiding this comment

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

By convention HTTP headers are Kebab-Case: RFC 2616, section 5.3. So IPv4-Address uses the same naming style because it is an HTTP Header. "DeviceIpv4Address" is the schema that the parameter is based on (which follows CAMARA UpperCamelCase convention for schema names).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Kevin,

Understood and OK to use kebab-case for the parameter property value. I checked with Jose, he was refering only to use UpperCamelCase for the schema name, just for aligment. Usually code generators also works better with schemas named following Camelcase. So it would read (just removing "-", for the new schemas included):

IPv4Address:
in: header
name: IPv4-Address
required: false
schema:
$ref: "#/components/schemas/DeviceIpv4Address"

WDYT?

@jlurien
Copy link
Contributor

jlurien commented Jan 24, 2024

Thank you @patrice-conil for pointing this out. As this small change: Addr to Address impacts subprojects like Device Identifier, Device Status and Device Location, QoD I am adding @eric-murray @bigludo7 @jlurien and @hdamker as reviewers.

Note: for IPv6 the DeviceIpv6Address name is used.

The change affects code generation as those names are schema names that may be mapped to classes. The API itself does not change.

Copy link
Contributor

@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.

  • Adopt UpperCamelCase for parameters schemas

@Kevsy
Copy link
Collaborator Author

Kevsy commented Jan 29, 2024

  • Adopt UpperCamelCase for parameters schemas

The schemas for the new parameters do have UpperCamelCase names.

But the parameter names (that reference the schemas) have Kebab-Case names, because they are specifically HTTP header parameters, which follow the convention in RFC 2616, section 5.3.

@PedroDiez
Copy link
Collaborator

Another comment: As it has been defining models por "headers" params, this model is not wide enough to cover with serialziation topics:

https://swagger.io/docs/specification/describing-parameters/#header-parameters
https://swagger.io/docs/specification/serialization/

Header Parameters

Header parameters always use the simple style, that is, comma-separated values. This corresponds to the {param_name} URI template. An optional explode keyword controls the object serialization. Given the request header named X-MyHeader, the header value is serialized as follows:

style | explode | URI template | Primitive value X-MyHeader = 5 | Array X-MyHeader = [3, 4, 5] | Object X-MyHeader = {"role": "admin", "firstName": "Alex"} -- | -- | -- | -- | -- | -- simple * | false * | {id} | X-MyHeader: 5 | X-MyHeader: 3,4,5 | X-MyHeader: role,admin,firstName,Alex simple | true | {id*} | X-MyHeader: 5 | X-MyHeader: 3,4,5 | X-MyHeader: role=admin,firstName=Alex

* Default serialization method

Header Parameters Header parameters always use the simple style, that is, comma-separated values. This corresponds to the {param_name} URI template. An optional explode keyword controls the object serialization. Given the request header named X-MyHeader, the header value is serialized as follows:

style explode URI template Primitive value X-MyHeader = 5 Array X-MyHeader = [3, 4, 5] Object X-MyHeader = {"role": "admin", "firstName": "Alex"}
simple * false * {id} X-MyHeader: 5 X-MyHeader: 3,4,5 X-MyHeader: role,admin,firstName,Alex
simple true {id*} X-MyHeader: 5 X-MyHeader: 3,4,5 X-MyHeader: role=admin,firstName=Alex

  • Default serialization method

@PedroDiez
Copy link
Collaborator

To summarize the comments:

- Comment 1: It can be used "UpperCamelCase" as model for schema convention which does not preclude to name a parameter which is a header as kebab-case notation

  IPv4-Address:
   in: header
   name: IPv4-Address
   required: false
   schema:
    $ref: "#/components/schemas/DeviceIpv4Address"

- Comment 2: With regards to the schema model to refer to a "header".

  • It would be needed to align on the serialization mode. (probably explode=true)
  • At least IPv6-Address "header" definition points to schema that is an object so this would need to define the serialziation mode

cc @Kevsy (we talked in 25th March Commonalities meeting about this PR)

@Kevsy
Copy link
Collaborator Author

Kevsy commented Apr 15, 2024

@PedroDiez , @rartych , please can you propose any further changes as a new commit of the PR? I believe Comment 1 above is already implemented in the current commit, but I'm not sure what is the decision on Comment 2.

@rartych
Copy link
Collaborator

rartych commented May 22, 2024

@PedroDiez @Kevsy I see headers for device identification are used in https://github.com/camaraproject/SimpleEdgeDiscovery/pull/4/files
All defined headers are simple strings, so the serialization is not needed.

The IPv4-Address header is defined as follows:

        - name: IPv4-Address
          in: header
          required: false

          schema:
            $ref: "#/components/schemas/SingleIpv4Addr"

Do you think this approach can be used here? But replacing SingleIpv4Addr with proposed SingleIpv4Address ?
This can be also the move in the direction indicated in #171 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
7 participants