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.

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

Colliding type names are not handled correctly #1307

Open
axos88 opened this issue Jan 16, 2025 · 3 comments
Open

Colliding type names are not handled correctly #1307

axos88 opened this issue Jan 16, 2025 · 3 comments
Assignees
Labels
k::api Related to API (application interface) k::design Related to overall design and/or architecture support

Comments

@axos88
Copy link

axos88 commented Jan 16, 2025

  struct Mutation {
    sub: submodule::Mutation
  }

  #[grapql_object()]
  impl Mutation {
    fn sub(&self) -> &submodule::Mutation { &self.sub }
  }

  mod submodule {
    struct Mutation;
  
    #[grapql_object()]
    impl Mutation {
      fn foo() -> String { String::new() }
    }
  }

Currently translates Mutation and submodule::Mutation to the same graphql type named Mutation, the resulting schema being:

type Mutation {
  sub: Mutation!
}

My expected behavior would be to either error out in this case, or have the ability to either always, or on collision include the module names in the auto-generated type names.

Note that including the module path only is not bulletproof either, my current setup has the Mutation and the [Sub]Mutation defined in different crates, in the top module, so including the crate name may be required too - but the error currently occurs with submodules within a single crate too.

@axos88 axos88 added bug Something isn't working needs-triage labels Jan 16, 2025
@tyranron tyranron added support k::api Related to API (application interface) k::design Related to overall design and/or architecture and removed bug Something isn't working needs-triage labels Jan 17, 2025
@tyranron tyranron self-assigned this Jan 17, 2025
@tyranron
Copy link
Member

tyranron commented Jan 17, 2025

@axos88 no... we unlikely would make that a default behavior.

My expected behavior would be to either error out in this case

That's not good. We really need a way to have different Rust types behind the same GraphQL type. This turned out to be a very useful feature, especially when schema is gathered for types for different crates, and, let's say, they use different Rust types for DateTime GraphQL scalar (one is chrono and another one is time). Another useful application is to have a Rust type with a type parameter represent the same GraphQL input object but in different places of schema control the exact validation/semantic of it by that type parameter. Making a rule "1 Rust type corresponds to exactly 1 GraphQL type" would be too limiting.

or on collision include the module names in the auto-generated type names.

Also, not very good decision to be a default, because minor refactoring of modules structure and tossing things around will just blow your schema up.

my current setup has the Mutation and the [Sub]Mutation defined in different crates, in the top module, so including the crate name may be required too - but the error currently occurs with submodules within a single crate too.

I see this situation is quite rare, to assume it for defaults. What we can do for such situations instead, is an ability to transparent wrap any GraphQL types, preserving its semantics while allowing to slightly tweak it on-fly.

struct Mutation {
    sub: SubmoduleMutation,
}

#[grapql_object()]
impl Mutation {
    fn sub(&self) -> &SubmoduleMutation { &self.sub }
}

#[derive(GraphQLObject)]
#[graphql(transparent)]
struct SubmoduleMutation(submodule::Mutation);

mod submodule { // image this is another crate we cannot change
    struct Mutation;

    #[grapql_object]
    impl Mutation {
        fn foo() -> String { String::new() }
    }
}

We already have this for GraphQL scalars. Providing similar capability for other GraphQL types doesn't seem to introduce any problems.

@axos88
Copy link
Author

axos88 commented Jan 17, 2025

Hmm, interesting, never thought of that unexpected use case. I would still argue that that allowing collisions should be explicit, and not the default, and definately not silent. Otherwise it will result in unexpected behaviour if the collision was not intended.

The transparent wrapping would be a good solution to my problem. Not sure what transparent actually does, or what the generated typenames would be in the example above, but I trust you know what you are talking about.

This would allow the application code to be in control of what the typenames of objects defined in foreign crates will be translated to, except if they are nested of course.

@axos88
Copy link
Author

axos88 commented Jan 18, 2025

Oh, maybe the collision should error out if they are not translated to the same definition? I believe that would be the case for chrono and std time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
k::api Related to API (application interface) k::design Related to overall design and/or architecture support
Projects
None yet
Development

No branches or pull requests

2 participants