Thrift bool parameter gets into unknown volo_genbool type, methods started with '_' losts underscore symbol #515

kubroid · 2024-11-01T09:10:13Z

Bug Report

Encountered compilation errors when trying to compile a simple Thrift IDL using the volo crate. The errors include unknown types and incorrect handling of underscored method names, which is critical for backward compatibility with existing service definitions.

Version

cargo tree | grep -E "volo|pilota"
volo_test v0.1.0 (/home/alex/git/volo-test)
├── pilota v0.11.7
├── volo v0.10.3
├── volo-gen v0.1.0 (/home/alex/git/volo-test/volo-gen)
│   ├── pilota v0.11.7 (*)
│   ├── volo v0.10.3 (*)
│   └── volo-thrift v0.10.4
│       ├── pilota v0.11.7 (*)
│       └── volo v0.10.3 (*)
│   └── volo-build v0.10.15
│       ├── pilota-build v0.11.22
│       │   ├── pilota-thrift-parser v0.11.2
│       ├── volo v0.10.3 (*)
└── volo-thrift v0.10.4 (*)

Platform

uname -a
Linux P53 5.15.0-91-generic #101-Ubuntu SMP Tue Nov 14 13:30:08 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Crates

volo, pilota

Description

Steps to Reproduce:

Create a Sample IDL File:

~/git/volo-test> tree
.
└── rts
    └── test.thrift

~/git/volo-test> cat rts/test.thrift
service Test {
    string health(
        1: bool deep_check
    )

    string _underscored(
        1: string param
    )
}

Initialize a Volo Project:

