OAS: Fix discriminator field definition for Artifact

[isinyaaa]

This fixes an improper polymorphic type definition by moving its discriminator to the composing types.

There's also a small fix for ignoring coverage.html, that is generated when running make test-cover.

Description

This problem was spotted while using the Kiota generator, and can be traced back to the following pattern,
which is incompatible with JSONSchema.

How Has This Been Tested?

Following the same steps laid out on the last PR referencing OAS #17:

make FORCE_SERVER_GENERATION=true clean build
make test

then,

1. start an MLMD server
2. run make proxy
3. run robot test/robot/ && TEST_MODE=python robot test/robot/MRandLogicalModel.robot

Merge criteria:

• The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work

[dhirajsb]

@isinyaaa I noticed this PR is moving the discriminator to derived types. How does that help, or what is the difference?

[lampajr]

Hi @isinyaaa , thanks a lot for this PR.

Overall the change looks good to me, I added a comment whether we should also add the artifactProperty i nthe default obj (outside the allOf) which seems making the WARN disappear on openapi generator side (but I don't know on Kiota side)

[lampajr]

I am still not sure if that is really enough or not because even with this change (which makes totally sense to me) I see:

[main] WARN  o.o.codegen.DefaultCodegen - 'Artifact' defines discriminator 'artifactType', but the referenced schema 'ModelArtifact' is incorrect. artifactType is missing from the schema, define it as required and type string
[main] WARN  o.o.codegen.DefaultCodegen - 'Artifact' defines discriminator 'artifactType', but the referenced schema 'DocArtifact' is incorrect. artifactType is missing from the schema, define it as required and type string

from the openapi generator logs, which seems to be fixed when adding artifactType as property before the allOf keyword:

    ModelArtifact:
      description: An ML model artifact.
      type: object
      required:
        - artifactType
      properties:
        artifactType:
          type: string
          default: "model-artifact"
      allOf:
        - $ref: "#/components/schemas/BaseArtifact"
        - $ref: "#/components/schemas/ModelArtifactCreate"
        - type: object
          properties:
            artifactType:
              type: string
              default: "model-artifact"
...
    DocArtifact:
      description: A document.
      type: object
      required:
        - artifactType
      properties:
        artifactType:
          type: string
          default: "doc-artifact"
      allOf:
        - $ref: "#/components/schemas/BaseArtifact"
        - type: object
          properties:
            artifactType:
              type: string
              default: "doc-artifact"

TBH I am not that sure that having artifactType defined in multiple places is correct but just thinking loud here.

[lampajr]

After some discussion with Peruffo we agreed that the following specs are equivalent each other, but keeping the artifactType to the leaf obj should make the WARN disappear on server side as well.

The spec defined in this PR where we still have the WARN on server side model generation (actually the spec is fine, we just have the WARN)

    ModelArtifact:
      description: An ML model artifact.
      type: object
      required:
        - artifactType
      allOf:
        - $ref: "#/components/schemas/BaseArtifact"
        - $ref: "#/components/schemas/ModelArtifactCreate"
        - type: object
          properties:
            artifactType:
              type: string
              default: "model-artifact"

And the proposed one (here the actual final result is the same but the warning disappears)

    ModelArtifact:
      description: An ML model artifact.
      type: object
      required:
        - artifactType
      properties:
        artifactType:
          type: string
          default: "model-artifact"
      allOf:
        - $ref: "#/components/schemas/BaseArtifact"
        - $ref: "#/components/schemas/ModelArtifactCreate"

To make the change I am proposing clearer I created a PR against this PR: isinyaaa#1

So I think it is up to us how to proceed here, if the WARN on server side is acceptable I think we can move ahead with this PR as it is.. otherwise I think the proposed change should solve the issue without changing the final code generation.

@isinyaaa can you check if this change works well with Kiota as well?
[edit: I tried using Kiota and there is no error like Schema Artifact must contain property specified in the discriminator artifactType in the required field list so I think that even with this change the issue should be fixed on kiota side]

wdyt @isinyaaa @tarilabs @dhirajsb ?

[tarilabs]

thank you for following up on this OpenAPI topic, if the "proposing clearer" PR is acceptable sounds to me the best setup consolidating contributions from all

[isinyaaa]

that's interesting, thanks for the investigation! updated now.

[isinyaaa]

@isinyaaa I noticed this PR is moving the discriminator to derived types. How does that help, or what is the difference?

IIUC, having the discriminator field defined on Artifact doesn't imply that it's present on the composing types, as there's no such thing as "required properties" that have been inherited. Thus, artifactType should be required on all the composing classes.

Just updated the broken link on the description which I think also highlights the point.

[lampajr]

Thanks again @isinyaaa , lgtm!

[tarilabs]

/lgtm

Thank you so much, this is great complementary work to make our OpenAPI contract more robust to be consumed by clients

[tarilabs]

/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: isinyaaa, lampajr, tarilabs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [tarilabs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment