GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — ResponseMapFactory #3857 by dfa1 · Pull Request #3894 · graphql-java/graphql-java

Conversation

@dfa1

@dfa1 dfa1 commented Apr 5, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dfa1 dfa1 changed the title proposal for 3857 proposal for #3857 Apr 5, 2025
@dfa1
dfa1 force-pushed the proposal/3857-custom-map branch 5 times, most recently from 58cdde1 to bf510c2 Compare April 5, 2025 17:45
@andimarek

andimarek commented Apr 5, 2025

Copy link
Copy Markdown
Member

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
dfa1 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dfa1

dfa1 commented Apr 6, 2025

Copy link
Copy Markdown
Contributor Author

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dfa1
dfa1 force-pushed the proposal/3857-custom-map branch 2 times, most recently from 69c9ee7 to a68fee8 Compare April 6, 2025 08:51
@andimarek

andimarek commented Apr 6, 2025

Copy link
Copy Markdown
Member

@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.
dfa1 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dfa1
dfa1 force-pushed the proposal/3857-custom-map branch from a68fee8 to 8f07b72 Compare April 6, 2025 15:33
@dfa1

dfa1 commented Apr 6, 2025

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dfa1
dfa1 force-pushed the proposal/3857-custom-map branch 3 times, most recently from a5cfbd8 to fca1f2c Compare April 8, 2025 08:04
@dfa1

dfa1 commented Apr 8, 2025

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dfa1 dfa1 changed the title proposal for #3857 proposal ResponseMapFactory #3857 Apr 13, 2025
@dfa1 dfa1 changed the title proposal ResponseMapFactory #3857 ResponseMapFactory #3857 Apr 13, 2025
bbakerman
bbakerman reviewed Apr 14, 2025
View reviewed changes
Comment thread src/main/java/graphql/execution/ResponseMapFactory.java Outdated
*/
ResponseMapFactory DEFAULT = new DefaultResponseMapFactory();

Map<String, Object> create(List<String> fieldNames, List<Object> results);

@bbakerman bbakerman Apr 14, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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(...)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bbakerman bbakerman Apr 14, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

List<String> fieldNames, List<Object> results

the names here should be generic

List<String> keys, List<Object> values

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bbakerman bbakerman Apr 14, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@dfa1 dfa1 Apr 14, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bbakerman
bbakerman reviewed Apr 14, 2025
View reviewed changes
Comment thread src/main/java/graphql/execution/DefaultResponseMapFactory.java Resolved

Uh oh!

There was an error while loading. Please reload this page.
bbakerman
bbakerman reviewed Apr 14, 2025
View reviewed changes
Comment thread src/main/java/graphql/GraphQL.java Resolved

Uh oh!

There was an error while loading. Please reload this page.
@dfa1
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
dfa1 force-pushed the proposal/3857-custom-map branch from fca1f2c to bff9b62 Compare April 14, 2025 07:29
@dfa1

dfa1 commented Apr 28, 2025

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bbakerman

bbakerman commented May 5, 2025

Copy link
Copy Markdown
Member

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 ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andimarek

andimarek commented May 5, 2025

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bbakerman

bbakerman commented May 6, 2025

Copy link
Copy Markdown
Member

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bbakerman bbakerman added this to the 24.0 milestone May 6, 2025
bbakerman
bbakerman approved these changes May 6, 2025
View reviewed changes

@bbakerman bbakerman left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bbakerman
bbakerman merged commit a272575 into graphql-java:master May 6, 2025
@dfa1
dfa1 deleted the proposal/3857-custom-map branch May 6, 2025 12:53
@dfa1

dfa1 commented May 6, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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 ;)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dondonz dondonz modified the milestones: 24.0, 25.x breaking changes May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andimarek andimarek andimarek left review comments
@bbakerman bbakerman bbakerman approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

25.x breaking changes

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@dfa1 @andimarek @bbakerman @dondonz
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.