[qod-api.yaml]: IPv6-object will be generated instead of a String

[maxl2287]

Problem

With the current definition of having two patterns in the "allOf"-definition of IPv6, the code-generator will generate a senseless Object of IPv6Addr, which cannot be used and which does not have anything relevant inside.

@Schema(name = "Ipv6Addr", description = "IPv6 address, following IETF 5952 format, may be specified in form <address/mask> as:   - address - The /128 subnet is optional for single addresses:     - 2001:db8:85a3:8d3:1319:8a2e:370:7344     - 2001:db8:85a3:8d3:1319:8a2e:370:7344/128   - address/mask - an IP v6 number with a mask:     - 2001:db8:85a3:8d3::0/64     - 2001:db8:85a3:8d3::/64 ")
@Generated(value = "org.openapitools.codegen.languages.SpringCodegen", date = "2023-01-13T13:15:16.364985400+01:00[Europe/Berlin]")
public class Ipv6Addr {

  @Override
  public boolean equals(Object o) {
    if (this == o) {
      return true;
    }
    if (o == null || getClass() != o.getClass()) {
      return false;
    }
    return true;
  }

  @Override
  public int hashCode() {
    return Objects.hash();
  }

  @Override
  public String toString() {
    StringBuilder sb = new StringBuilder();
    sb.append("class Ipv6Addr {\n");
    sb.append("}");
    return sb.toString();
  }

  /**
   * Convert the given object to string with each line indented by 4 spaces
   * (except the first line).
   */
  private String toIndentedString(Object o) {
    if (o == null) {
      return "null";
    }
    return o.toString().replace("\n", "\n    ");
  }
}

Means that IPv6 cannot be used as an input-variable at all.

https://github.com/camaraproject/QualityOnDemand/blob/5e9d512c0d5331f787cae274c613810052d70d27/code/API_definitions/qod-api.yaml#L398

Suggested fix

Remove the allOf and just use one pattern which is the combination of both patterns.

    Ipv6Addr:
      type: string
      format: ipv6
      pattern: '(^((:|(0?|([1-9a-f][0-9a-f]{0,3}))):)((0?|([1-9a-f][0-9a-f]{0,3})):){0,6}(:|(0?|([1-9a-f][0-9a-f]{0,3})))(\/(([0-9])|([0-9]{2})|(1[0-1][0-9])|(12[0-8])))?$)|(^((([^:]+:){7}([^:]+))|((([^:]+:)*[^:]+)?::(([^:]+:)*[^:]+)?))(\/.+)?$)'
      example: "2001:db8:85a3:8d3:1319:8a2e:370:7344"
      description: |
        IPv6 address, following IETF 5952 format, may be specified in form <address/mask> as:
          - address - The /128 subnet is optional for single addresses:
            - 2001:db8:85a3:8d3:1319:8a2e:370:7344
            - 2001:db8:85a3:8d3:1319:8a2e:370:7344/128
          - address/mask - an IP v6 number with a mask:
            - 2001:db8:85a3:8d3::0/64
            - 2001:db8:85a3:8d3::/64

[eric-murray]

Hi @maxl2287

This "allOf" type of construct for patterns originates from 3GPP (see TS 29.571). Although not currently used by the T8 for IPv6 addresses (that accepts any string), it can be expected that format restrictions imposed by the combined patterns will be enforced by 3GPP service based interfaces, and eventually by the T8 itself (which is long overdue an update). The pattern was adopted so that we can avoid having to do additional parameter checking before passing IPv6 addresses onto the NEF (although, having said that, it was agreed to make a /128 mask optional, so really that should be added if the client has omitted it).

I've no problem to replace the "allOf" construct with an equivalent single line pattern, but your proposed pattern does not achieve that. By concatenating the two patterns with an "OR", you are effectively replacing the "allOf" with an "anyOf" - not the same at all. So an alternative single line pattern needs to be proposed.

More generally, being an open-source project, swagger-codegen will always fail for some otherwise valid OAS definitions as the number of active developers is limited. I'm reluctant to be restricted (in general) by the capabilities of tooling if that limits or obscures what would otherwise be a clear definition that is anyway meant to be read by the developer.

And the intention of the 3GPP pattern construct is very clear, even if the tooling cannot compile it. Internally within Vodafone, we also encountered this problem, but simply replaced the combined IPv6 patterns with a less restrictive one. So the restrictions on IPv6 formatting remains clear to the client (i.e. use IETF 5952 compliant formatting), even if we are not actually properly enforcing that at the moment.

