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

api - port the description document to openapi v3.1 #12983

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

commonism
Copy link

Short description

besides the required structural changes, there were additional changes

  • path parameters are defined in the PathItem instead of the Operation this eleminates repetition of the parameters the operations of a path
  • common path parameters are referenced via #/components/parameters eleminates repetition of the parameter definition ({server,zone}_id)
  • SearchResult is the discriminated union the FIXME asked for

If you look into getStats - it should be list of a discriminated union (of the statistic types) instead?
c.f. SearchResult.

Without the getStats results list and references with descriptions targetting OpenAPI3 v3.0.x would be possible as well.

Checklist

I have:

besides the required structural changes, there were additional changes
 - path parameters are defined in the PathItem instead of the Operation
   this eleminates repetition of the parameters the operations of a path
 - common path parameters are referenced via #/components/parameters
   eleminates repetition of the parameter definition ({server,zone}_id)
 - SearchResult is the discriminated union the FIXME asked for
@Habbie Habbie added the auth label Jul 4, 2023
@Habbie Habbie added this to the auth-4.9.0 milestone Jul 4, 2023
@Habbie
Copy link
Member

Habbie commented Jan 23, 2024

@kpfleming do you happen to have an opinion on this?

@kpfleming
Copy link
Contributor

kpfleming commented Jan 23, 2024

Overall I think this is a good idea, but getStats is not in fact a discriminated union in the actual API results as far as I can tell, and so that's what stopped me in the past from getting the schema to the point where a schema validator would pronounce it compliant with any version of the OpenAPI specification. When I investigated it previously (more than 2 years ago), the API operation itself would have had to be changed in order to produce a result which can be properly described by an OpenAPI schema (and the result would not be backwards compatible).

If there's a real desire to fix that, a new version of getStats (getStatsEx anyone?) could be created and documented in the schema, and getStats intentionally not documented in the schema but still supported in the implementation.

@commonism
Copy link
Author

      responses:
        '200':
          description: List of Statistic Items
          content:
            application/json:
              schema:
                type: array
                items:
                  type: object
                  discriminator:
                    propertyName: object_type
                    mapping:
                      StatisticItem: '#/components/schemas/StatisticItem'
                      MapStatisticItem: '#/components/schemas/MapStatisticItem'
                      RingStatisticItem: '#/components/schemas/RingStatisticItem'
                  oneOf:
                    - $ref: '#/components/schemas/StatisticItem'
                    - $ref: '#/components/schemas/MapStatisticItem'
                    - $ref: '#/components/schemas/RingStatisticItem'

I tested this.

I need to correct myself, the use of :<< is not valid yaml 1.2 and therefore not valid OpenAPI, I'd update this and replace with "default".

I have some unit tests, in case you are interested.
The client is python & dynamic via aiopenapi3, does not require codegen.

def test_pdns_openapi():
    from aiopenapi3 import FileSystemLoader
    from pathlib import Path
    api = OpenAPI.load_file("http://localhost:1263",
                            Path("~/workspace/pdns/docs/http-api/openapi31/authoritative-api-openapi.yaml").expanduser(),
                            loader=FileSystemLoader(Path("/"), yload=yaml.SafeLoader),
                            session_factory=wget_factory)

    api.authenticate(APIKeyHeader=SEKRET)

    Server = api.components.schemas["Server"].get_type()
    Record = api.components.schemas["Record"].get_type()
    Comment = api.components.schemas["Comment"].get_type()
    RRSet = api.components.schemas["RRSet"].get_type()
    Zone = api.components.schemas["Zone"].get_type()

    for s in api._.listServers():
        assert isinstance(s, Server)
        p = {"server_id": s.id}
        for server in api._.listServer(parameters=p):
            assert isinstance(server, tuple)
        break

    r = api._.getStats(parameters={"server_id":s.id})

    zone_id = "test.zone."
    for zone in api._.listZones(parameters=p):
        assert isinstance(zone, Zone)
        print(zone)

    try:
        r = api._.deleteZone(parameters=collections.ChainMap(p, {"zone_id": zone_id}))
    except HTTPStatusError as e:
        if e.http_status != http.HTTPStatus.NOT_FOUND:
            raise

    account = ""

    import dns.rdtypes.ANY.SOA
    import dns.rdtypes.IN.A

    soa = dns.rdtypes.ANY.SOA.SOA(rdclass=dns.rdataclass.IN, rdtype="SOA", mname=zone_id, rname=zone_id, serial=0,
                                  refresh=0, retry=0, expire=0, minimum=0).to_text()
    z = Zone(
        name=zone_id,
        kind="Master",
        nameservers=["ns.the.zone."],
        account=account,
        rrsets=[RRSet(
            name=zone_id,
            type="SOA",
            ttl=86400,
            records=[Record(
                content=soa, disabled=False
            )],
            comments=[Comment(content=".", account=account)],
            changetype="ADD"
        )],
    )

    z = z.model_validate(z)
    r = api._.createZone(data=z.model_dump(exclude_none=True), parameters=p)


    r = api._.listZone(parameters=collections.ChainMap(p, {"zone_id": zone_id, "rrsets": True}))

    z.rrsets = [
        RRSet(
            name=f"www.{zone_id}",
            type="A",
            ttl=86400,
            records=[
                Record(
                    content=dns.rdtypes.IN.A.A(
                        rdclass=dns.rdataclass.IN,
                        rdtype="A",
                        address="1.1.1.1").to_text(),
                    disabled=False
                )
            ],
            comments=[Comment(content=".", account=account)],
            changetype="REPLACE"
        )
    ]

    r = api._.patchZone(data=z.model_dump(), parameters=collections.ChainMap(p, {"zone_id": zone_id, }))
    print(r)
    r = api._.listZone(parameters=collections.ChainMap(p, {"zone_id": zone_id, "rrsets": True}))
    print(r)
    rrset = next(filter(lambda x: x.name == "www.test.zone." and x.type == "A", r.rrsets))
    assert rrset
    rr = next(filter(lambda x: x.content == "1.1.1.1", rrset.records))
    assert rr

This example is hardcded to use the 3.1 spec locally, using the yaml 1.1 loader.
aiopenapi3 does swagger 2.0 as well, so you may be able to use re-use code for the different spec versions.

If you are interested, I'd polish a little.

@kpfleming
Copy link
Contributor

Ahh, if that works then it's great, it means the schema can be made compliant without having to change the implementation :-)

@commonism
Copy link
Author

I updated the document with the changes required.

@Habbie Habbie modified the milestones: auth-4.9.0, auth-5 Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants