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.

Sign up for GitHub

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

Jump to bottom

feat(grpc): generate union types for oneof protobuf #1992

Open
wants to merge 15 commits into
base: test/union-output
Choose a base branch
from
Open

feat(grpc): generate union types for oneof protobuf #1992

wants to merge 15 commits into from
+1,400 −217

Conversation

meskill
Copy link
Contributor

@meskill meskill commented May 20, 2024 •
edited

Summary:
Briefly describe the changes made in this PR.

Issue Reference(s):
Fixes #1912

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests.
  • I have updated the documentation accordingly.
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@github-actions github-actions bot added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label May 20, 2024
@meskill meskill mentioned this pull request May 20, 2024
Open
@tusharmath tusharmath marked this pull request as draft May 22, 2024 07:10
@tusharmath
Copy link
Contributor

Moving to draft until — conflicts & comments are resolved

@meskill meskill changed the base branch from feat/grpc-gen-oneof to main May 22, 2024 09:47
@meskill meskill mentioned this pull request May 22, 2024
Closed
6 tasks
@codecov Codecov
Copy link

codecov bot commented May 23, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.42%. Comparing base (9928b39) to head (be88942).
Report is 25 commits behind head on test/union-output.

❗ Current head be88942 differs from pull request most recent head ecdacc5

Please upload reports for the commit ecdacc5 to get more accurate results.

Additional details and impacted files 
@@                  Coverage Diff                  @@
##           test/union-output    #1992      +/-   ##
=====================================================
+ Coverage              84.25%   84.42%   +0.17%     
=====================================================
  Files                    214      215       +1     
  Lines                  20229    20454     +225     
=====================================================
+ Hits                   17043    17268     +225     
  Misses                  3186     3186

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@meskill meskill marked this pull request as ready for review May 23, 2024 18:44
@meskill meskill marked this pull request as draft May 24, 2024 09:52
@github-actions GitHub Actions
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels May 27, 2024
@github-actions GitHub Actions
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels May 29, 2024
@github-actions GitHub Actions
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 31, 2024
@github-actions github-actions bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jun 4, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 6, 2024

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels Jun 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 8, 2024

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jun 8, 2024
@meskill meskill removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jun 10, 2024
@github-actions github-actions bot added the ci: benchmark Runs benchmarks label Jun 10, 2024
@meskill meskill changed the base branch from main to test/union-output June 10, 2024 15:13
@meskill meskill marked this pull request as ready for review June 10, 2024 15:51
@github-actions GitHub Actions
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels Jun 12, 2024
@github-actions GitHub Actions
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels Jun 14, 2024
tusharmath
tusharmath reviewed Jun 15, 2024
View reviewed changes
src/core/config/config.rs
pub fn to_sdl(&self) -> String {
let doc = self.to_document();
crate::core::document::print(doc)
ConfigModule::from(self.clone()).to_sdl()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we moving this call into ConfigModule?

tusharmath
tusharmath reviewed Jun 15, 2024
View reviewed changes
src/core/config/config_module.rs

/// Normalizes current config with default settings
pub fn normalize_default(self) -> Valid<Self, String> {
self.transform(UnionInputType.pipe(AmbiguousType::default()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can also add remove_unused over here.

tusharmath
tusharmath reviewed Jun 15, 2024
View reviewed changes
tailcall-fixtures/fixtures/configs/yaml-union.yaml
@@ -0,0 +1,35 @@
schema:
Copy link
Contributor

Choose a reason for hiding this comment

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

schema:
  query: Query

types:
  T1:
    fields:
      t1:
        type: String
  T2:
    fields:
      t2:
        type: Int
  T3:
    fields:
      t3:
        type: Boolean
      t33:
        type: Float
        required: true

  V:
    fields:
      v:
        type: U
  Query:
    fields:
      test:
        type: U
        args:
          u:
            type: U
            required: true
          v:
            type: V
            required: true
        http:
          baseURL: http://localhost
          path: /users/{{args.input}}

unions:
  U:
    types: ["T1", "T2", "T3"]

Tried with a nested input type. It doesn't produce correct SDLs

tusharmath
tusharmath reviewed Jun 15, 2024
View reviewed changes
src/core/config/transformer/union_input_type.rs
#[derive(Default)]
pub struct UnionInputType;

impl Transform for UnionInputType {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we split this PR into two — One with just the transformer and one with the the from_proto changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tusharmath tusharmath tusharmath left review comments

Assignees
No one assigned
Labels
ci: benchmark Runs benchmarks type: feature Brand new functionality, features, pages, workflows, endpoints, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(grpc): support for one_of when generating config from protobuf
2 participants
@meskill @tusharmath