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

VertxDynamicGraphQLClient #976

Closed
hogmuzzle opened this issue Aug 15, 2021 · 7 comments
Closed

VertxDynamicGraphQLClient #976

hogmuzzle opened this issue Aug 15, 2021 · 7 comments

Comments

@hogmuzzle
Copy link
Contributor

In RequestImpl

private JsonObject _formatJsonVariables() {
JsonObjectBuilder varBuilder = Json.createObjectBuilder();

    variables.forEach((k, v) -> {
        // Other types to process here
        if (v instanceof String) {
            varBuilder.add(k, (String) v);
        } else if (v instanceof Integer) {
            varBuilder.add(k, (Integer) v);
        }
    });

    return varBuilder.build();
}

Why only String and Integer why can not I keep maps in variables? JsonBuilderFactory can create nested objects as well.

@hogmuzzle
Copy link
Contributor Author

hogmuzzle commented Aug 15, 2021

I changed to

private JsonObject _formatJsonVariables() {
        JsonObjectBuilder varBuilder = Json.createObjectBuilder();

        variables.forEach((k, v) -> {
            // Other types to process here
            if (v instanceof String) {
                varBuilder.add(k, (String) v);
            } else if (v instanceof Integer) {
                varBuilder.add(k, (Integer) v);
            } else {
                try(Jsonb jsonb = JsonbBuilder.create()) {
                    JsonStructure str = ((JsonBinding)jsonb).toJsonStructure(v);
                    varBuilder.add(k, str);

                } catch (Exception ignore) {}
            }
        });

        return varBuilder.build();
    }

@t1
Copy link
Collaborator

t1 commented Aug 16, 2021

It would be cool if you could create a PR for this! If you are not sure about how to do that, just ask.

@jmartisk
Copy link
Member

+1 for a PR. Just a few comments:

  • Maybe we should also allow the user to pass a JsonValue directly if they prefer? so,
} else if (v instanceof JsonValue) {
   varBuilder.add(k, (JsonValue)v);
}
  • Adding a small test into io.smallrye.graphql.tests.client.dynamic.DynamicClientTest would be cool too! Similar to testQueryWithVars, just a bit more complex)

@hogmuzzle
Copy link
Contributor Author

How can I create a PR? I cloned the repo and made modifications to code and tests but when I try to push my changes to a new branch I get an 403 error.

@phillip-kruger
Copy link
Member

You can make modifications in your own fork, and then open a PR from there.

@t1
Copy link
Collaborator

t1 commented Aug 24, 2021

The documentation in GitHub is actually very good. Maybe this page is what you need.

@jmartisk
Copy link
Member

done in #996

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

4 participants