~/git/volo-test> volo init volo-test rts/test.thrift`
cargo:rerun-if-changed=/home/alex/git/volo-test/rts/test.thrift

Resulting directory structure:

~/git/volo-test [master|…6]> tree
.
├── Cargo.lock
├── Cargo.toml
├── rts
│   └── test.thrift
├── src
│   ├── bin
│   │   └── server.rs
│   └── lib.rs
└── volo-gen
    ├── build.rs
    ├── Cargo.toml
    ├── src
    │   └── lib.rs
    └── volo.yml

Review Generated Code:

In src/lib.rs, observe issues such as:

Unknown type volo_genbool.
Incorrect method naming leading to _underscored issues.

~/git/volo-test [master|…5]> cat src/lib.rs
pub struct S;

impl volo_gen::test::Test for S {
    async fn health(
        &self,
        _deep_check: volo_genbool,  // <<< this is unknown
    ) -> ::core::result::Result<::pilota::FastStr, ::volo_thrift::ServerError> {
        ::std::result::Result::Ok(Default::default())
    }
    async fn underscored(  // <<< underscore is missing
        &self,
        _param: volo_gen::pilota::FastStr,
    ) -> ::core::result::Result<::pilota::FastStr, ::volo_thrift::ServerError> {
        ::std::result::Result::Ok(Default::default())
    }
}

Build the Project:

cargo build
...
error[E0422]: cannot find struct, variant or union type `Test_UnderscoredArgsSend` in this scope
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-47336832199418a2/out/volo_gen.rs:1112:57
     |
144  |         pub struct TestUnderscoredArgsSend {
     |         ---------------------------------- similarly named struct `TestUnderscoredArgsSend` defined here
...
1112 |                 let req = TestRequestSend::_Underscored(Test_UnderscoredArgsSend { param });
     |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: `TestUnderscoredArgsSend`

error[E0422]: cannot find struct, variant or union type `Test_UnderscoredArgsSend` in this scope
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-47336832199418a2/out/volo_gen.rs:1169:57
     |
144  |         pub struct TestUnderscoredArgsSend {
     |         ---------------------------------- similarly named struct `TestUnderscoredArgsSend` defined here
...
1169 |                 let req = TestRequestSend::_Underscored(Test_UnderscoredArgsSend { param });
     |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: `TestUnderscoredArgsSend`

error[E0412]: cannot find type `Test_UnderscoredArgsRecv` in this scope
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-47336832199418a2/out/volo_gen.rs:1280:26
     |
447  |         pub struct TestUnderscoredArgsRecv {
     |         ---------------------------------- similarly named struct `TestUnderscoredArgsRecv` defined here
...
1280 |             _Underscored(Test_UnderscoredArgsRecv),
     |                          ^^^^^^^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: `TestUnderscoredArgsRecv`

error[E0412]: cannot find type `Test_UnderscoredArgsSend` in this scope
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-47336832199418a2/out/volo_gen.rs:1286:26
     |
144  |         pub struct TestUnderscoredArgsSend {
     |         ---------------------------------- similarly named struct `TestUnderscoredArgsSend` defined here
...
1286 |             _Underscored(Test_UnderscoredArgsSend),
     |                          ^^^^^^^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: `TestUnderscoredArgsSend`

Decoder part from out/volo_gen.rs still has an underscore in name:

            fn decode<T: ::pilota::thrift::TInputProtocol>(
                __protocol: &mut T,
                msg_ident: &::pilota::thrift::TMessageIdentifier,
            ) -> ::core::result::Result<Self, ::pilota::thrift::ThriftException> {
                ::std::result::Result::Ok(match &*msg_ident.name {
                    "health" => Self::Health(::pilota::thrift::Message::decode(__protocol)?),

                    "_underscored" => { // <<< underscore is still here!

                        Self::_Underscored(::pilota::thrift::Message::decode(__protocol)?)
                    }
                    _ => {
                        return ::std::result::Result::Err(
                            ::pilota::thrift::new_application_exception(
                                ::pilota::thrift::ApplicationExceptionKind::UNKNOWN_METHOD,
                                format!("unknown method {}", msg_ident.name),
                            ),
                        );
                    }
                })
            }

After removing _ from IDL method definition we can see another error

~/git/volo-test [master|…6]> cargo build
   Compiling volo-gen v0.1.0 (/home/alex/git/volo-test/volo-gen)
   Compiling volo_test v0.1.0 (/home/alex/git/volo-test)
error[E0433]: failed to resolve: could not find `pilota` in `volo_gen`
  --> src/lib.rs:12:27
   |
12 |         _param: volo_gen::pilota::FastStr,
   |                           ^^^^^^ could not find `pilota` in `volo_gen`

error[E0412]: cannot find type `volo_genbool` in this scope
 --> src/lib.rs:6:22
  |
6 |         _deep_check: volo_genbool,
  |                      ^^^^^^^^^^^^ not found in this scope

The ability to correctly parse and handle IDLs with underscored method names is crucial for maintaining compatibility with existing production services. Adjustments to Thrift IDLs for naming conventions without underscores are not feasible due to service dependencies.

If further context or any specific file changes are required, please let me know so I can assist with providing them.

The text was updated successfully, but these errors were encountered:

PureWhiteWu · 2024-11-01T15:04:40Z

PTAL @Ggiggle @Millione

kubroid · 2024-11-05T09:13:08Z

Guys, you are the best!! Thanks for such fast fix.

kubroid · 2024-11-05T09:17:24Z

There is one more issue i found - pilota-build crashes in case meet semicolon after typedef:

typedef string ScoringMetadataLabel;

service Test {
    string health(
        1: bool deep_check;
    );
    string _underscored(
        1: string param;
    );
}

leads to

  thread 'main' panicked at /home/alex/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pilota-build-0.11.23/src/parser/thrift/mod.rs:34:53:
  called `Result::unwrap()` on an `Err` value: Error(Error { input: ";\n\nservice Test {\n    string health(\n        1: bool deep_check;\n    );\n    string _underscored(\n        1: string param;\n    );\n}\n", code: Satisfy })

And after service definition:

service Test {
  string health(
      1: bool deep_check;
  );
  string _underscored(
      1: string param;
  );
};

  thread 'main' panicked at /home/alex/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pilota-build-0.11.23/src/parser/thrift/mod.rs:34:53:
  called `Result::unwrap()` on an `Err` value: Error(Error { input: ";\n", code: Satisfy })

Ggiggle · 2024-11-05T09:34:14Z

Currently there is indeed a lack of handling of semicolons or commas at the end when parsing typedef or struct, thanks for discovering, we will fix it later. But I only found the typedef example using the semicolon or comma at the end, can you guide where to confirm there are similar syntaxes for other types, such as struct and service.

PureWhiteWu · 2024-11-05T09:39:32Z

Thanks very much for your report!

https://thrift.apache.org/docs/idl

According to the thrift idl spec, semicolon (ListSeparator) is not allowed after typedef and service.

But I think we can also support such usage later since it's more flexible for users and I think it will not lead to confusions.

For now, maybe you can just remove the semicolon and try again?

kubroid · 2024-11-05T09:49:10Z

Thanks! We just has semicolons in that places and thrift and thriftpy2 have no issues to compile it.
Of course i can remove it.

kubroid · 2024-11-05T10:30:01Z

Sorry to bother you again, I tried to compile our company thrift IDLs and now see other issues:

# some subset of definitions
struct TSchemaField {
    1: required string name;
    6: optional map<string, list<string>> selectors;
}

service Test {
    string health(
        1: bool deep_check
    )
    string test_map(
        1: required string name;
        6: optional set<TSchemaField> fields;
    )
}

cargo tree | grep -E "volo|pilota"
volo_test v0.1.0 (/home/alex/git/volo-test)
├── pilota v0.11.7
├── volo v0.10.3
├── volo-gen v0.1.0 (/home/alex/git/volo-test/volo-gen)
│   ├── pilota v0.11.7 (*)
│   ├── volo v0.10.3 (*)
│   └── volo-thrift v0.10.4
│       ├── pilota v0.11.7 (*)
│       └── volo v0.10.3 (*)
│   └── volo-build v0.10.16
│       ├── pilota-build v0.11.23
│       │   ├── pilota-thrift-parser v0.11.3
│       ├── volo v0.10.3 (*)
└── volo-thrift v0.10.4 (*)

    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1077:13
     |
1073 |         #[derive(Debug, Default, Clone, PartialEq)]
     |                                         --------- in this derive macro expansion
...
1077 |             pub fields: ::pilota::AHashSet<TSchemaField>,
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: the following type would have to `impl` its required traits for this operation to be valid
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:6:9
     |
6    |         pub struct TSchemaField {
     |         ^^^^^^^^^^^^^^^^^^^^^^^
     |         |
     |         must implement `Hash`
     |         must implement `std::cmp::Eq`
     = note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `TSchemaField` with `#[derive(Eq, Hash, PartialEq)]`
     |
