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

Dependency on yasson #2094

Closed
t1 opened this issue Apr 23, 2024 · 3 comments
Closed

Dependency on yasson #2094

t1 opened this issue Apr 23, 2024 · 3 comments

Comments

@t1
Copy link
Collaborator

t1 commented Apr 23, 2024

I just found the following code in RequestImpl that necessitates the dependency on yasson, which we should get rid of.

try (Jsonb jsonb = JsonbBuilder.create()) {
    JsonStructure struct = ((JsonBinding) jsonb).toJsonStructure(v);
    varBuilder.add(k, struct);
} catch (Exception ignore) {
}

Why is this not simply varBuilder.add(k, jsonb.toJson(v)); and why is the exception ignored?

If there is a valid reason, this deserves a comment. Anyone any idea? @hogmuzzle maybe?

@t1
Copy link
Collaborator Author

t1 commented Apr 23, 2024

Sorry, I meant varBuilder.add(k, Json.createReader(new StringReader(jsonb.toJson(v))).read());

If this is about performance, has anybody measured that? I think the JDK might very well be able to inline this.

@jmartisk
Copy link
Member

We bring in Yasson anyway so I guess using its classes wasn't considered a problem, but if we can remove that, then why not.
I'm not sure how often that branch is used though - if it's a complex Json object, that's caught already by else if (v instanceof JsonValue) no?

@t1
Copy link
Collaborator Author

t1 commented Apr 24, 2024

We bring in Yasson anyway so I guess using its classes wasn't considered a problem, but if we can remove that, then why not.

It's also a dependency in the client/implementation module, but it's not used anywhere. Maybe we should make it optional or even remove it: BYOJB - bring your own JsonB 😁 In the server, it's only a test dependency.

I'm not sure how often that branch is used though - if it's a complex Json object, that's caught already by else if (v instanceof JsonValue) no?

That catches the JsonValue parameters, but that final else is for arbitrary Java objects.

t1 added a commit to t1/smallrye-graphql that referenced this issue Apr 24, 2024
t1 added a commit to t1/smallrye-graphql that referenced this issue Jun 22, 2024
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

2 participants