graphql-rust / juniper

GraphQL server library for Rust
Other
5.7k stars 423 forks source link

Field name is changed when prefixing variable with underscore. #623

Closed arlyon closed 4 years ago

arlyon commented 4 years ago

Describe the bug

It is a common convention in rust code to prefix variables with an underscore if unused. When creating a graphql_object, I would like to prototype the interface and keep the compiler happy, so I prefix all the vars with an underscore.

struct Query;
#[juniper::graphql_object]
impl Query {
    fn blah() -> bool {
        true
    }
    fn fizz(_buzz: String) -> Option<bool> {
        None
    }
};

This changes the name of the buzz parameter in the schema. Without the underscore, the parameter is called buzz, but with it it is called Buzz. This is due to the way the camelCase is parsed here:

https://github.com/graphql-rust/juniper/blob/a05f4e55c41d4a5a912334a105a73bd33f0ff904/juniper_codegen/src/util/mod.rs#L252-L270

Expected behavior

I would expect that prefixing with an underscore would not change the variable name. Would fixing this cause any breakages?

arlyon commented 4 years ago

Seems from the tests that that is the desired behaviour so I suppose the next step is understanding the motivations. Is this:

https://github.com/graphql-rust/juniper/blob/a05f4e55c41d4a5a912334a105a73bd33f0ff904/juniper_codegen/src/util/mod.rs#L1580

addressing any concrete functionality or is it there for completeness?

LegNeato commented 4 years ago

This is odd behavior, I'm inclined to believe it is not addressing any concrete functionality. dropping a leading _means you can't name your input object starting with _ but I think that's probably ok and it is better to align to Rust's semantics...what do you think?

arlyon commented 4 years ago

I agree and can do a PR (it's really quite a simple fix) but the only issue is that this may break code on the off-chance people are using _x anywhere

LegNeato commented 4 years ago

The next release changes everything (yay async!) so I am fine with that as long as we put it in the changelog.

arlyon commented 4 years ago

I think the best course of action is to just add an additional function (in the same place) to_graphql_identifier which strips a single leading _ and passes it on to to_camel_case, otherwise it may be a little misleading. PR coming soon.

arlyon commented 4 years ago

Closed in #684