6    +         #[derive(Eq, Hash, PartialEq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: std::cmp::Eq` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1140:45
     |
1140 | ...                   val.insert(::pilota::thrift::Message::decode(__protocol)?);
     |                           ^^^^^^ the trait `std::cmp::Eq` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:8
     |
429  |     T: Eq + Hash,
     |        ^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function
help: consider annotating `TSchemaField` with `#[derive(Eq)]`
     |
6    +         #[derive(Eq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: Hash` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1140:45
     |
1140 | ...                   val.insert(::pilota::thrift::Message::decode(__protocol)?);
     |                           ^^^^^^ the trait `Hash` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:13
     |
429  |     T: Eq + Hash,
     |             ^^^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function

error[E0277]: the trait bound `TSchemaField: std::cmp::Eq` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1223:29
     |
1223 |                         val.insert(<TSchemaField as ::pilota::thrift::Message>::decode_async(__protocol).await?);
     |                             ^^^^^^ the trait `std::cmp::Eq` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:8
     |
429  |     T: Eq + Hash,
     |        ^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function
help: consider annotating `TSchemaField` with `#[derive(Eq)]`
     |
6    +         #[derive(Eq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: Hash` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1223:29
     |
1223 |                         val.insert(<TSchemaField as ::pilota::thrift::Message>::decode_async(__protocol).await?);
     |                             ^^^^^^ the trait `Hash` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:13
     |
429  |     T: Eq + Hash,
     |             ^^^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function

error[E0369]: binary operation `==` cannot be applied to type `AHashSet<TSchemaField>`
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1446:13
     |
1442 |         #[derive(Debug, Default, Clone, PartialEq)]
     |                                         --------- in this derive macro expansion
