Skip to content

Debug auto-derived implementation doesn't display queries correctly #518

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

Open
edmondop opened this issue Feb 3, 2025 · 10 comments
Open

Comments

@edmondop
Copy link

edmondop commented Feb 3, 2025

If you have a query saved in a graphql file on disk, it is likely to be correctly formatted and contain newlines

            pub struct RepositoryWithPullRequests;
            pub mod repository_with_pull_requests {
                #![allow(dead_code)]
                use std::result::Result;
                pub const OPERATION_NAME: &str = "RepositoryWithPullRequests";
                pub const QUERY: &str = "query RepositoryWithPullRequests(\n  $repositoryName: String!\n  $repositoryOwner: String!\n  $count: Int!\n  $pullRequestStates: [PullRequestState!]!\n) {\n  repository(name: $repositoryName, owner: $repositoryOwner) {\n    owner {\n      __typename\n      login\n    }\n    name\n    defaultBranchRef {\n      name\n    }\n    pullRequests(first: $count, states: $pullRequestStates) {\n      pageInfo {\n        hasNextPage\n        endCursor\n      }\n\n      totalCount\n      nodes {\n        author {\n          __typename\n          login\n        }\n        id\n        url\n        title\n        state\n        createdAt\n        updatedAt\n        closedAt\n        ## uh-oh not very safe but it's ok\n        labels(first: 100) {\n          edges {\n            node {\n              id\n              name\n            }\n          }\n        }\n      }\n    }\n  }\n}\n";
                const __QUERY_WORKAROUND: &str = "query RepositoryWithPullRequests(\n  $repositoryName: String!\n  $repositoryOwner: String!\n  $count: Int!\n  $pullRequestStates: [PullRequestState!]!\n) {\n  repository(name: $repositoryName, owner: $repositoryOwner) {\n    owner {\n      __typename\n      login\n    }\n    name\n    defaultBranchRef {\n      name\n    }\n    pullRequests(first: $count, states: $pullRequestStates) {\n      pageInfo {\n        hasNextPage\n        endCursor\n      }\n\n      totalCount\n      nodes {\n        author {\n          __typename\n          login\n        }\n        id\n        url\n        title\n        state\n        createdAt\n        updatedAt\n        closedAt\n        ## uh-oh not very safe but it\'s ok\n        labels(first: 100) {\n          edges {\n            node {\n              id\n              name\n            }\n          }\n        }\n      }\n    }\n  }\n}\n";

the problem with this is that the Debug macro auto-derive will escape newline, using the Debug format unusable

@tomhoule
Copy link
Member

tomhoule commented Feb 4, 2025

I'm not sure I understand. What is __QUERY_WORKAROUND in your example? The Debug impl for strings includes escaped newlines indeed, but that's a general thing, not graphql-client specific.

@edmondop
Copy link
Author

edmondop commented Feb 4, 2025

@tomhoule I think the problem is when the macro read from files, we need to use include! rather than reading the file and re-including it, because it will escape the \n

@tomhoule
Copy link
Member

tomhoule commented Feb 5, 2025

I don't see that as a problem, since that code isn't meant to be readable by humans. The reason why the full string is included, at the time when this was done, was because include_str!() would not always detect change and trigger recompilation, so you could be editing a query, but the compiler wouldn't pick up the changes. That might have changed, and in that case we should revisit.

As it is now, I don't think we should change the Debug impl — the query string in there can use the regular Debug impl for rust strings. If you need to print the strings with real newlines, the Display impl is the canonical way to do that and the field is public. (So something like format!("{}", query_body.query).

@tomhoule
Copy link
Member

tomhoule commented Feb 5, 2025

I think what I don't understand is the upsides of a custom Debug impl. People are familiar with the default output, and QueryBody is by design a very plain struct.

@edmondop
Copy link
Author

edmondop commented Feb 5, 2025

Imagine you have code like so:

        let query = RepositoryWithPullRequests::build_query(variables);
        debug!("Github GRAPHQL Query: {}", query);
        debug!("Github GraphQL Query: {:#?}", DebugPrettyQuery(&query));

the first line wouldn't work (no Display impl), the second line output would be unreadable. This is code from another service where I am wrapping the query into a custom Struct where I implement Debug correctly

@tomhoule
Copy link
Member

tomhoule commented Feb 5, 2025

The first line wouldn't work, but debug!("Github GRAPHQL Query: {}", query.query); would work, right?

@edmondop
Copy link
Author

edmondop commented Feb 5, 2025

Yes, one option is to have two debug statements

@tomhoule
Copy link
Member

tomhoule commented Feb 5, 2025

Since all fields are public, you could also have a wrapper around QueryBody for your own debug formatting, like:

struct QueryBodyCustomDebug(QueryBody);

impl Debug for QueryBodyCustomDebug {
// ...
}

@edmondop
Copy link
Author

edmondop commented Feb 5, 2025

That's what I did. My suggestion is to have a "readable Debug implementation" out of the box, rather than having people to reimplement it themselves. What's the benefit of having an unreadable implementation? This can also be achieved by modifying the codegen macro

@caspermeijn
Copy link
Contributor

Could you add an example of the current debug output and your suggested output to make the discussion easier?

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

No branches or pull requests

3 participants