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

fix(grpc): remove package from type names #1747

Closed
wants to merge 8 commits into from
Closed

fix(grpc): remove package from type names #1747

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5649345
minor grpc fixes
shashitnak Apr 16, 2024
0bd007b
reverting conversion to snake case during validation
shashitnak Apr 17, 2024
16a6ba7
fixing snapshots
shashitnak Apr 17, 2024
88b6908
fixing package prefix bug
shashitnak Apr 17, 2024
e3a4373
add append and prepend APIs
tusharmath Apr 18, 2024
25ca494
fixing grpc method names
shashitnak Apr 18, 2024
9c57f92
fixing enum tag id
shashitnak Apr 18, 2024
1423084
remove unused API
tusharmath Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/generator/from_proto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,11 @@ impl Context {

ty.variants = Some(variants);

let type_name = GraphQLType::new(enum_name).as_enum().unwrap().to_string();
let type_name = GraphQLType::new(enum_name)
.package(&self.package)
.as_enum()
.unwrap()
.to_string();
Copy link
Contributor

@coderabbitai coderabbitai bot Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Consider handling the potential None case in the chain of method calls for type_name instead of using unwrap(), which could lead to runtime panics if the conversion fails.

- .unwrap()
+ .ok_or_else(|| "Failed to convert to enum".to_string())?
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
let type_name = GraphQLType::new(enum_name)
.package(&self.package)
.as_enum()
.unwrap()
.to_string();
let type_name = GraphQLType::new(enum_name)
.package(&self.package)
.as_enum()
.ok_or_else(|| "Failed to convert to enum".to_string())?
.to_string();

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, the GraphQLType<Unparsed>::parse that is used by as_enum could be rewritten without return Option that could simplify usage of it and remove unwraps

Copy link
Contributor

Choose a reason for hiding this comment

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

@meskill, your suggestion to rewrite the GraphQLType<Unparsed>::parse method used by as_enum to not return an Option is intriguing. Removing the Option could indeed simplify the usage by eliminating the need for unwraps. However, this change would require a careful review of all places where this method is used to ensure that there are no cases where a None return is expected or required.

Would you like to proceed with exploring this change in the current PR, or would you prefer to handle this as a separate task? If it's the latter, I can help by opening a GitHub issue to track this enhancement. Please let me know how you'd like to proceed!

self = self.insert_type(type_name, ty);
}
self
Expand Down Expand Up @@ -185,7 +189,7 @@ impl Context {
body: None,
group_by: vec![],
headers: vec![],
method: field_name.id(),
method: format!("{}.{}.{}", self.package, service_name, field_name.name()),
});

let ty = self
Expand Down
4 changes: 4 additions & 0 deletions src/generator/graphql_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ impl GraphQLType<Parsed> {
self.0.name.clone()
}
}

pub fn name(&self) -> String {
self.0.name.clone()
}
tusharmath marked this conversation as resolved.
Show resolved Hide resolved
}

// FIXME: make it private
Expand Down
21 changes: 13 additions & 8 deletions src/generator/snapshots/tailcall__generator__from_proto__test__from_proto.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ input NEWS_NEWS_ID @tag(id: "news.NewsId") {
id: Int
}

