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

Automatic data loader naming on classes annotated with nameless @DgsDataLoader #904

Merged

Conversation

jord1e
Copy link
Contributor

@jord1e jord1e commented Mar 3, 2022

Pull request checklist

  • Please read our contributor guide
  • Consider creating a discussion on the discussion forum
    first
  • Make sure the PR doesn't introduce backward compatibility issues
  • Make sure to have sufficient test cases

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Changes in this PR

This PR automatically names dataloaders, which is useful when using the DgsDataFetchingEnvironment#getDataLoader(Class) method.

It also implements automatic naming of field loaders, which is arguably less useful. If anyone objects to this we can error out on nameless field dataloaders.

Something to consider is that metric and tracing solutions may expose (field?)/class/package names of the dataloader. In the future we can add a DgsDataLoaderNamingFactory or something that gives library consumers the ability to modify the name programmatically (e.g. - only use the class name instead of the FQN) - this PR does not implement this.

Partially related to #861

Alternatives considered

Name the loaders manually (gets old quickly)

@srinivasankavitha
Copy link
Contributor

Thanks for the PR. The changes look good to me, I need to test it out a bit before merging. Will do that this week.

@jord1e
Copy link
Contributor Author

jord1e commented Mar 10, 2022

Please look at the automatic field naming the closest, I am seriously debating leaving it out (and throwing a exception on unnamed field dataloaders) because refactoring the field name breaks code (fields cannot be referenced as parameters in methods

@srinivasankavitha
Copy link
Contributor

Regarding #904 (comment)

I agree, I think generating the names automatically for the field loaders feels like it may result in more confusion than help. Let's remove that and keep just the generation of class data loader names.

Otherwise looks good to me. Thanks for the PR!

Copy link
Contributor

@srinivasankavitha srinivasankavitha left a comment

Choose a reason for hiding this comment

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

We can merge the PR after you get a chance to remove the generation of names for field data loaders. Thanks!

@jord1e jord1e changed the title Automatic DgsDataLoader naming Automatic data loader naming on classes annotated with nameless @DgsDataLoader Mar 14, 2022
@jord1e
Copy link
Contributor Author

jord1e commented Mar 14, 2022

Made the following changes:

  • Fields annotated with nameless @DgsDataLoader now throw DgsUnnamedDataLoaderOnFieldException upon initialization
  • Added javadoc/ktdoc
  • DgsDataLoaderProvider refactoring to DRY up some code (did I just invent a new verb?)
  • Replaced the prefix+_+fqnClassName with just Class.getSimpleName (this is unambiguous enough in my opinion)

Copy link
Contributor

@srinivasankavitha srinivasankavitha left a comment

Choose a reason for hiding this comment

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

The changes look good to me. Will merge the Pr.

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

Successfully merging this pull request may close these issues.

None yet

2 participants