[patrice-conil]

Hi @maxl2287 @eric-murray ,

We have also experienced disagreements with openapi-generator. On our side we decided to use this pattern: pattern: '^((:|(0?|([1-9a-f][0-9a-f]{0,3}))):)((0?|([1-9a-f][0-9a-f]{0,3})):){0,6}(:|(0?|([1-9a-f][0-9a-f]{0,3})))(\/(([0-9])|([0-9]{2})|(1[0-1][0-9])|(12[0-8])))?$' And we've also replaced all occurences of oneOf (enum, string) ... by a simple string.

My 2 cents Patrice

[jlurien]

Hi,

We have also received feedback due to problems parsing this allOf with 2 pattterns, so I think we should convert it to a single line pattern.

[maxl2287]

@jlurien @eric-murray

so what do to with this issue? Is it okay to put it in one line, as we saw that several colleagues have the same issue with this "allOf"?

Ipv6Addr:
      type: string
      format: ipv6
      pattern: '(^((:|(0?|([1-9a-f][0-9a-f]{0,3}))):)((0?|([1-9a-f][0-9a-f]{0,3})):){0,6}(:|(0?|([1-9a-f][0-9a-f]{0,3})))(\/(([0-9])|([0-9]{2})|(1[0-1][0-9])|(12[0-8])))?$)|(^((([^:]+:){7}([^:]+))|((([^:]+:)*[^:]+)?::(([^:]+:)*[^:]+)?))(\/.+)?$)'
      example: "2001:db8:85a3:8d3:1319:8a2e:370:7344"
      description: |
        IPv6 address, following IETF 5952 format, may be specified in form <address/mask> as:
          - address - The /128 subnet is optional for single addresses:
            - 2001:db8:85a3:8d3:1319:8a2e:370:7344
            - 2001:db8:85a3:8d3:1319:8a2e:370:7344/128
          - address/mask - an IP v6 number with a mask:
            - 2001:db8:85a3:8d3::0/64
            - 2001:db8:85a3:8d3::/64

[eric-murray]

Hi @maxl2287

As I mentioned previously, an OR (|) in a pattern is equivalent to anyOf not allOf, so that pattern doesn't work.

How about just the first part of the 3GPP pattern (with optional subnet)? '^((:|(0?|([1-9a-f][0-9a-f]{0,3}))):)((0?|([1-9a-f][0-9a-f]{0,3})):){0,6}(:|(0?|([1-9a-f][0-9a-f]{0,3})))(\/(([0-9])|([0-9]{2})|(1[0-1][0-9])|(12[0-8])))?$'

I don't know what additional constraints the second part adds, and don't have time to run tests, but maybe it can be dropped.

[jlurien]

Another option would be to avoid providing a pattern, if we are not sure of using the correct one, and rely on format: ipv6. ipv6 as a format is not explicitly specified in https://spec.openapis.org/oas/v3.0.0#dataTypeFormat, but it allows to use other values that may be supported by an implementation. ipv6 is not uncommon for JSON-schema: https://json-schema.org/understanding-json-schema/reference/string.html#ip-addresses.

Indeed we are already using format: uuid for SessionId, which is not explicitly specified in OAS3 either,.

[eric-murray]

@jlurien There is a lot to be said in favour of that approach. However, first we should agree which of the various IETF documents that define valid IPv6 address formats should be followed. The link you give references RFC 2373, the latest JSON draft references RFC 4291, whilst the 3GPP pattern is trying to enforce RFC 5952.

My personal preference is for IETF 5952, as we can be fairly sure that 3GPP network elements (such as the NEF) will accept this. The risk of relying solely on format: ipv6 is that we may end up with implementation differences, with some specific formats being accepted by some implementations and rejected by others The only way to avoid this is to specify that ANY valid IPv6 format should be accepted. Each API provider would have to come up with their own means of enforcing this, as I doubt there is a regex pattern that would accept any valid IPv6 format but reject all others.

The value of specifying a pattern is that each implementation should accept and reject the same formats.

[tlohmar]

The format 'ipv6' does not allow an netmask.

[hdamker]

The format 'ipv6' does not allow an netmask.

That's addressed within #153. Beyond that the issue is staled and will be closed as discussed.