enum NEWS_STATUS @tag(id: "Status") {
tusharmath marked this conversation as resolved.
Show resolved Hide resolved
DRAFT
PUBLISHED
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for messages inside of messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

news.proto already contains message inside of message https://github.com/tailcallhq/tailcall/pull/1747/files#diff-771adfaaae91c7816aaf69af02c57749bfaaaa74e3db399e055104a8a1a6de23R26-R33

is it enough?


type GREETINGS_A_B_HELLO_REPLY @tag(id: "greetings_a.b.HelloReply") {
message: String
}
Expand All @@ -49,12 +54,12 @@ type NEWS_NEWS_LIST @tag(id: "news.NewsList") {
}

type Query {
greetings_a_b_say_hello(hello_request: GREETINGS_A_B_HELLO_REQUEST!): GREETINGS_A_B_HELLO_REPLY! @grpc(method: "greetings_a.b.SayHello")
greetings_b_c_say_hello(hello_request: GREETINGS_B_C_HELLO_REQUEST!): GREETINGS_B_C_HELLO_REPLY! @grpc(method: "greetings_b.c.SayHello")
news_add_news(news: NEWS_NEWS!): NEWS_NEWS! @grpc(method: "news.AddNews")
news_delete_news(news_id: NEWS_NEWS_ID!): NEWS_EMPTY! @grpc(method: "news.DeleteNews")
news_edit_news(news: NEWS_NEWS!): NEWS_NEWS! @grpc(method: "news.EditNews")
news_get_all_news: NEWS_NEWS_LIST! @grpc(method: "news.GetAllNews")
news_get_multiple_news(multiple_news_id: NEWS_MULTIPLE_NEWS_ID!): NEWS_NEWS_LIST! @grpc(method: "news.GetMultipleNews")
news_get_news(news_id: NEWS_NEWS_ID!): NEWS_NEWS! @grpc(method: "news.GetNews")
greetings_a_b_say_hello(hello_request: GREETINGS_A_B_HELLO_REQUEST!): GREETINGS_A_B_HELLO_REPLY! @grpc(method: "greetings_a.b.Greeter.SayHello")
greetings_b_c_say_hello(hello_request: GREETINGS_B_C_HELLO_REQUEST!): GREETINGS_B_C_HELLO_REPLY! @grpc(method: "greetings_b.c.Greeter.SayHello")
news_add_news(news: NEWS_NEWS!): NEWS_NEWS! @grpc(method: "news.NewsService.AddNews")
news_delete_news(news_id: NEWS_NEWS_ID!): NEWS_EMPTY! @grpc(method: "news.NewsService.DeleteNews")
news_edit_news(news: NEWS_NEWS!): NEWS_NEWS! @grpc(method: "news.NewsService.EditNews")
news_get_all_news: NEWS_NEWS_LIST! @grpc(method: "news.NewsService.GetAllNews")
news_get_multiple_news(multiple_news_id: NEWS_MULTIPLE_NEWS_ID!): NEWS_NEWS_LIST! @grpc(method: "news.NewsService.GetMultipleNews")
news_get_news(news_id: NEWS_NEWS_ID!): NEWS_NEWS! @grpc(method: "news.NewsService.GetNews")
}
2 changes: 1 addition & 1 deletion src/generator/snapshots/tailcall__generator__from_proto__test__from_proto_no_pkg_file.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@ type NEWS_LIST @tag(id: "NewsList") {
}

type Query {
get_all_news: NEWS_LIST! @grpc(method: "GetAllNews")
get_all_news: NEWS_LIST! @grpc(method: ".NewsService.GetAllNews")
}
2 changes: 1 addition & 1 deletion src/generator/snapshots/tailcall__generator__from_proto__test__greetings_proto_file.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ type GREETINGS_HELLO_REPLY @tag(id: "greetings.HelloReply") {
}

type Query {
greetings_say_hello(hello_request: GREETINGS_HELLO_REQUEST!): GREETINGS_HELLO_REPLY! @grpc(method: "greetings.SayHello")
greetings_say_hello(hello_request: GREETINGS_HELLO_REQUEST!): GREETINGS_HELLO_REPLY! @grpc(method: "greetings.Greeter.SayHello")
}
2 changes: 1 addition & 1 deletion src/generator/snapshots/tailcall__generator__from_proto__test__required_types.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ type PERSON_PHONE_NUMBER @tag(id: "person.PhoneNumber") {
}

type Query {
person_get_person: PERSON_PERSON! @grpc(method: "person.GetPerson")
person_get_person: PERSON_PERSON! @grpc(method: "person.PersonService.GetPerson")
}
Loading