api/swagger - set type & additionalProperties

[commonism]

Short description

This PR addresses missing type fields in the swagger description document.
Additionally RRSet.changetype is not to be "required" as it is empty when listing.

Checklist

I have:

• read the CONTRIBUTING.md document
• [-] compiled this code
• [-] tested this code
• [-] included documentation (including possible behaviour changes)
• [-] documented the code
• [-] added or modified regression test(s)
• [-] added or modified unit test(s)

The [-] checks do not apply.

[Habbie]

@kpfleming could we get your eyes on this? :)

[Habbie]

The [-] checks do not apply.

I'd say testing definitely applies. Did you test or validate this in any way? :)

[commonism]

I validated the description document.

[kpfleming]

This looks reasonable to me, with the caveat that setting additionalProperties: False will only benefit validation tools which are producing API requests; the actual API implementation will ignore any 'additional' properties in the objects passed to it, I believe.

[kpfleming]

@Habbie has noted that RRSet.changetype is in fact required when it is used in a request, even though it may not be present in a response. In order for validation to understand both cases, it seems that two definitions will be required, or if each of those definitions would be used only once then the schema moved into the actual points of use, with a comment in each place documenting why the apparent duplication exists.

[commonism]

This looks reasonable to me, with the caveat that setting additionalProperties: False will only benefit validation tools

It helps making sure the description document matches the protocol implmenented when clients bail out for unknown properties.
I've just seen last_check in Zone, it's missing in the dd and unnoticed with additionalProperties != False.

@Habbie has noted that RRSet.changetype is in fact required when it is used in a request, even though it may not be present in a response.

Yes.
Possible via allOf or duplication of the record.
I would address this by having adequate error messages from the backend for missing/bad parameters instead of crustifying the description document.

[kpfleming]

I would address this by having adequate error messages from the backend for missing/bad parameters instead of crustifying the description document.

The goal of the document is describe the API and to allow validation tools to confirm that requests and responses are compliant. If the field is required in a request, then the only way for a tool to validate the request is for the document to indicate that it is required.

An error from the API endpoint indicating that a required property was not supplied is certainly welcome, but if it conflicts with the description document then it will potentially be confusing to developers using the API.

[commonism]

I agree with the intention of best-possible-validation and would propose something along …

    Item:
      type: object
      additionalProperties: False
      required: [ data ]
      properties:
        data:
          type: string
    ItemCreate:
      type: object
      additionalProperties: False
      required: [ data, createonly ]
      properties:
        createonly:
          type: string
      allOf:
        - $ref: "#/components/schemas/Item"

but as the entity in question is embedded in Zone for (put|patch|create)Zone, this would require complete duplication of Zone to embed the changetype'd RRSet.

As for parameters, the use of Can only be used together with … is accepted - instead of having multiple operations for different parameter combinations, have changetype required but nullable maybe?

[kpfleming]

Ugh... the inclusion into Zone does make this more complicated.

I think the idea of making changetype required, but emitted as null in a response, would be a simpler solution, but is a small change to the API codebase.

[commonism]

Using swagger nullable ain't easy as well …
https://jane.readthedocs.io/en/latest/tips/nullable.html

I'd go to v3.1.

[kpfleming]

Unfortunately doing that will require making changes to the API itself, as the current schema is not even fully 2.0 compliant, and will definitely not be 3.x compliant.

[zeha]

I don't think we want to emit additional json properties in RRsets, just to make the doc/validation tools happy. For large zones, the response size is already a problem.

Maybe an ResponseRRSet object is a better idea.

[commonism]

This would include ResponseZone as well as it embeds ResponseRRSet.

Not "required" is still an option - it will fix the dd to match the current implementation.

[zeha]

This would include ResponseZone as well as it embeds ResponseRRSet.

A zone response is already different from a zone in a request, so that would only make sense.

[commonism]

Is there any documentation on the difference so I can have a look and adjust accordingly?