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

Add api group names for typesafe clients #2169

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

RoMiRoSSaN
Copy link
Contributor

@RoMiRoSSaN RoMiRoSSaN commented Aug 27, 2024

See #2173

Server implementation allows create api groups

@Name("Group")
@GraphQLApi
public class Api {
    /////
}

and create queries like

query {
    Group {
        .......
    }
}

But typesafe client has not this. This code

@Name("User")
@GraphQLClientApi
public interface SuperHeroesApi {
    List<User> users(List<Long> id);
}

build not correct query (it not conains User wrapper on methods)

This PR fix it

@jmartisk
Copy link
Member

Hi, thanks for your PR. But TBH I'm not yet completely sure we want this.

The grouping feature on the server side probably didn't prove very useful (it's not how GraphQL schemas are usually created) - maybe @phillip-kruger has more thoughts on that, as he implemented it? We also removed it from the docs because we don't really recommend using it.

On the client side, this would be closely tied to how specifically the SmallRye GraphQL server side does it, but mostly useless for calling GraphQL services based on anything else.

@RoMiRoSSaN
Copy link
Contributor Author

Hi Jan. I can't agree with. Grouping api by any type can create structured api. For example, in rest we can create api like
/crm/product/create
/admin/user/create
But by default in smallrye-graphql if not do groups (namespaces) methods can have unreadeble names (crmProductCreate, adminUserCreate)

Also for example, in nodejs you can do schema like (or more nesting)

type Query {
   crm: Crm
}

type Crm {
   user: User
}

type User {
   create: Response
}

I think it may be very useful for big api if create it right. Becouse in my project there are already about 1k endpoints... For example, there is methods like getCustomerRequestsManagementServiceCustomerRequestPositionsById. This is pain))

The Graphql spec allows namespaces. And I think that removed it may be not best idea, becouse it can be someone use.
As for this PR. we using third party api, which uses namespace types. And absence of this on typesafe client forces us using dynamic client.

@jmartisk
Copy link
Member

jmartisk commented Sep 2, 2024

Could you add a note into the documentation please? (probably into typesafe-client-usage.md)

@RoMiRoSSaN
Copy link
Contributor Author

@jmartisk yes. of course

@RoMiRoSSaN
Copy link
Contributor Author

@jmartisk please, check it

@jmartisk
Copy link
Member

jmartisk commented Sep 3, 2024

I can't see any documentation changes, maybe you didn't push it?
Also, I know I'm probably a bit annoying, but could you add a simple test? :) For example ClientModelBuilderTest may be a good place to do this without it getting too complex
Otherwise, looks great!

@RoMiRoSSaN
Copy link
Contributor Author

@jmartisk please, sorry. I made a mistake with PR 😑.
Yes, I all wiil do here. I think I wiil do it today

@RoMiRoSSaN
Copy link
Contributor Author

@jmartisk I have added what I think is enough detail about namespaces for both server and client. Please take a look. Maybe you'll tell me that everything is bad =)

Copy link
Member

@jmartisk jmartisk left a comment

Choose a reason for hiding this comment

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

Absolutely outstanding! Thanks!

@jmartisk jmartisk merged commit c9f4649 into smallrye:main Sep 4, 2024
5 checks passed
@jmartisk
Copy link
Member

jmartisk commented Sep 4, 2024

Btw I'm planning to release SR-GQL 2.10 around this Friday, to catch the Quarkus 3.15 release train, so if you have any more stuff that you think should be included, please let me know, I may forget things...

@RoMiRoSSaN
Copy link
Contributor Author

@jmartisk #2172 maybe too?

@jmartisk
Copy link
Member

jmartisk commented Sep 4, 2024

Yeah, merged that one too, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants