ResponseMapFactory #3857 by dfa1 · Pull Request #3894 · graphql-java/graphql-java · GitHub

Proposal for #3857. I would like to write a unit test too: the idea is to make sure that all nested maps are really built with the customization.

Thanks for the PR @dfa1.

In general we prefer not to open up ExecutionStrategy more or encourage people to derive from it.

Can we instead define an abstract Factory method or so that can be provided via config, similar to ValueUnboxer? thx

@andimarek sounds good... I have several concrete classes deriving from ExecutionStrategy (and feels wrong actually, because I'm tapping into the internals).

What could be a good name? MapFactory?

@andimarek sounds good... I have several concrete classes deriving from ExecutionStrategy (and feels wrong actually, because I'm tapping into the internals).

What could be a good name? MapFactory?

Let's go with ResponseMapFactory for now.

@andimarek done... please have another look and feel free to suggest better javadoc ;)

@andimarek @bbakerman I just rebased to fix some conflicts, do you like the current state? do you expect more javadoc?

I feel like this should be generic

    <K,V> Map<K, V> create(List<K> fieldNames, List<V> results);

Also naming matters

The map we give back is insertion ordered. This is important at times

So I think the create method should be createInsertionOrdered(...)

List<String> fieldNames, List<Object> results

the names here should be generic

List<String> keys, List<Object> values

at some time in the future we might be using this to make more "maps" in different places say

thanks for the feedback! I covered all the points but one: I left out the generics because it is in contrast with a comment from @andimarek.

I do see 2 alternatives:

use MapFactory and have the generics
use ResponseMapFactory and drop the generics (as they will always K=String and V=Object)

it is acceptance to keep ResponseMapFactory without generics by now?

This will allows to customize which java.util.Map concrete class to use. For example, eclipse-collections has Map implementations with better memory footprint than JDK maps.

@andimarek @bbakerman hi guys, this PR is ready to be reviewed... or do you see something else missing?

I am happy to take this PR in shape HOWEVER I dont think we should wire the MapResponseFactory into the graphql object

This is unsual config - we dont expect people to use this very often.

As such I think we should wire this into play via the new unusualConfiguration() mechanism

#3945

If we had out time again I would do this for ValueUnboxer as well but that is a past decision and this is a present once

So while I support the idea of a MapResponseFactory - it is not usual to expect this to be set and its clutters the API.

Thoughts @andimarek ?

I agree with @bbakerman here .. perfect for our very new unusual config area.

second question @dfa1 : does it need to be a function doing so much? could it be a just a constructor/factory function?

@dfa1 - we are likely to merge this PR and then change so that ResponseMapFactory is not set per GraphQL object but rather per execution.

This fits in with our new "unusual configutation" mechanism AND it declutters the public common API

Approving but we are going to fix this up to the shape we want. This will be set per request not per GraphQL

thanks @bbakerman @andimarek ! 💯

second question @dfa1 : does it need to be a function doing so much? could it be a just a constructor/factory function?

@andimarek : yes, also a factory function is fine

Approving but we are going to fix this up to the shape we want. This will be set per request not per GraphQL

@bbakerman in our projects, all requests will use same ResponseMapFactory...right now I cannot think a usecase to customize this per request rather per instance).

We are using GraphQL.tranform extensively (and it is working nicely, by caching the instances via caffeine) to customize the behavior per request.

... of course, I'm going to change our code to use the "unusual configuration" mechanism you're proposing ;)

dfa1 changed the title proposal for 3857 proposal for #3857 Apr 5, 2025

dfa1 force-pushed the proposal/3857-custom-map branch 5 times, most recently from 58cdde1 to bf510c2 Compare April 5, 2025 17:45

dfa1 force-pushed the proposal/3857-custom-map branch 2 times, most recently from 69c9ee7 to a68fee8 Compare April 6, 2025 08:51

dfa1 force-pushed the proposal/3857-custom-map branch from a68fee8 to 8f07b72 Compare April 6, 2025 15:33

dfa1 force-pushed the proposal/3857-custom-map branch 3 times, most recently from a5cfbd8 to fca1f2c Compare April 8, 2025 08:04

dfa1 changed the title proposal for #3857 proposal ResponseMapFactory #3857 Apr 13, 2025

dfa1 changed the title proposal ResponseMapFactory #3857 ResponseMapFactory #3857 Apr 13, 2025

bbakerman reviewed Apr 14, 2025

View reviewed changes

Comment thread src/main/java/graphql/execution/DefaultResponseMapFactory.java

bbakerman reviewed Apr 14, 2025

View reviewed changes

Comment thread src/main/java/graphql/GraphQL.java Outdated

Introduce ResponseMapFactory

bff9b62

This will allows to customize which java.util.Map concrete class to use. For example, eclipse-collections has Map implementations with better memory footprint than JDK maps.

dfa1 force-pushed the proposal/3857-custom-map branch from fca1f2c to bff9b62 Compare April 14, 2025 07:29

bbakerman added this to the 24.0 milestone May 6, 2025

bbakerman approved these changes May 6, 2025

View reviewed changes

bbakerman merged commit a272575 into graphql-java:master May 6, 2025

dfa1 deleted the proposal/3857-custom-map branch May 6, 2025 12:53

dondonz modified the milestones: 24.0, 25.x breaking changes May 12, 2025

+                   */
+                  ResponseMapFactory DEFAULT = new DefaultResponseMapFactory();
+                  Map<String, Object> create(List<String> fieldNames, List<Object> results);

ResponseMapFactory #3857 by dfa1 · Pull Request #3894 · graphql-java/graphql-java · GitHub

Uh oh!

ResponseMapFactory #3857 - #3894

ResponseMapFactory #3857#3894bbakerman merged 1 commit intographql-java:mastergraphql-java/graphql-java:masterfrom dfa1:proposal/3857-custom-mapdfa1/graphql-java:proposal/3857-custom-mapCopy head branch name to clipboard

Conversation

dfa1 commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andimarek commented Apr 5, 2025

Uh oh!

dfa1 commented Apr 6, 2025

Uh oh!

andimarek commented Apr 6, 2025

Uh oh!

dfa1 commented Apr 6, 2025

Uh oh!

dfa1 commented Apr 8, 2025

Uh oh!

bbakerman Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

bbakerman Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

bbakerman Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

dfa1 Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dfa1 commented Apr 28, 2025

Uh oh!

bbakerman commented May 5, 2025

Uh oh!

andimarek commented May 5, 2025

Uh oh!

bbakerman commented May 6, 2025

Uh oh!

bbakerman left a comment

Choose a reason for hiding this comment

Uh oh!

dfa1 commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ResponseMapFactory #3857#3894
bbakerman merged 1 commit into
graphql-java:masterfrom
dfa1:proposal/3857-custom-map

dfa1 commented Apr 5, 2025 •
edited
Loading

dfa1 Apr 14, 2025 •
edited
Loading

dfa1 commented May 6, 2025 •
edited
Loading