Skip to content

Ability to create a snake_case field with the graphql_object! macro #201

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

Closed
sylvain101010 opened this issue Jun 22, 2018 · 8 comments
Closed
Labels
easy enhancement Improvement of existing features or bugfix

Comments

@sylvain101010
Copy link

sylvain101010 commented Jun 22, 2018

Hi, as I wasn't able to find any way to do this I open this issue:

a way to create snake_case fields should be provided.

Now we can create an object

graphql_object!(Human: () |&self| {
    field id() -> Option<&str> {
        self.id.as_str()
    }

    field appearsIN() -> Vec<Episode> {
        vec![Episode::NewHope, Episode::Jedi]
    }
});

which results in

query {
human(id: "1234") {
    id
    appearsIN
  }
}

but we can't create the following object due to the automatic field transformation

query {
human(id: "1234") {
    id
    appears_in
  }
}

indeed

graphql_object!(Human: () |&self| {
    field id() -> Option<&str> {
        self.id.as_str()
    }

    field appears_in() -> Vec<Episode> {
        vec![Episode::NewHope, Episode::Jedi]
    }
});

will results in

query {
human(id: "1234") {
    id
    appearsIn
  }
}
@sylvain101010 sylvain101010 changed the title Ability to create a snek_case field Ability to create a snake_case field Jun 22, 2018
@sylvain101010 sylvain101010 changed the title Ability to create a snake_case field Ability to create a snake_case field with the graphql_object! macro Jun 22, 2018
@theduke
Copy link
Member

theduke commented Jun 26, 2018

Yup we should probably allow snake case fields even though they are against the convention.

@theduke theduke added enhancement Improvement of existing features or bugfix easy labels Jun 26, 2018
@sylvain101010
Copy link
Author

the source of the problem come from constructions like this

$reg.field_convert::<$t, _, Self::Context>(
                &$crate::to_camel_case(stringify!($name)), $info)
                .deprecated($reason),

https://github.com/graphql-rust/juniper/blob/master/juniper/src/macros/object.rs#L280

but being not fluent enough in rust, I don't know how to modify the macro to allow snake_case call instead of to_camel_case

@theduke
Copy link
Member

theduke commented Jun 26, 2018

We will need to add some kind of attribute that disables to to_camel_case conversion.

Maybe

#[juniper(keep_field_name)]
field appears_in() -> Vec<Episode> {
        vec![Episode::NewHope, Episode::Jedi]
    }

@kestred
Copy link
Contributor

kestred commented Jun 27, 2018

I disagree that the current behavior should be changed.

Juniper should keep the current behavior (names are coerced into camel case).

Many new users create fields using snake_case intuitively, which is part of the current benefits of Juniper that your rust code looks more or less like rust, using rust conventions (instead of diverging completely into a custom DSL), and your GraphQL schema still uses GraphQL conventions.

While field names are always coerced to strings, and can be handled arbitrarily because of that, other parts of the graphql_xyz! macros are hard coded and cannot be changed according to preference.

Example:

graphql_interface!(<'a> &'a Character: () as "Character" |&self| {
    field myCamelCaseField() -> &str { self.id() }
    field myOtherCamelCaseField() -> &str { self.id() }

    instance_resolvers: |_| { // <--- not a field
        &Human => self.as_human(),
        &Droid => self.as_droid(),
    }
});

As an additional note, the documentation currently uses snake_case in its examples, so I would expect more existing users to write with juniper that way and expect the current behavior (See http://juniper.graphql.rs/types/input_objects.html).

To get the behavior you want, where schema names have non-standard GraphQL names, I'd instead recommend one of the patterns like:

/// Option Foo
#[juniper(rename = "Appears_In")]
field appears_in() -> Vec<Episode> {
        vec![Episode::NewHope, Episode::Jedi]
    }
}

/// Option Bar
field appears_in() rename "Appears_In" -> Vec<Episode> {
        vec![Episode::NewHope, Episode::Jedi]
    }
}

The first option has the benefit of being similar to serde's macros and so should be easier to learn and more familiar for the majority of rustaceans.


Post Script 1. - If anything I'd strongly recommend removing support for non-snakecase field identifiers in the macros (or adding a lint, and have them warn by default); especially prior to v1.0, allowing juniper to make larger changes to macro generated code in the future without affecting stability.

@theduke
Copy link
Member

theduke commented Jun 28, 2018

@kestred no one suggested changing the default behaviour.

All this is about is a option to disable the automatic camel case conversion if you really need it, which might be neccessary in some cases.

@andy128k
Copy link
Contributor

@z0mbie42 Why not to query like this

query {
  human(id: "1234") {
    id
    appears_in: appearsIn
  }
}

and not introduce extra complexity in this crate?

@theduke
Copy link
Member

theduke commented May 7, 2019

I think the current behaviour is fine and this is a pretty edge use case.

renaming fields is a relatively small manual step if you really need it.

@theduke theduke closed this as completed May 7, 2019
@2p4b
Copy link

2p4b commented Aug 16, 2019

@theduke the camelCase naming is a preference NOT a specification or even a convention. Technically __typename is snake case... In fact the the spec in supports the use of snake case when you read names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy enhancement Improvement of existing features or bugfix
Projects
None yet
Development

No branches or pull requests

5 participants