octokit / source-generator

6 stars 3 forks source link

Hack: Kiota doesn't clean up models #22

Closed kfcampbell closed 7 months ago

kfcampbell commented 8 months ago

This code:

https://github.com/octokit/source-generator/blob/d1c29eea0c06262a42cc119b3ab0304f5e8e67ef/post-processors/go/main.go#L153-L170

is in place to remove models that Kiota partially optimizes away but not fully. Observe this log snippet from generation:

info: Kiota.Builder.KiotaBuilder[0]
      Removing unused model codeScanningAlertSeverity as it is not referenced by the client API surface
info: Kiota.Builder.KiotaBuilder[0]
      Removing unused model contentDirectory as it is not referenced by the client API surface
info: Kiota.Builder.KiotaBuilder[0]
      Removing unused model codeScanningAlertStateQuery as it is not referenced by the client API surface

However, when removeModelsKiotaDoesNotCleanUp is removed from generation, building the generated SDK errors with the following:

+ go build ./...
# github.com/octokit/kiota/orgs
orgs/item_code_scanning_alerts_request_builder.go:33:108: undefined: i158396662f32fe591e8faa247af18558546841dba91f24f5c824e11e34188830.CodeScanningAlertSeverity
orgs/item_code_scanning_alerts_request_builder.go:43:107: undefined: i158396662f32fe591e8faa247af18558546841dba91f24f5c824e11e34188830.CodeScanningAlertStateQuery
# github.com/octokit/kiota/repos
repos/item_item_code_scanning_alerts_request_builder.go:32:108: undefined: i158396662f32fe591e8faa247af18558546841dba91f24f5c824e11e34188830.CodeScanningAlertSeverity
repos/item_item_code_scanning_alerts_request_builder.go:42:107: undefined: i158396662f32fe591e8faa247af18558546841dba91f24f5c824e11e34188830.CodeScanningAlertStateQuery

We should take this issue to the Kiota team and see if they can help pinpoint what's going wrong.

baywet commented 8 months ago

Can you provide more context on why you're trying to remove additional models at this point? The trimming logic is a bit complex, but if you model things as a graph where nodes are types and edges are relationships between those types (property of type, inheritance, etc...). The trimming logic will remove (not project in fact) any type that's not connected at all to the graph, as well as any type that's 2 levels of inheritance or more from a referenced type. (this is done to support generic discriminated references).

kfcampbell commented 8 months ago

Can you provide more context on why you're trying to remove additional models at this point?

The generated Go code does not compile otherwise. That is, I think Kiota is not fully optimizing away the unused models, leaving them in a state of partial generation that breaks compilation: undefined: i158396662f32fe591e8faa247af18558546841dba91f24f5c824e11e34188830.CodeScanningAlertSeverity and Removing unused model codeScanningAlertSeverity as it is not referenced by the client API surface appear to be correlated.

baywet commented 8 months ago

Are you saying there's a reference in the remaining code that point to a type that has been removed?

kfcampbell commented 8 months ago

@baywet that's exactly right! The reference isn't itself referenced anywhere else, so it can be removed as well (which is what we're doing in our dirty hack).

baywet commented 8 months ago

So we probably have two independent clusters in the graph. How did you end up in this situation? Did you generate for the whole API or filter endpoints? If you generated for the whole API, any chance of being able to remove the isolated cluster type definitions from the description when the service emits it for no reason?

kfcampbell commented 7 months ago

We've generated code from the whole API.

any chance of being able to remove the isolated cluster type definitions from the description when the service emits it for no reason?

I'm a bit confused...I'm seeing references to both of those models:

{
            "name": "state",
            "description": "If specified, only code scanning alerts with this state will be returned.",
            "in": "query",
            "required": false,
            "schema": {
              "$ref": "#/components/schemas/code-scanning-alert-state-query"
            }
          },
          {
            "name": "severity",
            "description": "If specified, only code scanning alerts with this severity will be returned.",
            "in": "query",
            "required": false,
            "schema": {
              "$ref": "#/components/schemas/code-scanning-alert-severity"
            }
          }

in the "List code scanning alerts for an organization" and "List code scanning alerts for a repository" endpoints. Should I remove the references to the schema as well as the schema itself, or just the references to the schema and then see if Kiota successfully cleans up the models?

I'm also curious while looking at the schema more why those options aren't present in the generated code as they seem to be valid.

baywet commented 7 months ago

ah, I understand now, these are enums in query parameters. We fixed this issue with kiota 1.8.1 can you try again with 1.8.2 please?

kfcampbell commented 7 months ago

Thanks Vincent! That update does indeed resolve the issue.