...
1446 |             pub fields: ::pilota::AHashSet<TSchemaField>,
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: the following type would have to `impl` its required traits for this operation to be valid
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:6:9
     |
6    |         pub struct TSchemaField {
     |         ^^^^^^^^^^^^^^^^^^^^^^^
     |         |
     |         must implement `Hash`
     |         must implement `std::cmp::Eq`
     = note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider annotating `TSchemaField` with `#[derive(Eq, Hash, PartialEq)]`
     |
6    +         #[derive(Eq, Hash, PartialEq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: std::cmp::Eq` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1509:45
     |
1509 | ...                   val.insert(::pilota::thrift::Message::decode(__protocol)?);
     |                           ^^^^^^ the trait `std::cmp::Eq` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:8
     |
429  |     T: Eq + Hash,
     |        ^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function
help: consider annotating `TSchemaField` with `#[derive(Eq)]`
     |
6    +         #[derive(Eq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: Hash` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1509:45
     |
1509 | ...                   val.insert(::pilota::thrift::Message::decode(__protocol)?);
     |                           ^^^^^^ the trait `Hash` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:13
     |
429  |     T: Eq + Hash,
     |             ^^^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function

error[E0277]: the trait bound `TSchemaField: std::cmp::Eq` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1592:29
     |
1592 |                         val.insert(<TSchemaField as ::pilota::thrift::Message>::decode_async(__protocol).await?);
     |                             ^^^^^^ the trait `std::cmp::Eq` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:8
     |
429  |     T: Eq + Hash,
     |        ^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function
help: consider annotating `TSchemaField` with `#[derive(Eq)]`
     |
6    +         #[derive(Eq)]
7    |         pub struct TSchemaField {
     |

error[E0277]: the trait bound `TSchemaField: Hash` is not satisfied
    --> /home/alex/git/volo-test/target/debug/build/volo-gen-470dfe4d935de4fb/out/volo_gen.rs:1592:29
     |
1592 |                         val.insert(<TSchemaField as ::pilota::thrift::Message>::decode_async(__protocol).await?);
     |                             ^^^^^^ the trait `Hash` is not implemented for `TSchemaField`
     |
note: required by a bound in `HashSet::<T, S>::insert`
    --> /home/alex/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/collections/hash/set.rs:429:13
     |
429  |     T: Eq + Hash,
     |             ^^^^ required by this bound in `HashSet::<T, S>::insert`
...
889  |     pub fn insert(&mut self, value: T) -> bool {
     |            ------ required by a bound in this associated function

Commenting 6: optional map<string, list<string>> selectors; line "solves" the issue.

error[E0433]: failed to resolve: could not find `pilota` in `volo_gen`
  --> src/lib.rs:12:26
   |
12 |         _name: volo_gen::pilota::FastStr,
   |                          ^^^^^^ could not find `pilota` in `volo_gen`

error[E0433]: failed to resolve: could not find `pilota` in `volo_gen`
  --> src/lib.rs:13:28
   |
13 |         _fields: volo_gen::pilota::AHashSet<::test::TSchemaField>,
   |                            ^^^^^^ could not find `pilota` in `volo_gen`

error[E0412]: cannot find type `volo_genbool` in this scope
 --> src/lib.rs:6:22
  |
6 |         _deep_check: volo_genbool,
  |                      ^^^^^^^^^^^^ not found in this scope

error[E0433]: failed to resolve: could not find `test` in the list of imported crates
  --> src/lib.rs:13:47
   |
13 |         _fields: volo_gen::pilota::AHashSet<::test::TSchemaField>,
   |                                               ^^^^ could not find `test` in the list of imported crates
   |
help: consider importing this module
   |
1  + use volo_gen::test;
   |
help: if you import `test`, refer to it directly
   |
13 -         _fields: volo_gen::pilota::AHashSet<::test::TSchemaField>,
13 +         _fields: volo_gen::pilota::AHashSet<test::TSchemaField>,
   |

PureWhiteWu · 2024-11-05T10:46:50Z

Thanks very much for your report, we will investigate this asap.

kubroid · 2024-11-05T17:31:23Z

Currently there is indeed a lack of handling of semicolons or commas at the end when parsing typedef or struct, thanks for discovering, we will fix it later. But I only found the typedef example using the semicolon or comma at the end, can you guide where to confirm there are similar syntaxes for other types, such as struct and service.

i have a feeling that semicolon could be almost on each line, especially if author is c++ guy :)
Found it after typedefs, struct/unions fields, methods with and without throws:

    TReindexStatus get_reindex_status(
        1: common.SiteId site_id,
    ) throws (1: common.TServiceException e);

never after struct definitions (closing '}')

PureWhiteWu · 2024-11-06T02:55:05Z

The semicolon(ListSeparator) has been supported in cloudwego/pilota#284.
We are now investigating the new issue.

PureWhiteWu · 2024-11-06T03:12:24Z

Hi, sorry for the late reply, I found that the issue is because there's a map<string, list<string>> that is not comparable in TSchemaField , which is used in the param set<TSchemaField> that requires the inner element comparable.

Can this idl be compiled successfully in other languages such as go/cpp?

kubroid · 2024-11-06T06:48:06Z

Hi, sorry for the late reply, I found that the issue is because there's a map<string, list<string>> that is not comparable in TSchemaField , which is used in the param set<TSchemaField> that requires the inner element comparable.

Can this idl be compiled successfully in other languages such as go/cpp?

yes, thrift c++ and rs compile it well. also thift async is good.

PureWhiteWu · 2024-11-06T07:37:03Z

let me try using thrift rs

kubroid · 2024-11-06T08:32:04Z

It could be even worse, like:

    7: optional map<string, list< map<string, list<string>>>> transforms;

PureWhiteWu · 2024-11-06T09:16:53Z

I've found that, in thrift-rs, they use BTreeMap and BTreeSet which implements PartialEq, Eq, PartialOrd, Ord that makes it able to be used in set/map.
But in fact, the implementation is very inefficient, as it will iterator all the elements to calc the hash and compare the value.

For now, pilota doesn't support using BTreeMap and BTreeSet for map and set, we uses AHashMap/AHashSet for much better performance.

Supporting BTreeMap and BTreeSet in those cases are not impossible in pilota, but may take some time.

Could you modify your idl to avoid this limitation for now?

kubroid · 2024-11-06T10:54:09Z

Thanks for your investigation.
Unfortunately this is a part of production IDL currently in use by multiple services, so i cant change it.
I'll try to remove it for my test app just to see if it fly.

kubroid · 2024-11-06T11:35:47Z

Still there are few erros after parsing:

_deep_check: volo_genbool,
_exclude: volo_gen::pilota::AHashSet<::resources::Id>, instead of _exclude: ::pilota::AHashSet<volo_gen::resources::Id>,
_ids: volo_gen::std::vec::Vec<::resources::Id>, instead of _ids: ::std::vec::Vec<volo_gen::resources::Id>,

  async fn get_candidates(
      &self,
      ...
      ) -> ::core::result::Result<
      ::volo_thrift::MaybeException<
          volo_gen::candidates::TCandidateList,
          CandidatesGetCandidatesException, <<< volo_gen::candidates:: is missing
      >,
      ::volo_thrift::ServerError,
  > {
      ::std::result::Result::Ok(Default::default())
  }```

Default is not implemented for MaybeException:

error[E0277]: the trait bound `MaybeException<Vec<TJobInfo>, CandidatesJobInfoException>: Default` is not satisfied
   --> src/lib.rs:298:35
    |
298 |         ::std::result::Result::Ok(Default::default())
    |                                   ^^^^^^^^^^^^^^^^^^ the trait `Default` is not implemented for `MaybeException<Vec<TJobInfo>, CandidatesJobInfoException>`

I'll prepare a test IDLs for you.

PureWhiteWu · 2024-11-06T11:59:03Z

Hi, thanks very much for your quick response! We will investigate those problems asap.
Are you planning to use volo in your production environment?

If so, we can try to support your usage faster.

kubroid · 2024-11-06T12:50:39Z

It's still a bit early to discuss production plans. Currently, I'm working on migrating one of our Python projects to Rust. At this stage, I’m gathering information on libraries, frameworks, usability, support, and other relevant factors.

I have one implementation in place that uses an async Thrift library, and so far, it’s showing promising results. However, when I combine it with another library (async Aerospike), the Aerospike library’s performance is unexpectedly poor—even slower than Python! I haven't yet identified the root cause of this performance issue.

Since I’m not yet an expert in Rust, there’s a chance I may have overlooked something in my code here. To address this, I plan to test Volo, as it’s written purely in Rust, uses more advanced techniques, and in preliminary tests, it appears to offer better performance and lower CPU usage.

PureWhiteWu · 2024-11-06T13:47:40Z

Hi, the two issues you mentioned before should be fixed now, could you please cargo update and try again?

PureWhiteWu · 2024-11-06T13:48:48Z

For the BTreeMap and BTreeSet issue, do you have time to support this? @Millione
I think maybe we can support users to switch to these two types by pilota annotation?

Millione · 2024-11-06T14:00:31Z

@PureWhiteWu Yhea, I will implement it as soon as possible. Maybe just introduce a feature to get this done?

kubroid · 2024-11-06T14:36:58Z

cat rts/test.thrift
service Test {
    string health(
        1: bool deep_check
    )

    string _underscored(
        1: string param
    )
}

volo init volo-test rts/test.thrift
cargo:rerun-if-changed=/home/alex/git/volo-test/rts/test.thrift

cargo update
    Updating crates.io index
     Locking 0 packages to latest compatible versions
note: pass `--verbose` to see 36 unchanged dependencies behind latest

cargo tree | grep -E "volo|pilota"
test v0.1.0 (/home/alex/git/volo-test)
├── pilota v0.11.7
├── volo v0.10.3
├── volo-gen v0.1.0 (/home/alex/git/volo-test/volo-gen)
│   ├── pilota v0.11.7 (*)
│   ├── volo v0.10.3 (*)
│   └── volo-thrift v0.10.5
│       ├── pilota v0.11.7 (*)
│       └── volo v0.10.3 (*)
│   └── volo-build v0.10.17
│       ├── pilota-build v0.11.25
│       │   ├── pilota-thrift-parser v0.11.5
│       ├── volo v0.10.3 (*)
└── volo-thrift v0.10.5 (*)

cargo --version
cargo 1.81.0 (2dbb1af80 2024-08-20)
rustc --version
rustc 1.81.0 (eeb90cda1 2024-09-04)

cargo build
...
error[E0407]: method `underscored` is not a member of trait `volo_gen::test::Test`
  --> src/lib.rs:10:5
   |
10 |       async fn underscored(
   |       ^        ----------- help: there is an associated function with a similar name: `_underscored`
   |  _____|
   | |
11 | |         &self,
12 | |         _param: volo_gen::pilota::FastStr,
13 | |     ) -> ::core::result::Result<::pilota::FastStr, ::volo_thrift::ServerError> {
14 | |         ::std::result::Result::Ok(Default::default())
15 | |     }
   | |_____^ not a member of trait `volo_gen::test::Test`

error[E0433]: failed to resolve: could not find `pilota` in `volo_gen`
  --> src/lib.rs:12:27
   |
12 |         _param: volo_gen::pilota::FastStr,
   |                           ^^^^^^ could not find `pilota` in `volo_gen`

error[E0412]: cannot find type `volo_genbool` in this scope
 --> src/lib.rs:6:22
  |
6 |         _deep_check: volo_genbool,
  |                      ^^^^^^^^^^^^ not found in this scope

error[E0046]: not all trait items implemented, missing: `_underscored`
 --> src/lib.rs:3:1
  |
3 | impl volo_gen::test::Test for S {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `_underscored` in implementation
  |
  = help: implement the missing item: `fn _underscored(&self, _: FastStr) -> impl Future<Output = Result<FastStr, ServerError>> + Send { todo!() }`

PureWhiteWu · 2024-11-07T02:51:45Z

@PureWhiteWu Yhea, I will implement it as soon as possible. Maybe just introduce a feature to get this done?

I don't think a feature is enough, because the codec part in pilota also needs to add extra methods such as write_btree_set

PureWhiteWu · 2024-11-07T03:15:08Z

Oh you need to re-install your volo-cli @kubroid

kubroid · 2024-11-07T08:14:39Z

Oh you need to re-install your volo-cli @kubroid

Sorry, after update all looks good!!
The only thing missing is a volo_gen namespace for custom(?) exceptions

common.rs
exception TServiceException {
...
}

include "common.thrift"
service Candidates {
...
  list<common.SiteId> cleanup_indices() throws (1: common.TServiceException e)
}

    async fn cleanup_indices(
        &self,
    ) -> ::core::result::Result<
        ::volo_thrift::MaybeException<
            ::std::vec::Vec<volo_gen::common::SiteId>,
            CandidatesCleanupIndicesException,     <<<<<<<<<<<<
        >,
        ::volo_thrift::ServerError,
    > {
        ::std::result::Result::Ok(Default::default())
    }

PureWhiteWu · 2024-11-07T08:31:20Z

@kubroid Yep, this is also a bug of the generated template, could you manually import the Exception type in your code for now? We will try to fix this later.

kubroid · 2024-11-07T08:34:40Z

Sure, i've already did.
Thanks for such an agile problem fixing!!!

PureWhiteWu · 2024-11-07T08:47:15Z

Are you able to run your application without any problems now?

kubroid · 2024-11-07T10:03:32Z

Yes, its runs now. I need to implement all internal things.
Thanks you!!!

I have one, maybe silly question related to thrift client.

At this moment i have something like this for tests:

pub struct S {
    pub client: volo_gen::candidates::CandidatesClient,
}
...
    volo_gen::candidates::CandidatesServer::new(S {
        client: {
            let addr = "127.0.0.1:7070".parse::<std::net::SocketAddr>().unwrap();

            volo_gen::candidates::CandidatesClientBuilder::new("service_name")
                .make_codec(volo_thrift::codec::DefaultMakeCodec::framed())
                .address(addr)
                .build()
        },
    })
    .run(addr)
    .await
    .unwrap();

but in real life 127.0.0.1:7070 value will come from a config as some.dns.name:port, so i need to resolve it, which gives me a list of endpoints for load sharing.

Make client for each call seems expensive.

What you think is a best way to handle this?

Sorry, this is completely out of the scope of current issue.

PureWhiteWu · 2024-11-07T12:54:33Z

Hi, for your use case, you can implement a discover according to this doc: https://www.cloudwego.io/docs/volo/guide/discovery_lb/

Do we have a dns resolver out of box now? @Millione

Feel free to raise any questions.

kubroid · 2024-11-07T14:42:39Z

@PureWhiteWu thank you for your previous assistance! I have another request regarding the following Thrift definition:

    void filters_changed(
        1: common.SiteId site_id,
        2: optional list<string> filter_names,
       99: optional common.TraceParent traceparent,
    )

produces

    async fn filters_changed(
        &self,
        _site_id: volo_gen::common::SiteId,
        _filter_names: ::std::vec::Vec<::pilota::FastStr>,
        _traceparent: volo_gen::common::TraceParent,
    ) -> ::core::result::Result<(), ::volo_thrift::ServerError> {
        ::std::result::Result::Ok(Default::default())
    }

In this case, traceparent is treated as a non-optional value. However, we have logic that heavily relies on the presence, absence, or specific value of this parameter.

Unfortunately, due to the behavior of Python (specifically ThriftPy2), required or default fields can be omitted in the protocol if they are None. This leads to some complications in our call logic, as traceparent can be None, an empty string (""), or contain specific values (e.g., "some value").

Is it possible to implement support for optional method parameters? Perhaps this could be controlled with an additional flag in the generator to avoid interfering with the standard implementation.

Thank you once again for your excellent support!

PureWhiteWu · 2024-11-07T17:07:31Z

Thanks for your feedback, you are right I think changing this to use Option is more suitable.

We will implement this asap.

Millione · 2024-11-08T06:30:13Z

I don't think a feature is enough, because the codec part in pilota also needs to add extra methods such as write_btree_set

Get it.

Do we have a dns resolver out of box now? @Millione

Currently I think the answer is no, using https://docs.rs/trust-dns-resolver/latest/trust_dns_resolver/ maybe helpful, and @AH-dark has mentioned that he would like to help providing something like Consul、Etcd in the future.

kubroid · 2024-11-11T17:03:37Z

Thanks @PureWhiteWu @Millione
Discover + trust_dns_resolver works fine!

PureWhiteWu · 2024-11-19T03:04:32Z

Reopened since there's still an issue that needs to be solved.

kubroid · 2024-11-19T08:28:13Z

@PureWhiteWu if you relay on this, maybe its better to create a separate issue for better tracking?

Millione · 2024-11-22T07:07:14Z

It could be even worse, like:

    7: optional map<string, list< map<string, list<string>>>> transforms;

@kubroid Hi, this get supported now by annotations in IDL, you can refer to cloudwego/pilota#289 to see how it works.

PureWhiteWu assigned Millione and Ggiggle Nov 1, 2024

This was referenced Nov 5, 2024

fix(pilota-build): keep the leading underscore in idl method name cloudwego/pilota#282

Merged

fix(volo-build): not add vologen prefix for method arguments type with rust type kind #520

Merged

Ggiggle mentioned this issue Nov 5, 2024

fix(pilota-thrift-parser): compatible with separators after syntax item blocks in idl cloudwego/pilota#284

Merged

PureWhiteWu added a commit that referenced this issue Nov 6, 2024

fix(#515): codegen and default problem

4f921c2

PureWhiteWu mentioned this issue Nov 6, 2024

fix(#515): codegen and default problem #525

Merged

PureWhiteWu added a commit that referenced this issue Nov 6, 2024

fix(#515): codegen and default problem (#525)

5d85c3d

PureWhiteWu closed this as completed in #525 Nov 6, 2024

PureWhiteWu reopened this Nov 6, 2024

Millione closed this as completed Nov 18, 2024

PureWhiteWu reopened this Nov 19, 2024

Thrift bool parameter gets into unknown volo_genbool type, methods started with '_' losts underscore symbol #515

Thrift bool parameter gets into unknown volo_genbool type, methods started with '_' losts underscore symbol #515

Comments

kubroid commented Nov 1, 2024

Bug Report

Version

Platform

Crates

Description

Steps to Reproduce:

Create a Sample IDL File:

Initialize a Volo Project:

Resulting directory structure:

Review Generated Code:

Build the Project:

PureWhiteWu commented Nov 1, 2024

kubroid commented Nov 5, 2024

kubroid commented Nov 5, 2024

Ggiggle commented Nov 5, 2024

PureWhiteWu commented Nov 5, 2024

kubroid commented Nov 5, 2024

kubroid commented Nov 5, 2024

PureWhiteWu commented Nov 5, 2024

kubroid commented Nov 5, 2024

PureWhiteWu commented Nov 6, 2024

PureWhiteWu commented Nov 6, 2024

kubroid commented Nov 6, 2024

PureWhiteWu commented Nov 6, 2024

kubroid commented Nov 6, 2024

PureWhiteWu commented Nov 6, 2024

kubroid commented Nov 6, 2024

kubroid commented Nov 6, 2024

PureWhiteWu commented Nov 6, 2024

kubroid commented Nov 6, 2024

PureWhiteWu commented Nov 6, 2024

PureWhiteWu commented Nov 6, 2024

Millione commented Nov 6, 2024

kubroid commented Nov 6, 2024

PureWhiteWu commented Nov 7, 2024

PureWhiteWu commented Nov 7, 2024

kubroid commented Nov 7, 2024

PureWhiteWu commented Nov 7, 2024

kubroid commented Nov 7, 2024

PureWhiteWu commented Nov 7, 2024

kubroid commented Nov 7, 2024

PureWhiteWu commented Nov 7, 2024

kubroid commented Nov 7, 2024

PureWhiteWu commented Nov 7, 2024

Millione commented Nov 8, 2024 • edited Loading

kubroid commented Nov 11, 2024

PureWhiteWu commented Nov 19, 2024

kubroid commented Nov 19, 2024

Millione commented Nov 22, 2024

Millione commented Nov 8, 2024 •

edited

Loading