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

fix: narrow generated API types #5735

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

briangregoryholmes
Copy link
Contributor

@briangregoryholmes briangregoryholmes commented Sep 19, 2024

The lack of required properties in the Swagger yaml results in the generated types to have unnecessary optional properties, which results in avoidable conditional checks on the applications side. This PR fixes that issue on the "supply side" for a few of the most impactful properties, but does not remove the conditional checks.

@briangregoryholmes briangregoryholmes marked this pull request as draft September 19, 2024 04:06
@briangregoryholmes briangregoryholmes marked this pull request as ready for review September 20, 2024 01:50
Copy link
Contributor

@begelundmuller begelundmuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I think these annotations will just be overridden next time we run make proto.generate. For the change to persist, I think it would require a config change in proto/buf.gen.openapi-runtime.yaml or one of its adjacent files, but I'm not sure if it's possible.

@briangregoryholmes
Copy link
Contributor Author

Looks like there are annotations we can add to the proto files as seen here: (https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_openapi_output/). Updated the PR.

@briangregoryholmes
Copy link
Contributor Author

briangregoryholmes commented Sep 20, 2024

One small issue I'm running into:

Is it possible to change the type of the dimension_value field in MetricsViewComparisonRow to a string instead of the broader google.protobuf.Value? Changing it resulted in some type errors via the checks, but I'm not sure how or why this would be returned as anything other than a string. Want to hold off on fixing the errors if there's a reason I'm missing for using the other type.

Relatedly, this MetricsViewComparisonRow type (and related query) is being used to populate the list of possible dimension values when adding a filter, though it doesn't seem to be designed for that considering it's also pulling in measure data. Is there a better way to just get a plain list of these values (max 100) filtered by a search parameter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants